From: Tom Lane <tgl@sss.pgh.pa.us>
To: Akshat Jaimini <destrex271@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org, Jelte Fennema-Nio <postgres@jeltef.nl>
Subject: Re: Always show correct error message for statement timeouts, fixes random buildfarm failures
Date: Thu, 08 Jan 2026 12:50:52 -0500
Message-ID: <1977913.1767894652@sss.pgh.pa.us> (raw)
In-Reply-To: <176511833932.1475507.5111967663635215418.pgcf@coridan.postgresql.org>
References: <CAGECzQQD5f9ce-D1v6vCTT94hG=JbGdTHuf8vQ=HBLz9T4dA1g@mail.gmail.com>
<CAGECzQSP17h9nB1LDndKRk4MXEjC17CLyZbVKbLNz39CjL7Rcw@mail.gmail.com>
<176511833932.1475507.5111967663635215418.pgcf@coridan.postgresql.org>
Akshat Jaimini <destrex271@gmail.com> writes:
> Just reviewed the patch and it seems harmless. I think we should commit this.
I took another look and I still don't like it. The intent of the code
is that once we're out of a query, a query cancel that arrived
meanwhile should be a no-op. I completely disagree with the idea that
a late-arriving cancel should cancel the next query. (From the user's
standpoint: if you're a bit slow to hit control-C and so your query
finishes anyway, and then you go for coffee, would you be surprised
to come back awhile later and enter a new query and have it fail?)
And I think the same should apply to statement timeouts: if one
happens slightly too late to cancel the current query, that does not
mean it should cancel the next one.
That's not to say that I don't think there's anything to do about
this. I just don't think this is what to do.
Looking around at the code, I think perhaps some blame should fall
on this bit in PostgresMain:
/*
* (5) disable async signal conditions again.
*
* Query cancel is supposed to be a no-op when there is no query in
* progress, so if a query cancel arrived while we were idle, just
* reset QueryCancelPending. ProcessInterrupts() has that effect when
* it's called when DoingCommandRead is set, so check for interrupts
* before resetting DoingCommandRead.
*/
CHECK_FOR_INTERRUPTS();
DoingCommandRead = false;
We are taking care to discard a stale QueryCancelPending flag, but
this code is ignorant of the fact that timeouts now also feed into
QueryCancelPending. Perhaps we should clear the timeout indicators
here? Or maybe using CHECK_FOR_INTERRUPTS here is too cute and
we should just clear QueryCancelPending directly (and clear the
indicators too, likely).
Also, I could buy making the proposed change in
disable_statement_timeout without the one in enable_statement_timeout.
I think that would have the desired effect that if ProcessInterrupts
runs shortly after disable_statement_timeout it would not lie about
the cause of the cancel, while not sacrificing the principle that
a statement timeout can't kill the next statement.
As a more thorough redesign, maybe we should get rid of the
idea that QueryCancelPending represents all three of these
conditions. That would mean a bit more duplication of logic
in ProcessInterrupts, but I don't think that code is especially
performance-critical.
regards, tom lane
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, destrex271@gmail.com, postgres@jeltef.nl
Subject: Re: Always show correct error message for statement timeouts, fixes random buildfarm failures
In-Reply-To: <1977913.1767894652@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox