Re: two statement_timeout race conditions

On 11/9/26 09:49, Ayush Tiwari wrote:
>
> ...
>>
>> 2) statement timeout reported as "user request" cancellation
>>
>> While looking at the code and trying to come up with reproducers for the
>> first issue, I ran into a second race, where we correctly cancel the
>> query due to a timeout, but report it happened due to a "user request".
>>
>> It can happen if the timeout expires while parsing a long query string
>> (yes, that's not very common), because that does not check interrupts.
>> In exec_simple_query() we start the timer, do parsing etc., and then
>> call enable_statement_timeout() again, which does this:
>>
>> if (!get_timeout_active(STATEMENT_TIMEOUT) &&
>> !get_timeout_indicator(STATEMENT_TIMEOUT, false))
>> enable_timeout_after(STATEMENT_TIMEOUT, StatementTimeout);
>>
>> But get_timeout_active(() returns false if the timer already fired, so
>> the timer gets restarted, but leaves QueryCancelPending set. And so on
>> the next CHECK_FOR_INTERRUPTS() the query gets cancelled with the
>> misleading "canceling statement due to user request" reason.
>>
>> I'm attaching a SQL reproducer for this too (repro-parse.sql), hitting
>> this. It's a bit artificial, I don't think I've seen this in practice
>> (even though some of the queries the stress test produces are long and
>> complex). 0002 fixes that by not restarting the timer that has fired
>> (which relies on the previous commit).
>
> I was poking at this a bit, and I think it would be good to also check
> QueryCancelPending there, i.e. only skip the restart while the cancel
> is actually still pending:
>
> if (!get_timeout_active(STATEMENT_TIMEOUT) &&
> !(QueryCancelPending &&
> get_timeout_indicator(STATEMENT_TIMEOUT, false)))
> enable_timeout_after(STATEMENT_TIMEOUT, StatementTimeout);
>
> Otherwise, if the cancel already got handled somewhere else (e.g. in the
> sync rep wait), the indicator stays set and the next query can end up
> running without any statement timeout.
>
> It's a pretty narrow case, but seems worth fixing.
>

Yeah, thanks for noticing that. I agree this this can happen, and it
would prevent starting the statement timeout after that. But I'm not
sure this is the right fix, as it seems to contradict the comment before
enable_statement_timeout (and the contract for these functions).

The 0001 commit should probably spell out the behavior/contract change
more explicitly. Before the commit, enable_statement_timeout() would
start the timeout unless it's already active. With 0001 the timeout is
started only if it's neither active nor fired. I think adding the
QueryCancelPending check would make this less clear.

I was wondering if syncrep is the only place with this issue, and it
seem it is. The other places clearing QueryCancelPending seems safe:

* Autovacuum workers force statement_timeout and lock_timeout to 0, so
those timeouts are never started in that context.

* Error recovery in PostgresMain() and in the autovacuum launcher calls
disable_all_timeouts(false) first, which resets them.

* ProcessInterrupts() and disable_statement_timeout() reset both timeout
indicators themselves.

* ProcessInterrupts() and proc_exit_prepare() clear the flag right
before the process exits, so who cares if the timeouts run.

The non-exit cases are safe because they reset/disable the timeouts,
making the stale indicator impossible. Syncrep is the only case not
doing that. I think that's the root cause, and it'd be better to fix
that by doing roughly:

if (QueryCancelPending)
{
QueryCancelPending = false;
(void) get_timeout_indicator(LOCK_TIMEOUT, true);
(void) get_timeout_indicator(STATEMENT_TIMEOUT, true);
ereport(WARNING, ...);
}

Attached is v2 doing it this way.

BTW the earlier patches suggested I'm planning to backpatch this (to all
supported branches), but I've changed my mind and probably won't do
that. At least not initially. I've not seen a single report about these
issues, it's not worth the risk.

regards

--
Tomas Vondra