Hi, While running a stress test on a patch I'm working on, I ran into two annoying race conditions with statement_timeout. It's not a huge issue, most people either don't have statement_timeout set at all, or there's sufficient margin to not hit the timeout very often. I've since mitigated that in my test script, but it was annoying and it took me a while to figure out what's going to (and that it's not a bug in the new code I was testing).
1) statement_timeout can kill the next command instead
Roughly what happens is that the command takes just about the timeout,
the timeout fires, but the statement does not check interrupts before it
ends, and we leave the cancel request as pending. Then we run the next
command (which could be as simple as "SET guc"), and that gets killed by
the timeout. That's exactly what I saw with statement_timeout = '60s',
SET statements getting aborted.
This happens because isable_statement_timeout() does nothing if
get_timeout_active() says the timeout is no longer active, which is the
case once it has fired. So the indicator stays set, and then is reported
as a statement timeout against whatever comes next (without being
executed at all).
I'm attaching a SQL reproducer for that (repro-exec.sql), leveraging the
fact that lpad() does not check interrupts even for very long values
(which we should probably fix, "repeat" already does that). And
CALL/FETCH don't either.
The attached 0001 fixes this race by doing pretty much the same thing as
ProcessInterrupts() for QueryCancelPending - checking both lock/timeout
indicators, and resetting the flag. With this the reproducer no longer
reproduces the unexpected interruptions.
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).
regards
--
Tomas Vondra
From f179240230d91661ef31e0992614e1e9c0be7e08 Mon Sep 17 00:00:00 2001 From: Tomas Vondra <[email protected]> Date: Mon, 9 Nov 2026 00:30:25 +0100 Subject: [PATCH v1 2/2] Don't restart a statement timeout that has already fired. Since commit 22f6f2c1ccb, enable_statement_timeout() relies on get_timeout_active() to avoid restarting a running timeout, but that returns false once the timeout has fired. If the timeout fires while exec_simple_query() parses a long query string, which doesn't check for interrupts, the next start_xact_command() restarts it, resetting its indicator, so the pending cancel request is reported as "canceling statement due to user request" instead of a statement timeout. To fix, don't restart the timer while the indicator is set. This relies on the previous commit resetting the indicator at the end of each statement; otherwise a stale indicator could keep later statements from being timed out. Back-patch to all supported branches, like the previous commit. Author: Tomas Vondra <[email protected]> Reviewed-by: FIXME Discussion: FIXME Backpatch-through: 15 --- src/backend/tcop/postgres.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index 6d301f8102b..ee75f81194f 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -5378,7 +5378,10 @@ log_disconnections(int code, Datum arg) * * If there's already a timeout running, don't restart the timer. That * enables compromises between accuracy of timeouts and cost of starting a - * timeout. + * timeout. Likewise, don't restart it if it has already fired, but the query + * cancel request it sent hasn't been serviced yet (e.g. because it fired + * while parsing a long query string). Restarting it would reset its + * indicator, causing the request to be reported as a user request. */ static void enable_statement_timeout(void) @@ -5389,7 +5392,8 @@ enable_statement_timeout(void) if (StatementTimeout > 0 && (StatementTimeout < TransactionTimeout || TransactionTimeout == 0)) { - if (!get_timeout_active(STATEMENT_TIMEOUT)) + if (!get_timeout_active(STATEMENT_TIMEOUT) && + !get_timeout_indicator(STATEMENT_TIMEOUT, false)) enable_timeout_after(STATEMENT_TIMEOUT, StatementTimeout); } else -- 2.55.0
From 7295c719e3c0c392d5de1f4f54433cad7f6e75b2 Mon Sep 17 00:00:00 2001 From: Tomas Vondra <[email protected]> Date: Mon, 9 Nov 2026 00:26:55 +0100 Subject: [PATCH v1 1/2] Discard pending timeout cancel requests at end of statement. If statement_timeout fires and the statement then completes without reaching a CHECK_FOR_INTERRUPTS(), the timeout's cancel request is left pending. Before commit 22f6f2c1ccb (v13), disable_statement_timeout() reset the timeout's indicator, so the request was ignored while reading the next command. Since then, it does nothing once the timeout has fired, so the next command fails with a statement timeout without being executed. This is relatively easy to hit without special privileges, e.g. with CALL or FETCH, which don't check for interrupts after evaluating a long-running expression. Of course, most systems either don't have the statement_timeout set at all, or there's a significant margin. To fix, make disable_statement_timeout() always fetch and reset the indicator, and discard the pending request if it was set. Likewise for lock_timeout, which can leave a request behind if it fires just as the lock is granted. ProcessInterrupts() uses this pattern too. This also discards a user's cancel request arriving at about the same time, which seems acceptable, as the statement has completed anyway. Back-patch to all supported branches. Author: Tomas Vondra <[email protected]> Reviewed-by: FIXME Discussion: FIXME Backpatch-through: 15 --- src/backend/tcop/postgres.c | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index b6bdfe213fe..6d301f8102b 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -5401,10 +5401,32 @@ enable_statement_timeout(void) /* * Disable statement timeout, if active. + * + * If the statement timeout has already fired, but the statement completed + * without reaching a CHECK_FOR_INTERRUPTS() afterwards, the query cancel + * request sent by the timeout is still pending. Discard it, as otherwise + * it would cancel whatever we do next. This also discards a cancel request + * from the user that arrived at about the same time, since we can't tell + * them apart, but that seems acceptable: the statement has completed anyway. */ static void disable_statement_timeout(void) { + bool lock_timeout_occurred; + bool stmt_timeout_occurred; + + /* Keep the indicator in case the timeout fires right now; see below */ if (get_timeout_active(STATEMENT_TIMEOUT)) - disable_timeout(STATEMENT_TIMEOUT, false); + disable_timeout(STATEMENT_TIMEOUT, true); + + /* + * A lock timeout that fired just as the lock was granted can leave a + * pending cancel request behind in the same way (see ProcSleep()). Always + * fetch and reset both indicators, as in ProcessInterrupts(). + */ + lock_timeout_occurred = get_timeout_indicator(LOCK_TIMEOUT, true); + stmt_timeout_occurred = get_timeout_indicator(STATEMENT_TIMEOUT, true); + + if (lock_timeout_occurred || stmt_timeout_occurred) + QueryCancelPending = false; } -- 2.55.0
repro-parse.sql
Description: application/sql
repro-exec.sql
Description: application/sql
