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;
 
                /*