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?

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index a648bdf22abb9053c5c18acd1433e86bdbf0b606..28560b940830244d014c8832ee620cce2b437c2b 100644 (file)
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/backend/tcop/postgres.c,v 1.580 2010/01/02 16:57:52 momjian Exp $
+ *   $PostgreSQL: pgsql/src/backend/tcop/postgres.c,v 1.581 2010/01/07 16:29:58 tgl Exp $
  *
  * NOTES
  *   this is the "main" module of the postgres backend and
@@ -476,11 +476,10 @@ prepare_for_client_read(void)
        EnableNotifyInterrupt();
        EnableCatchupInterrupt();
 
-       /* Allow "die" interrupt to be processed while waiting */
+       /* Allow cancel/die interrupts to be processed while waiting */
        ImmediateInterruptOK = true;
 
        /* And don't forget to detect one that already arrived */
-       QueryCancelPending = false;
        CHECK_FOR_INTERRUPTS();
    }
 }
@@ -494,7 +493,6 @@ client_read_ended(void)
    if (DoingCommandRead)
    {
        ImmediateInterruptOK = false;
-       QueryCancelPending = false;     /* forget any CANCEL signal */
 
        DisableNotifyInterrupt();
        DisableCatchupInterrupt();
@@ -2640,12 +2638,11 @@ StatementCancelHandler(SIGNAL_ARGS)
        QueryCancelPending = true;
 
        /*
-        * If it's safe to interrupt, and we're waiting for a lock, service
-        * the interrupt immediately.  No point in interrupting if we're
-        * waiting for input, however.
+        * If it's safe to interrupt, and we're waiting for input or a lock,
+        * service the interrupt immediately
         */
-       if (InterruptHoldoffCount == 0 && CritSectionCount == 0 &&
-           (DoingCommandRead || ImmediateInterruptOK))
+       if (ImmediateInterruptOK && InterruptHoldoffCount == 0 &&
+           CritSectionCount == 0)
        {
            /* bump holdoff count to make ProcessInterrupts() a no-op */
            /* until we are done getting ready for it */
@@ -2717,25 +2714,36 @@ ProcessInterrupts(void)
    if (QueryCancelPending)
    {
        QueryCancelPending = false;
-       ImmediateInterruptOK = false;   /* not idle anymore */
-       DisableNotifyInterrupt();
-       DisableCatchupInterrupt();
-       /* As in quickdie, don't risk sending to client during auth */
-       if (ClientAuthInProgress && whereToSendOutput == DestRemote)
-           whereToSendOutput = DestNone;
        if (ClientAuthInProgress)
+       {
+           ImmediateInterruptOK = false;   /* not idle anymore */
+           DisableNotifyInterrupt();
+           DisableCatchupInterrupt();
+           /* As in quickdie, don't risk sending to client during auth */
+           if (whereToSendOutput == DestRemote)
+               whereToSendOutput = DestNone;
            ereport(ERROR,
                    (errcode(ERRCODE_QUERY_CANCELED),
                     errmsg("canceling authentication due to timeout")));
-       else if (cancel_from_timeout)
+       }
+       if (cancel_from_timeout)
+       {
+           ImmediateInterruptOK = false;   /* not idle anymore */
+           DisableNotifyInterrupt();
+           DisableCatchupInterrupt();
            ereport(ERROR,
                    (errcode(ERRCODE_QUERY_CANCELED),
                     errmsg("canceling statement due to statement timeout")));
-       else if (IsAutoVacuumWorkerProcess())
+       }
+       if (IsAutoVacuumWorkerProcess())
+       {
+           ImmediateInterruptOK = false;   /* not idle anymore */
+           DisableNotifyInterrupt();
+           DisableCatchupInterrupt();
            ereport(ERROR,
                    (errcode(ERRCODE_QUERY_CANCELED),
                     errmsg("canceling autovacuum task")));
-       else
+       }
        {
            int cancelMode = MyProc->recoveryConflictMode;
 
@@ -2756,34 +2764,50 @@ ProcessInterrupts(void)
            switch (cancelMode)
            {
                case CONFLICT_MODE_FATAL:
-                       Assert(RecoveryInProgress());
-                       ereport(FATAL,
+                   ImmediateInterruptOK = false;   /* not idle anymore */
+                   DisableNotifyInterrupt();
+                   DisableCatchupInterrupt();
+                   Assert(RecoveryInProgress());
+                   ereport(FATAL,
                            (errcode(ERRCODE_QUERY_CANCELED),
                             errmsg("canceling session due to conflict with recovery")));
 
                case CONFLICT_MODE_ERROR:
-                       /*
-                        * We are aborting because we need to release
-                        * locks. So we need to abort out of all
-                        * subtransactions to make sure we release
-                        * all locks at whatever their level.
-                        *
-                        * XXX Should we try to examine the
-                        * transaction tree and cancel just enough
-                        * subxacts to remove locks? Doubt it.
-                        */
-                       Assert(RecoveryInProgress());
-                       AbortOutOfAnyTransaction();
-                       ereport(ERROR,
+                   /*
+                    * We are aborting because we need to release
+                    * locks. So we need to abort out of all
+                    * subtransactions to make sure we release
+                    * all locks at whatever their level.
+                    *
+                    * XXX Should we try to examine the
+                    * transaction tree and cancel just enough
+                    * subxacts to remove locks? Doubt it.
+                    */
+                   ImmediateInterruptOK = false;   /* not idle anymore */
+                   DisableNotifyInterrupt();
+                   DisableCatchupInterrupt();
+                   Assert(RecoveryInProgress());
+                   AbortOutOfAnyTransaction();
+                   ereport(ERROR,
                            (errcode(ERRCODE_QUERY_CANCELED),
                             errmsg("canceling statement due to conflict with recovery")));
-                       return;
 
                default:
-                       /* No conflict pending, so fall through */
-                       break;
+                   /* No conflict pending, so fall through */
+                   break;
            }
+       }
 
+       /*
+        * If we are reading a command from the client, just ignore the
+        * cancel request --- sending an extra error message won't
+        * accomplish anything.  Otherwise, go ahead and throw the error.
+        */
+       if (!DoingCommandRead)
+       {
+           ImmediateInterruptOK = false;   /* not idle anymore */
+           DisableNotifyInterrupt();
+           DisableCatchupInterrupt();
            ereport(ERROR,
                    (errcode(ERRCODE_QUERY_CANCELED),
                     errmsg("canceling statement due to user request")));
@@ -3626,7 +3650,6 @@ PostgresMain(int argc, char *argv[], const char *username)
         * conditional since we don't want, say, reads on behalf of COPY FROM
         * STDIN doing the same thing.)
         */
-       QueryCancelPending = false;     /* forget any earlier CANCEL signal */
        DoingCommandRead = true;
 
        /*