Make critical sections (elog->crash) and interrupt holdoff sections

into distinct concepts, per recent discussion on pghackers.

src/backend/access/transam/xact.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/access/transam/xact.c,v 1.94 2001/01/18 18:33:45 vadim Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/access/transam/xact.c,v 1.95 2001/01/19 22:08:46 tgl Exp $
  *
  * NOTES
  *		Transaction aborts can now occur two ways:
@@ -1016,7 +1016,7 @@ CommitTransaction(void)
 		elog(NOTICE, "CommitTransaction and not in in-progress state ");
 
 	/* Prevent cancel/die interrupt while cleaning up */
-	START_CRIT_SECTION();
+	HOLD_INTERRUPTS();
 
 	/* ----------------
 	 *	Tell the trigger manager that this transaction is about to be
@@ -1087,7 +1087,7 @@ CommitTransaction(void)
 	 */
 	s->state = TRANS_DEFAULT;
 
-	END_CRIT_SECTION();
+	RESUME_INTERRUPTS();
 }
 
 /* --------------------------------
@@ -1101,7 +1101,7 @@ AbortTransaction(void)
 	TransactionState s = CurrentTransactionState;
 
 	/* Prevent cancel/die interrupt while cleaning up */
-	START_CRIT_SECTION();
+	HOLD_INTERRUPTS();
 
 	/*
 	 * Let others to know about no transaction in progress - vadim
@@ -1133,7 +1133,7 @@ AbortTransaction(void)
 	 */
 	if (s->state == TRANS_DISABLED)
 	{
-		END_CRIT_SECTION();
+		RESUME_INTERRUPTS();
 		return;
 	}
 
@@ -1185,7 +1185,7 @@ AbortTransaction(void)
 	 *	State remains TRANS_ABORT until CleanupTransaction().
 	 * ----------------
 	 */
-	END_CRIT_SECTION();
+	RESUME_INTERRUPTS();
 }
 
 /* --------------------------------

src/backend/commands/vacuum.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/commands/vacuum.c,v 1.183 2001/01/14 05:08:15 tgl Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/commands/vacuum.c,v 1.184 2001/01/19 22:08:46 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1425,9 +1425,6 @@ repair_frag(VRelStats *vacrelstats, Relation onerel,
 					ToPage = BufferGetPage(cur_buffer);
 					Cpage = BufferGetPage(Cbuf);
 
-					/* NO ELOG(ERROR) TILL CHANGES ARE LOGGED */
-					START_CRIT_SECTION();
-
 					Citemid = PageGetItemId(Cpage,
 							ItemPointerGetOffsetNumber(&(tuple.t_self)));
 					tuple.t_datamcxt = NULL;
@@ -1442,6 +1439,9 @@ repair_frag(VRelStats *vacrelstats, Relation onerel,
 
 					RelationInvalidateHeapTuple(onerel, &tuple);
 
+					/* NO ELOG(ERROR) TILL CHANGES ARE LOGGED */
+					START_CRIT_SECTION();
+
 					TransactionIdStore(myXID, (TransactionId *) &(tuple.t_data->t_cmin));
 					tuple.t_data->t_infomask &=
 						~(HEAP_XMIN_COMMITTED | HEAP_XMIN_INVALID | HEAP_MOVED_IN);
@@ -1626,6 +1626,9 @@ repair_frag(VRelStats *vacrelstats, Relation onerel,
 
 			RelationInvalidateHeapTuple(onerel, &tuple);
 
+			/* NO ELOG(ERROR) TILL CHANGES ARE LOGGED */
+			START_CRIT_SECTION();
+
 			/*
 			 * Mark new tuple as moved_in by vacuum and store vacuum XID
 			 * in t_cmin !!!
@@ -1635,9 +1638,6 @@ repair_frag(VRelStats *vacrelstats, Relation onerel,
 				~(HEAP_XMIN_COMMITTED | HEAP_XMIN_INVALID | HEAP_MOVED_OFF);
 			newtup.t_data->t_infomask |= HEAP_MOVED_IN;
 
-			/* NO ELOG(ERROR) TILL CHANGES ARE LOGGED */
-			START_CRIT_SECTION();
-
 			/* add tuple to the page */
 			newoff = PageAddItem(ToPage, (Item) newtup.t_data, tuple_len,
 								 InvalidOffsetNumber, LP_USED);
@@ -2070,7 +2070,6 @@ vacuum_page(Relation onerel, Buffer buffer, VacPage vacpage)
 		PageSetSUI(page, ThisStartUpID);
 	}
 	END_CRIT_SECTION();
-
 }
 
 /*

src/backend/storage/buffer/bufmgr.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/storage/buffer/bufmgr.c,v 1.104 2001/01/14 05:08:15 tgl Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/storage/buffer/bufmgr.c,v 1.105 2001/01/19 22:08:46 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -873,10 +873,10 @@ WaitIO(BufferDesc *buf, SPINLOCK spinlock)
 	while ((buf->flags & BM_IO_IN_PROGRESS) != 0)
 	{
 		SpinRelease(spinlock);
-		START_CRIT_SECTION();	/* don't want to die() holding the lock... */
+		HOLD_INTERRUPTS();		/* don't want to die() holding the lock... */
 		S_LOCK(&(buf->io_in_progress_lock));
 		S_UNLOCK(&(buf->io_in_progress_lock));
-		END_CRIT_SECTION();
+		RESUME_INTERRUPTS();
 		SpinAcquire(spinlock);
 	}
 }
@@ -1027,14 +1027,14 @@ BufmgrCommit(void)
  *		Returns the block number associated with a buffer.
  *
  * Note:
- *		Assumes that the buffer is valid.
+ *		Assumes that the buffer is valid and pinned, else the
+ *		value may be obsolete immediately...
  */
 BlockNumber
 BufferGetBlockNumber(Buffer buffer)
 {
 	Assert(BufferIsValid(buffer));
 
-	/* XXX should be a critical section */
 	if (BufferIsLocal(buffer))
 		return LocalBufferDescriptors[-buffer - 1].tag.blockNum;
 	else
@@ -1956,7 +1956,7 @@ UnlockBuffers(void)
 		Assert(BufferIsValid(i + 1));
 		buf = &(BufferDescriptors[i]);
 
-		START_CRIT_SECTION();	/* don't want to die() holding the lock... */
+		HOLD_INTERRUPTS();		/* don't want to die() holding the lock... */
 
 		S_LOCK(&(buf->cntx_lock));
 
@@ -1986,7 +1986,7 @@ UnlockBuffers(void)
 
 		BufferLocks[i] = 0;
 
-		END_CRIT_SECTION();
+		RESUME_INTERRUPTS();
 	}
 }
 
@@ -2003,7 +2003,7 @@ LockBuffer(Buffer buffer, int mode)
 	buf = &(BufferDescriptors[buffer - 1]);
 	buflock = &(BufferLocks[buffer - 1]);
 
-	START_CRIT_SECTION();		/* don't want to die() holding the lock... */
+	HOLD_INTERRUPTS();			/* don't want to die() holding the lock... */
 
 	S_LOCK(&(buf->cntx_lock));
 
@@ -2028,7 +2028,7 @@ LockBuffer(Buffer buffer, int mode)
 		else
 		{
 			S_UNLOCK(&(buf->cntx_lock));
-			END_CRIT_SECTION();
+			RESUME_INTERRUPTS();
 			elog(ERROR, "UNLockBuffer: buffer %lu is not locked", buffer);
 		}
 	}
@@ -2040,9 +2040,9 @@ LockBuffer(Buffer buffer, int mode)
 		while (buf->ri_lock || buf->w_lock)
 		{
 			S_UNLOCK(&(buf->cntx_lock));
-			END_CRIT_SECTION();
+			RESUME_INTERRUPTS();
 			S_LOCK_SLEEP(&(buf->cntx_lock), i++);
-			START_CRIT_SECTION();
+			HOLD_INTERRUPTS();
 			S_LOCK(&(buf->cntx_lock));
 		}
 		(buf->r_locks)++;
@@ -2068,9 +2068,9 @@ LockBuffer(Buffer buffer, int mode)
 				buf->ri_lock = true;
 			}
 			S_UNLOCK(&(buf->cntx_lock));
-			END_CRIT_SECTION();
+			RESUME_INTERRUPTS();
 			S_LOCK_SLEEP(&(buf->cntx_lock), i++);
-			START_CRIT_SECTION();
+			HOLD_INTERRUPTS();
 			S_LOCK(&(buf->cntx_lock));
 		}
 		buf->w_lock = true;
@@ -2092,12 +2092,12 @@ LockBuffer(Buffer buffer, int mode)
 	else
 	{
 		S_UNLOCK(&(buf->cntx_lock));
-		END_CRIT_SECTION();
+		RESUME_INTERRUPTS();
 		elog(ERROR, "LockBuffer: unknown lock mode %d", mode);
 	}
 
 	S_UNLOCK(&(buf->cntx_lock));
-	END_CRIT_SECTION();
+	RESUME_INTERRUPTS();
 }
 
 /*
@@ -2118,7 +2118,7 @@ static bool IsForInput;
  *	BM_IO_IN_PROGRESS mask is not set for the buffer
  *	The buffer is Pinned
  *
- * Because BufMgrLock is held, we are already in a CRIT_SECTION here,
+ * Because BufMgrLock is held, we are already in an interrupt holdoff here,
  * and do not need another.
  */
 static void
@@ -2152,7 +2152,7 @@ StartBufferIO(BufferDesc *buf, bool forInput)
  *	BufMgrLock is held
  *	The buffer is Pinned
  *
- * Because BufMgrLock is held, we are already in a CRIT_SECTION here,
+ * Because BufMgrLock is held, we are already in an interrupt holdoff here,
  * and do not need another.
  */
 static void
@@ -2170,7 +2170,7 @@ TerminateBufferIO(BufferDesc *buf)
  *	BufMgrLock is held
  *	The buffer is Pinned
  *
- * Because BufMgrLock is held, we are already in a CRIT_SECTION here,
+ * Because BufMgrLock is held, we are already in an interrupt holdoff here,
  * and do not need another.
  */
 static void

src/backend/storage/ipc/ipc.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/storage/ipc/ipc.c,v 1.60 2001/01/14 05:08:15 tgl Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/storage/ipc/ipc.c,v 1.61 2001/01/19 22:08:46 tgl Exp $
  *
  * NOTES
  *
@@ -136,7 +136,8 @@ proc_exit(int code)
 	QueryCancelPending = false;
 	/* And let's just make *sure* we're not interrupted ... */
 	ImmediateInterruptOK = false;
-	CritSectionCount = 1;
+	InterruptHoldoffCount = 1;
+	CritSectionCount = 0;
 
 	if (DebugLvl > 1)
 		elog(DEBUG, "proc_exit(%d)", code);

src/backend/storage/ipc/spin.c

@@ -14,7 +14,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/storage/ipc/Attic/spin.c,v 1.29 2001/01/14 05:08:15 tgl Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/storage/ipc/Attic/spin.c,v 1.30 2001/01/19 22:08:47 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -148,19 +148,19 @@ SpinAcquire(SPINLOCK lockid)
 	PRINT_SLDEBUG("SpinAcquire", lockid, slckP);
 	/*
 	 * Acquire the lock, then record that we have done so (for recovery
-	 * in case of elog(ERROR) during the critical section).  Note we assume
+	 * in case of elog(ERROR) while holding the lock).  Note we assume
 	 * here that S_LOCK will not accept cancel/die interrupts once it has
 	 * acquired the lock.  However, interrupts should be accepted while
-	 * waiting, if CritSectionCount is zero.
+	 * waiting, if InterruptHoldoffCount is zero.
 	 */
 	S_LOCK(&(slckP->shlock));
 	PROC_INCR_SLOCK(lockid);
 	/*
-	 * Lock out cancel/die interrupts until we exit the critical section
+	 * Lock out cancel/die interrupts until we exit the code section
 	 * protected by the spinlock.  This ensures that interrupts will not
 	 * interfere with manipulations of data structures in shared memory.
 	 */
-	START_CRIT_SECTION();
+	HOLD_INTERRUPTS();
 
     PRINT_SLDEBUG("SpinAcquire/done", lockid, slckP);
 }
@@ -182,9 +182,9 @@ SpinRelease(SPINLOCK lockid)
 	PROC_DECR_SLOCK(lockid);
 	S_UNLOCK(&(slckP->shlock));
 	/*
-	 * Exit the critical section entered in SpinAcquire().
+	 * Exit the interrupt holdoff entered in SpinAcquire().
 	 */
-	END_CRIT_SECTION();
+	RESUME_INTERRUPTS();
 
     PRINT_SLDEBUG("SpinRelease/done", lockid, slckP);
 }
@@ -329,7 +329,7 @@ SpinAcquire(SPINLOCK lock)
 	 */
 	IpcSemaphoreLock(SpinLockIds[0], lock, false);
 	PROC_INCR_SLOCK(lock);
-	START_CRIT_SECTION();
+	HOLD_INTERRUPTS();
 }
 
 /*
@@ -351,7 +351,7 @@ SpinRelease(SPINLOCK lock)
     Assert(!MyProc || MyProc->sLocks[lockid] > 0);
 	PROC_DECR_SLOCK(lock);
 	IpcSemaphoreUnlock(SpinLockIds[0], lock);
-	END_CRIT_SECTION();
+	RESUME_INTERRUPTS();
 }
 
 /*

src/backend/tcop/postgres.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/tcop/postgres.c,v 1.202 2001/01/16 20:59:34 tgl Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/tcop/postgres.c,v 1.203 2001/01/19 22:08:47 tgl Exp $
  *
  * NOTES
  *	  this is the "main" module of the postgres backend and
@@ -943,7 +943,8 @@ die(SIGNAL_ARGS)
 		 * If it's safe to interrupt, and we're waiting for input or a lock,
 		 * service the interrupt immediately
 		 */
-		if (ImmediateInterruptOK && CritSectionCount == 0)
+		if (ImmediateInterruptOK && InterruptHoldoffCount == 0 &&
+			CritSectionCount == 0)
 		{
 			DisableNotifyInterrupt();
 			/* Make sure HandleDeadLock won't run while shutting down... */
@@ -974,8 +975,8 @@ QueryCancelHandler(SIGNAL_ARGS)
 		 * service the interrupt immediately.  No point in interrupting
 		 * if we're waiting for input, however.
 		 */
-		if (ImmediateInterruptOK && CritSectionCount == 0 &&
-			LockWaitCancel())
+		if (ImmediateInterruptOK && InterruptHoldoffCount == 0 &&
+			CritSectionCount == 0 && LockWaitCancel())
 		{
 			DisableNotifyInterrupt();
 			ProcessInterrupts();
@@ -1012,8 +1013,8 @@ SigHupHandler(SIGNAL_ARGS)
 void
 ProcessInterrupts(void)
 {
-	/* Cannot accept interrupts inside critical sections */
-	if (CritSectionCount != 0)
+	/* OK to accept interrupt now? */
+	if (InterruptHoldoffCount != 0 || CritSectionCount != 0)
 		return;
 	InterruptPending = false;
 	if (ProcDiePending)
@@ -1679,7 +1680,7 @@ PostgresMain(int argc, char *argv[], int real_argc, char *real_argv[], const cha
 	if (!IsUnderPostmaster)
 	{
 		puts("\nPOSTGRES backend interactive interface ");
-		puts("$Revision: 1.202 $ $Date: 2001/01/16 20:59:34 $\n");
+		puts("$Revision: 1.203 $ $Date: 2001/01/19 22:08:47 $\n");
 	}
 
 	/*
@@ -1712,12 +1713,13 @@ PostgresMain(int argc, char *argv[], int real_argc, char *real_argv[], const cha
 		 *
 		 * Make sure we're not interrupted while cleaning up.  Also forget
 		 * any pending QueryCancel request, since we're aborting anyway.
-		 * Force CritSectionCount to a known state in case we elog'd
-		 * from inside a critical section.
+		 * Force InterruptHoldoffCount to a known state in case we elog'd
+		 * from inside a holdoff section.
 		 */
 		ImmediateInterruptOK = false;
 		QueryCancelPending = false;
-		CritSectionCount = 1;
+		InterruptHoldoffCount = 1;
+		CritSectionCount = 0;	/* should be unnecessary, but... */
 
 		/*
 		 * Make sure we are in a valid memory context during recovery.
@@ -1746,10 +1748,10 @@ PostgresMain(int argc, char *argv[], int real_argc, char *real_argv[], const cha
 		InError = false;
 
 		/*
-		 * Exit critical section we implicitly established above.
+		 * Exit interrupt holdoff section we implicitly established above.
 		 * (This could result in accepting a cancel or die interrupt.)
 		 */
-		END_CRIT_SECTION();
+		RESUME_INTERRUPTS();
 	}
 
 	Warn_restart_ready = true;	/* we can now handle elog(ERROR) */

src/backend/utils/error/elog.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/utils/error/elog.c,v 1.76 2001/01/14 05:08:16 tgl Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/utils/error/elog.c,v 1.77 2001/01/19 22:08:47 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -132,7 +132,7 @@ elog(int lev, const char *fmt, ...)
 	int			space_needed;
 	int			len;
 	/* size of the prefix needed for timestamp and pid, if enabled */
-	size_t      timestamp_size;
+	size_t		timestamp_size;
 
 	if (lev <= DEBUG && Debugfile < 0)
 		return;					/* ignore debug msgs if noplace to send */
@@ -148,15 +148,25 @@ elog(int lev, const char *fmt, ...)
 	}
 #else
 	/* assume strerror() will cope gracefully with bogus errno values */
-    errorstr = strerror(errno);
+	errorstr = strerror(errno);
 #endif
 
-	/* Convert initialization errors into fatal errors.
-	 * This is probably redundant, because Warn_restart_ready won't
-	 * be set anyway...
-	 */
-	if (lev == ERROR && IsInitProcessingMode())
-		lev = FATAL;
+	if (lev == ERROR || lev == FATAL)
+	{
+		/*
+		 * Convert initialization errors into fatal errors.
+		 * This is probably redundant, because Warn_restart_ready won't
+		 * be set anyway...
+		 */
+		if (IsInitProcessingMode())
+			lev = FATAL;
+		/*
+		 * If we are inside a critical section, all errors become STOP errors.
+		 * See miscadmin.h.
+		 */
+		if (CritSectionCount > 0)
+			lev = STOP;
+	}
 
 	/* choose message prefix and indent level */
 	switch (lev)

src/backend/utils/init/globals.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/utils/init/globals.c,v 1.50 2001/01/14 05:08:16 tgl Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/utils/init/globals.c,v 1.51 2001/01/19 22:08:47 tgl Exp $
  *
  * NOTES
  *	  Globals used all over the place should be declared here and not
@@ -39,6 +39,7 @@ volatile bool InterruptPending = false;
 volatile bool QueryCancelPending = false;
 volatile bool ProcDiePending = false;
 volatile bool ImmediateInterruptOK = false;
+volatile uint32 InterruptHoldoffCount = 0;
 volatile uint32 CritSectionCount = 0;
 
 int			MyProcPid;

src/include/miscadmin.h

@@ -12,7 +12,7 @@
  * Portions Copyright (c) 1996-2000, PostgreSQL, Inc
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $Id: miscadmin.h,v 1.77 2001/01/14 05:08:16 tgl Exp $
+ * $Id: miscadmin.h,v 1.78 2001/01/19 22:08:47 tgl Exp $
  *
  * NOTES
  *	  some of the information in this file should be moved to
@@ -26,7 +26,7 @@
 #include "storage/ipc.h"
 
 /*****************************************************************************
- *    System interrupt handling
+ *    System interrupt and critical section handling
  *
  * There are two types of interrupts that a running backend needs to accept
  * without messing up its state: QueryCancel (SIGINT) and ProcDie (SIGTERM).
@@ -40,15 +40,23 @@
  * where it is normally safe to accept a cancel or die interrupt.  In some
  * cases, we invoke CHECK_FOR_INTERRUPTS() inside low-level subroutines that
  * might sometimes be called in contexts that do *not* want to allow a cancel
- * or die interrupt.  The CRIT_SECTION mechanism allows code to ensure that
- * no cancel or die interrupt will be accepted, even if CHECK_FOR_INTERRUPTS
- * gets called in a subroutine.
+ * or die interrupt.  The HOLD_INTERRUPTS() and RESUME_INTERRUPTS() macros
+ * allow code to ensure that no cancel or die interrupt will be accepted,
+ * even if CHECK_FOR_INTERRUPTS() gets called in a subroutine.  The interrupt
+ * will be held off until the last matching RESUME_INTERRUPTS() occurs.
  *
  * Special mechanisms are used to let an interrupt be accepted when we are
  * waiting for a lock or spinlock, and when we are waiting for command input
- * (but, of course, only if the critical section counter is zero).  See the
+ * (but, of course, only if the interrupt holdoff counter is zero).  See the
  * related code for details.
  *
+ * A related, but conceptually distinct, mechanism is the "critical section"
+ * mechanism.  A critical section not only holds off cancel/die interrupts,
+ * but causes any elog(ERROR) or elog(FATAL) to become elog(STOP) --- that is,
+ * a system-wide reset is forced.  Needless to say, only really *critical*
+ * code should be marked as a critical section!  Currently, this mechanism
+ * is only used for XLOG-related code.
+ *
  *****************************************************************************/
 
 /* in globals.c */
@@ -58,6 +66,7 @@ extern volatile bool QueryCancelPending;
 extern volatile bool ProcDiePending;
 /* these are marked volatile because they are examined by signal handlers: */
 extern volatile bool ImmediateInterruptOK;
+extern volatile uint32 InterruptHoldoffCount;
 extern volatile uint32 CritSectionCount;
 
 /* in postgres.c */
@@ -69,13 +78,23 @@ extern void ProcessInterrupts(void);
 			ProcessInterrupts(); \
 	} while(0)
 
+#define	HOLD_INTERRUPTS()  (InterruptHoldoffCount++)
+
+#define RESUME_INTERRUPTS() \
+	do { \
+		Assert(InterruptHoldoffCount > 0); \
+		InterruptHoldoffCount--; \
+		if (InterruptPending) \
+			ProcessInterrupts(); \
+	} while(0)
+
 #define	START_CRIT_SECTION()  (CritSectionCount++)
 
 #define END_CRIT_SECTION() \
 	do { \
 		Assert(CritSectionCount > 0); \
 		CritSectionCount--; \
-		if (CritSectionCount == 0 && InterruptPending) \
+		if (InterruptPending) \
 			ProcessInterrupts(); \
 	} while(0)