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
