From dec91d1e6402bb827696506e2ac30b04645450ed Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Mon, 9 Nov 2026 00:30:25 +0100 Subject: [PATCH v2 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. The only relevant place resetting QueryCancelPending without also resetting the indicator was SyncRepWaitForLSN(). When that happened, later commands would not have a statement timeout active. Fix by resetting both indicators (for statement and lock timeouts) in SyncRepWaitForLSN() too. Author: Tomas Vondra Reviewed-by: Chao Li Reviewed-by: Ayush Tiwari Reviewed-by: surya poondla Discussion: https://postgr.es/m/50fabd6e-7006-42e5-b3f5-a90431db5055@vondra.me --- src/backend/replication/syncrep.c | 11 +++++++++++ src/backend/tcop/postgres.c | 21 +++++++++++++-------- 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c index d870f09e0a0..1216baf0de2 100644 --- a/src/backend/replication/syncrep.c +++ b/src/backend/replication/syncrep.c @@ -85,6 +85,7 @@ #include "tcop/tcopprot.h" #include "utils/guc_hooks.h" #include "utils/ps_status.h" +#include "utils/timeout.h" #include "utils/wait_event.h" /* User-settable parameters for sync rep */ @@ -323,10 +324,20 @@ SyncRepWaitForLSN(XLogRecPtr lsn, bool commit) * can't actually abort at this point, but ignoring the interrupt * altogether is not helpful, so we just terminate the wait with a * suitable warning. + * + * The request may have come from the statement or lock timeout, so + * reset their indicators, as ProcessInterrupts() does. We do this + * for two reasons. First, enable_statement_timeout() doesn't start + * the timer while the statement timeout's indicator is set, so the + * next statement would run without a statement timeout. Second, a + * query cancel arriving while waiting for the next message would be + * reported as a timeout, rather than ignored. */ if (QueryCancelPending) { QueryCancelPending = false; + (void) get_timeout_indicator(LOCK_TIMEOUT, true); + (void) get_timeout_indicator(STATEMENT_TIMEOUT, true); ereport(WARNING, (errmsg("canceling wait for synchronous replication due to user request"), errdetail("The transaction has already committed locally, but might not have been replicated to the standby."))); diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index a1c63bcd504..bb362dbb81c 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -5375,7 +5375,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) @@ -5386,7 +5389,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 @@ -5399,12 +5403,13 @@ 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. + * This is called whenever a statement completes. 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) -- 2.56.0