Thread: pgsql: Fix (some of the) breakage introduced into query-cancel

Log Message:
-----------
Fix (some of the) breakage introduced into query-cancel processing by HS.

It is absolutely not okay to throw an ereport(ERROR) in any random place in
the code just because DoingCommandRead is set; interrupting, say, OpenSSL
in the midst of its activities is guaranteed to result in heartache.

Instead of that, undo the original optimizations that threw away
QueryCancelPending anytime we were starting or finishing a command read, and
instead discard the cancel request within ProcessInterrupts if we find that
there is no HS reason for forcing a cancel and we are DoingCommandRead.

In passing, may I once again condemn the practice of changing the code
and not fixing the adjacent comment that you just turned into a lie?

Modified Files:
--------------
    pgsql/src/backend/tcop:
        postgres.c (r1.580 -> r1.581)
        (http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/tcop/postgres.c?r1=1.580&r2=1.581)

On Thu, 2010-01-07 at 16:29 +0000, Tom Lane wrote:

> Fix (some of the) breakage introduced into query-cancel processing by HS.

> It is absolutely not okay to throw an ereport(ERROR) in any random place in
> the code just because DoingCommandRead is set; interrupting, say, OpenSSL
> in the midst of its activities is guaranteed to result in heartache.

...That was not introduced by HS.

> Instead of that, undo the original optimizations that threw away
> QueryCancelPending anytime we were starting or finishing a command read, and
> instead discard the cancel request within ProcessInterrupts if we find that
> there is no HS reason for forcing a cancel and we are DoingCommandRead.
>
> In passing, may I once again condemn the practice of changing the code
> and not fixing the adjacent comment that you just turned into a lie?

You may.

--
 Simon Riggs           www.2ndQuadrant.com

Simon Riggs <simon@2ndquadrant.com> writes:
> On Thu, 2010-01-07 at 16:29 +0000, Tom Lane wrote:
>> Fix (some of the) breakage introduced into query-cancel processing by HS.

>> It is absolutely not okay to throw an ereport(ERROR) in any random place in
>> the code just because DoingCommandRead is set; interrupting, say, OpenSSL
>> in the midst of its activities is guaranteed to result in heartache.

> ...That was not introduced by HS.

Yes it was; or at least it was in your HS commit:

http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/tcop/postgres.c.diff?r1=1.578;r2=1.579

            regards, tom lane

On Thu, 2010-01-07 at 15:29 -0500, Tom Lane wrote:
> Simon Riggs <simon@2ndquadrant.com> writes:
> > On Thu, 2010-01-07 at 16:29 +0000, Tom Lane wrote:
> >> Fix (some of the) breakage introduced into query-cancel processing by HS.
>
> >> It is absolutely not okay to throw an ereport(ERROR) in any random place in
> >> the code just because DoingCommandRead is set; interrupting, say, OpenSSL
> >> in the midst of its activities is guaranteed to result in heartache.
>
> > ...That was not introduced by HS.
>
> Yes it was; or at least it was in your HS commit:
>
> http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/tcop/postgres.c.diff?r1=1.578;r2=1.579

Ah, you were referring to those lines. I thought you were referring to
the additional lines you put in around ClientAuthInProgress.

--
 Simon Riggs           www.2ndQuadrant.com

Simon Riggs <simon@2ndQuadrant.com> writes:
> Ah, you were referring to those lines. I thought you were referring to
> the additional lines you put in around ClientAuthInProgress.

That's not really additional code --- I was just moving (and duplicating
:-() some of the steps so that they wouldn't get executed if we choose
to fall out without processing the query cancel request.  The previous
logic here was essentially to prevent ProcessInterrupts from being
called at all while DoingCommandRead.  The new logic is to call it and
have it understand that it shouldn't do anything if DoingCommandRead
(except when the HS state indicates an override).

            regards, tom lane

On Thu, 2010-01-07 at 16:29 +0000, Tom Lane wrote:

> Instead of that, undo the original optimizations that threw away
> QueryCancelPending anytime we were starting or finishing a command
> read, and instead discard the cancel request within ProcessInterrupts
> if we find that there is no HS reason for forcing a cancel and we are
> DoingCommandRead.

Is there a reason why we are calling DisableNotifyInterrupt() and
DisableCatchupInterrupt() before we call ProcessInterrupts() and again
within it?

--
 Simon Riggs           www.2ndQuadrant.com

Simon Riggs <simon@2ndquadrant.com> writes:
> On Thu, 2010-01-07 at 16:29 +0000, Tom Lane wrote:
>> Instead of that, undo the original optimizations that threw away
>> QueryCancelPending anytime we were starting or finishing a command
>> read, and instead discard the cancel request within ProcessInterrupts
>> if we find that there is no HS reason for forcing a cancel and we are
>> DoingCommandRead.

> Is there a reason why we are calling DisableNotifyInterrupt() and
> DisableCatchupInterrupt() before we call ProcessInterrupts() and again
> within it?

Different purposes.  ProcessInterrupts is called from a lot of places
and should not assume the status of those flags --- in particular, if it
is called directly from the signal handler while we are waiting for a
command, those disables are needed because the interrupts are active.
The call site you are probably looking at is where we are exiting the
active-command-read state, and we want to disable the interrupts before
beginning active processing, whether or not there is any cancel interrupt
waiting.

            regards, tom lane