Re: two statement_timeout race conditions

Hi,

On Sat, 14 Nov 2026 at 01:14, Tomas Vondra <tomas(at)vondra(dot)me> wrote:
>
> 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.

Yup, I went through the QueryCancelPending = false sites too, and syncrep
is the only one that neither resets the indicators nor exits. Fixing it
there is better than my check, fine with me. Thanks for the update!

> 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.

Makes sense.

Regards,
Ayush