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
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>
Comments: In-reply-to Akshat Jaimini <destrex271@gmail.com>
	message dated "Sun, 07 Dec 2025 14:38:59 +0000"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1977912.1767894652.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Thu, 08 Jan 2026 12:50:52 -0500
Message-ID: <1977913.1767894652@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1977913.1767894652%40sss.pgh.pa.us>
Precedence: bulk

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 i=
n
         * progress, so if a query cancel arrived while we were idle, just
         * reset QueryCancelPending. ProcessInterrupts() has that effect w=
hen
         * it's called when DoingCommandRead is set, so check for interrup=
ts
         * before resetting DoingCommandRead.
         */
        CHECK_FOR_INTERRUPTS();
        DoingCommandRead =3D 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