Prevent interrupts while reporting non-ERROR elog messages.

tglsfdc · tglsfdc · commit 6c461cb92f29 · 2014-03-13T20:59:42.000-04:00
This should eliminate the risk of recursive entry to syslog(3), which
appears to be the cause of the hang reported in bug #9551 from James
Morton.

Arguably, the real problem here is auth.c's willingness to turn on
ImmediateInterruptOK while executing fairly wide swaths of backend code.
We may well need to work at narrowing the code ranges in which the
authentication_timeout interrupt is enabled.  For the moment, though,
this is a cheap and reasonably noninvasive fix for a field-reported
failure; the other approach would be complex and not necessarily
bug-free itself.

Back-patch to all supported branches.
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
@@ -410,13 +410,26 @@ errfinish(int dummy,...)
 {
 	ErrorData  *edata = &errordata[errordata_stack_depth];
 	int			elevel;
+	bool		save_ImmediateInterruptOK;
 	MemoryContext oldcontext;
 	ErrorContextCallback *econtext;
 
 	recursion_depth++;
 	CHECK_STACK_DEPTH();
 	elevel = edata->elevel;
 
+	/*
+	 * Ensure we can't get interrupted while performing error reporting.  This
+	 * is needed to prevent recursive entry to functions like syslog(), which
+	 * may not be re-entrant.
+	 *
+	 * Note: other places that save-and-clear ImmediateInterruptOK also do
+	 * HOLD_INTERRUPTS(), but that should not be necessary here since we
+	 * don't call anything that could turn on ImmediateInterruptOK.
+	 */
+	save_ImmediateInterruptOK = ImmediateInterruptOK;
+	ImmediateInterruptOK = false;
+
 	/*
 	 * Do processing in ErrorContext, which we hope has enough reserved space
 	 * to report an error.
@@ -442,17 +455,16 @@ errfinish(int dummy,...)
 		/*
 		 * We do some minimal cleanup before longjmp'ing so that handlers can
 		 * execute in a reasonably sane state.
-		 */
-
-		/* This is just in case the error came while waiting for input */
-		ImmediateInterruptOK = false;
-
-		/*
+		 *
 		 * Reset InterruptHoldoffCount in case we ereport'd from inside an
 		 * interrupt holdoff section.  (We assume here that no handler will
 		 * itself be inside a holdoff section.	If necessary, such a handler
 		 * could save and restore InterruptHoldoffCount for itself, but this
 		 * should make life easier for most.)
+		 *
+		 * Note that we intentionally don't restore ImmediateInterruptOK here,
+		 * even if it was set at entry.  We definitely don't want that on
+		 * while doing error cleanup.
 		 */
 		InterruptHoldoffCount = 0;
 
@@ -519,10 +531,7 @@ errfinish(int dummy,...)
 	{
 		/*
 		 * For a FATAL error, we let proc_exit clean up and exit.
-		 */
-		ImmediateInterruptOK = false;
-
-		/*
+		 *
 		 * If we just reported a startup failure, the client will disconnect
 		 * on receiving it, so don't send any more to the client.
 		 */
@@ -555,15 +564,18 @@ errfinish(int dummy,...)
 		 * XXX: what if we are *in* the postmaster?  abort() won't kill our
 		 * children...
 		 */
-		ImmediateInterruptOK = false;
 		fflush(stdout);
 		fflush(stderr);
 		abort();
 	}
 
 	/*
-	 * We reach here if elevel <= WARNING. OK to return to caller.
-	 *
+	 * We reach here if elevel <= WARNING.  OK to return to caller, so restore
+	 * caller's setting of ImmediateInterruptOK.
+	 */
+	ImmediateInterruptOK = save_ImmediateInterruptOK;
+
+	/*
 	 * But check for cancel/die interrupt first --- this is so that the user
 	 * can stop a query emitting tons of notice or warning messages, even if
 	 * it's in a loop that otherwise fails to check for interrupts.
