Don't let timeout interrupts happen unless ImmediateInterruptOK is set.

Serious oversight in commit 16e1b7a1b7f7ffd8a18713e83c8cd72c9ce48e07:
we should not allow an interrupt to take control away from mainline code
except when ImmediateInterruptOK is set.  Just to be safe, let's adopt
the same save-clear-restore dance that's been used for many years in
HandleCatchupInterrupt and HandleNotifyInterrupt, so that nothing bad
happens if a timeout handler invokes code that tests or even manipulates
ImmediateInterruptOK.

Per report of "stuck spinlock" failures from Christophe Pettus, though
many other symptoms are possible.  Diagnosis by Andres Freund.

diff --git a/src/backend/utils/misc/timeout.c b/src/backend/utils/misc/timeout.c
index aae947e601b483b4ab0281cc884c6fe60a6f5380..059253904380d40a71594a35b6811ee04ecdc61c 100644 (file)
--- a/src/backend/utils/misc/timeout.c
+++ b/src/backend/utils/misc/timeout.c
@@ -259,22 +259,27 @@ static void
 handle_sig_alarm(SIGNAL_ARGS)
 {
        int                     save_errno = errno;
+       bool            save_ImmediateInterruptOK = ImmediateInterruptOK;
 
        /*
         * We may be executing while ImmediateInterruptOK is true (e.g., when
         * mainline is waiting for a lock).  If SIGINT or similar arrives while
         * this code is running, we'd lose control and perhaps leave our data
-        * structures in an inconsistent state.  Hold off interrupts to prevent
-        * that.
+        * structures in an inconsistent state.  Disable immediate interrupts, and
+        * just to be real sure, bump the holdoff counter as well.      (The reason
+        * for this belt-and-suspenders-too approach is to make sure that nothing
+        * bad happens if a timeout handler calls code that manipulates
+        * ImmediateInterruptOK.)
         *
-        * Note: it's possible for a SIGINT to interrupt handle_sig_alarm even
-        * before we reach HOLD_INTERRUPTS(); the net effect would be as if the
-        * SIGALRM event had been silently lost.  Therefore error recovery must
-        * include some action that will allow any lost interrupt to be
-        * rescheduled.  Disabling some or all timeouts is sufficient, or if
-        * that's not appropriate, reschedule_timeouts() can be called.  Also, the
-        * signal blocking hazard described below applies here too.
+        * Note: it's possible for a SIGINT to interrupt handle_sig_alarm before
+        * we manage to do this; the net effect would be as if the SIGALRM event
+        * had been silently lost.      Therefore error recovery must include some
+        * action that will allow any lost interrupt to be rescheduled.  Disabling
+        * some or all timeouts is sufficient, or if that's not appropriate,
+        * reschedule_timeouts() can be called.  Also, the signal blocking hazard
+        * described below applies here too.
         */
+       ImmediateInterruptOK = false;
        HOLD_INTERRUPTS();
 
        /*
@@ -330,9 +335,12 @@ handle_sig_alarm(SIGNAL_ARGS)
        }
 
        /*
-        * Re-allow query cancel, and then service any cancel request that arrived
-        * meanwhile (this might in particular include a cancel request fired by
-        * one of the timeout handlers).
+        * Re-allow query cancel, and then try to service any cancel request that
+        * arrived meanwhile (this might in particular include a cancel request
+        * fired by one of the timeout handlers).  Since we are in a signal
+        * handler, we mustn't call ProcessInterrupts unless ImmediateInterruptOK
+        * is set; if it isn't, the cancel will happen at the next mainline
+        * CHECK_FOR_INTERRUPTS.
         *
         * Note: a longjmp from here is safe so far as our own data structures are
         * concerned; but on platforms that block a signal before calling the
@@ -341,7 +349,9 @@ handle_sig_alarm(SIGNAL_ARGS)
         * unblocking any blocked signals.
         */
        RESUME_INTERRUPTS();
-       CHECK_FOR_INTERRUPTS();
+       ImmediateInterruptOK = save_ImmediateInterruptOK;
+       if (save_ImmediateInterruptOK)
+               CHECK_FOR_INTERRUPTS();
 
        errno = save_errno;
 }