Fix a number of places where brittle data structures or overly strong

Asserts would lead to a server core dump if an error occurred while
trying to abort a failed subtransaction (thereby leading to re-execution
of whatever parts of AbortSubTransaction had already run).  This of course
does not prevent such an error from creating an infinite loop, but at
least we don't make the situation worse.  Responds to an open item on
the subtransactions to-do list.

src/backend/commands/async.c

@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/commands/async.c,v 1.115 2004/08/29 05:06:41 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/commands/async.c,v 1.116 2004/09/06 23:32:54 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -630,6 +630,9 @@ AtSubStart_Notify(void)
 
 	upperPendingNotifies = lcons(pendingNotifies, upperPendingNotifies);
 
+	Assert(list_length(upperPendingNotifies) ==
+		   GetCurrentTransactionNestLevel() - 1);
+
 	pendingNotifies = NIL;
 
 	MemoryContextSwitchTo(old_cxt);
@@ -648,6 +651,9 @@ AtSubCommit_Notify(void)
 	parentPendingNotifies = (List *) linitial(upperPendingNotifies);
 	upperPendingNotifies = list_delete_first(upperPendingNotifies);
 
+	Assert(list_length(upperPendingNotifies) ==
+		   GetCurrentTransactionNestLevel() - 2);
+
 	/*
 	 * We could try to eliminate duplicates here, but it seems not
 	 * worthwhile.
@@ -661,13 +667,23 @@ AtSubCommit_Notify(void)
 void
 AtSubAbort_Notify(void)
 {
+	int			my_level = GetCurrentTransactionNestLevel();
+
 	/*
 	 * All we have to do is pop the stack --- the notifies made in this
 	 * subxact are no longer interesting, and the space will be freed when
 	 * CurTransactionContext is recycled.
+	 *
+	 * This routine could be called more than once at a given nesting level
+	 * if there is trouble during subxact abort.  Avoid dumping core by
+	 * using GetCurrentTransactionNestLevel as the indicator of how far
+	 * we need to prune the list.
 	 */
-	pendingNotifies = (List *) linitial(upperPendingNotifies);
-	upperPendingNotifies = list_delete_first(upperPendingNotifies);
+	while (list_length(upperPendingNotifies) > my_level - 2)
+	{
+		pendingNotifies = (List *) linitial(upperPendingNotifies);
+		upperPendingNotifies = list_delete_first(upperPendingNotifies);
+	}
 }
 
 /*

src/backend/commands/trigger.c

@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/commands/trigger.c,v 1.168 2004/08/29 05:06:41 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/commands/trigger.c,v 1.169 2004/09/06 23:32:54 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1669,8 +1669,11 @@ ltrmark:;
  * state data; each subtransaction level that modifies that state first
  * saves a copy, which we use to restore the state if we abort.
  *
- * numpushed and numalloc keep control of allocation and storage in the above
- * stacks.	numpushed is essentially the current subtransaction nesting depth.
+ * We use GetCurrentTransactionNestLevel() to determine the correct array
+ * index in these stacks.  numalloc is the number of allocated entries in
+ * each stack.  (By not keeping our own stack pointer, we can avoid trouble
+ * in cases where errors during subxact abort cause multiple invocations
+ * of DeferredTriggerEndSubXact() at the same nesting depth.)
  *
  * XXX We need to be able to save the per-event data in a file if it grows too
  * large.
@@ -1742,7 +1745,6 @@ typedef struct DeferredTriggersData
 	DeferredTriggerEvent *tail_stack;
 	DeferredTriggerEvent *imm_stack;
 	DeferredTriggerState *state_stack;
-	int			numpushed;
 	int			numalloc;
 } DeferredTriggersData;
 
@@ -2165,7 +2167,6 @@ DeferredTriggerBeginXact(void)
 	deferredTriggers->imm_stack = NULL;
 	deferredTriggers->state_stack = NULL;
 	deferredTriggers->numalloc = 0;
-	deferredTriggers->numpushed = 0;
 }
 
 
@@ -2251,6 +2252,8 @@ DeferredTriggerAbortXact(void)
 void
 DeferredTriggerBeginSubXact(void)
 {
+	int			my_level = GetCurrentTransactionNestLevel();
+
 	/*
 	 * Ignore call if the transaction is in aborted state.
 	 */
@@ -2260,7 +2263,7 @@ DeferredTriggerBeginSubXact(void)
 	/*
 	 * Allocate more space in the stacks if needed.
 	 */
-	if (deferredTriggers->numpushed == deferredTriggers->numalloc)
+	while (my_level >= deferredTriggers->numalloc)
 	{
 		if (deferredTriggers->numalloc == 0)
 		{
@@ -2282,32 +2285,28 @@ DeferredTriggerBeginSubXact(void)
 		else
 		{
 			/* repalloc will keep the stacks in the same context */
-			deferredTriggers->numalloc *= 2;
+			int		new_alloc = deferredTriggers->numalloc * 2;
 
 			deferredTriggers->tail_stack = (DeferredTriggerEvent *)
 				repalloc(deferredTriggers->tail_stack,
-			  deferredTriggers->numalloc * sizeof(DeferredTriggerEvent));
+						 new_alloc * sizeof(DeferredTriggerEvent));
 			deferredTriggers->imm_stack = (DeferredTriggerEvent *)
 				repalloc(deferredTriggers->imm_stack,
-			  deferredTriggers->numalloc * sizeof(DeferredTriggerEvent));
+						 new_alloc * sizeof(DeferredTriggerEvent));
 			deferredTriggers->state_stack = (DeferredTriggerState *)
 				repalloc(deferredTriggers->state_stack,
-			  deferredTriggers->numalloc * sizeof(DeferredTriggerState));
+						 new_alloc * sizeof(DeferredTriggerState));
+			deferredTriggers->numalloc = new_alloc;
 		}
 	}
 
 	/*
-	 * Push the current list position into the stack and reset the
-	 * pointer.
+	 * Push the current information into the stack.
 	 */
-	deferredTriggers->tail_stack[deferredTriggers->numpushed] =
-		deferredTriggers->tail_thisxact;
-	deferredTriggers->imm_stack[deferredTriggers->numpushed] =
-		deferredTriggers->events_imm;
+	deferredTriggers->tail_stack[my_level] = deferredTriggers->tail_thisxact;
+	deferredTriggers->imm_stack[my_level] = deferredTriggers->events_imm;
 	/* State is not saved until/unless changed */
-	deferredTriggers->state_stack[deferredTriggers->numpushed] = NULL;
-
-	deferredTriggers->numpushed++;
+	deferredTriggers->state_stack[my_level] = NULL;
 }
 
 /*
@@ -2318,6 +2317,7 @@ DeferredTriggerBeginSubXact(void)
 void
 DeferredTriggerEndSubXact(bool isCommit)
 {
+	int			my_level = GetCurrentTransactionNestLevel();
 	DeferredTriggerState state;
 
 	/*
@@ -2327,18 +2327,18 @@ DeferredTriggerEndSubXact(bool isCommit)
 		return;
 
 	/*
-	 * Move back the "top of the stack."
+	 * Pop the prior state if needed.
 	 */
-	Assert(deferredTriggers->numpushed > 0);
-
-	deferredTriggers->numpushed--;
+	Assert(my_level < deferredTriggers->numalloc);
 
 	if (isCommit)
 	{
 		/* If we saved a prior state, we don't need it anymore */
-		state = deferredTriggers->state_stack[deferredTriggers->numpushed];
+		state = deferredTriggers->state_stack[my_level];
 		if (state != NULL)
 			pfree(state);
+		/* this avoids double pfree if error later: */
+		deferredTriggers->state_stack[my_level] = NULL;
 	}
 	else
 	{
@@ -2346,9 +2346,9 @@ DeferredTriggerEndSubXact(bool isCommit)
 		 * Aborting --- restore the pointers from the stacks.
 		 */
 		deferredTriggers->tail_thisxact =
-			deferredTriggers->tail_stack[deferredTriggers->numpushed];
+			deferredTriggers->tail_stack[my_level];
 		deferredTriggers->events_imm =
-			deferredTriggers->imm_stack[deferredTriggers->numpushed];
+			deferredTriggers->imm_stack[my_level];
 
 		/*
 		 * Cleanup the head and the tail of the list.
@@ -2367,12 +2367,14 @@ DeferredTriggerEndSubXact(bool isCommit)
 		 * Restore the trigger state.  If the saved state is NULL, then
 		 * this subxact didn't save it, so it doesn't need restoring.
 		 */
-		state = deferredTriggers->state_stack[deferredTriggers->numpushed];
+		state = deferredTriggers->state_stack[my_level];
 		if (state != NULL)
 		{
 			pfree(deferredTriggers->state);
 			deferredTriggers->state = state;
 		}
+		/* this avoids double pfree if error later: */
+		deferredTriggers->state_stack[my_level] = NULL;
 	}
 }
 
@@ -2457,6 +2459,8 @@ DeferredTriggerStateAddItem(DeferredTriggerState state,
 void
 DeferredTriggerSetState(ConstraintsSetStmt *stmt)
 {
+	int			my_level = GetCurrentTransactionNestLevel();
+
 	/*
 	 * Ignore call if we aren't in a transaction.
 	 */
@@ -2468,10 +2472,10 @@ DeferredTriggerSetState(ConstraintsSetStmt *stmt)
 	 * already, save it so it can be restored if the subtransaction
 	 * aborts.
 	 */
-	if (deferredTriggers->numpushed > 0 &&
-		deferredTriggers->state_stack[deferredTriggers->numpushed - 1] == NULL)
+	if (my_level > 1 &&
+		deferredTriggers->state_stack[my_level] == NULL)
 	{
-		deferredTriggers->state_stack[deferredTriggers->numpushed - 1] =
+		deferredTriggers->state_stack[my_level] =
 			DeferredTriggerStateCopy(deferredTriggers->state);
 	}
 

src/backend/storage/ipc/sinval.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/storage/ipc/sinval.c,v 1.72 2004/08/29 05:06:48 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/storage/ipc/sinval.c,v 1.73 2004/09/06 23:33:35 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1059,8 +1059,14 @@ XidCacheRemoveRunningXids(TransactionId xid, int nxids, TransactionId *xids)
 				break;
 			}
 		}
-		/* We should have found it, unless the cache has overflowed */
-		Assert(j >= 0 || MyProc->subxids.overflowed);
+		/*
+		 * Ordinarily we should have found it, unless the cache has overflowed.
+		 * However it's also possible for this routine to be invoked multiple
+		 * times for the same subtransaction, in case of an error during
+		 * AbortSubTransaction.  So instead of Assert, emit a debug warning.
+		 */
+		if (j < 0 && !MyProc->subxids.overflowed)
+			elog(WARNING, "did not find subXID %u in MyProc", anxid);
 	}
 
 	for (j = MyProc->subxids.nxids - 1; j >= 0; j--)
@@ -1071,8 +1077,9 @@ XidCacheRemoveRunningXids(TransactionId xid, int nxids, TransactionId *xids)
 			break;
 		}
 	}
-	/* We should have found it, unless the cache has overflowed */
-	Assert(j >= 0 || MyProc->subxids.overflowed);
+	/* Ordinarily we should have found it, unless the cache has overflowed */
+	if (j < 0 && !MyProc->subxids.overflowed)
+		elog(WARNING, "did not find subXID %u in MyProc", xid);
 
 	LWLockRelease(SInvalLock);
 }

src/backend/utils/cache/inval.c

@@ -80,12 +80,13 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/utils/cache/inval.c,v 1.66 2004/08/29 05:06:50 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/utils/cache/inval.c,v 1.67 2004/09/06 23:33:48 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
 #include "postgres.h"
 
+#include "access/xact.h"
 #include "catalog/catalog.h"
 #include "miscadmin.h"
 #include "storage/sinval.h"
@@ -139,6 +140,9 @@ typedef struct TransInvalidationInfo
 	/* Back link to parent transaction's info */
 	struct TransInvalidationInfo *parent;
 
+	/* Subtransaction nesting depth */
+	int		my_level;
+
 	/* head of current-command event list */
 	InvalidationListHeader CurrentCmdInvalidMsgs;
 
@@ -603,6 +607,7 @@ AtStart_Inval(void)
 	transInvalInfo = (TransInvalidationInfo *)
 		MemoryContextAllocZero(TopTransactionContext,
 							   sizeof(TransInvalidationInfo));
+	transInvalInfo->my_level = GetCurrentTransactionNestLevel();
 }
 
 /*
@@ -619,6 +624,7 @@ AtSubStart_Inval(void)
 		MemoryContextAllocZero(TopTransactionContext,
 							   sizeof(TransInvalidationInfo));
 	myInfo->parent = transInvalInfo;
+	myInfo->my_level = GetCurrentTransactionNestLevel();
 	transInvalInfo = myInfo;
 }
 
@@ -649,11 +655,11 @@ AtSubStart_Inval(void)
 void
 AtEOXact_Inval(bool isCommit)
 {
-	/* Must be at top of stack */
-	Assert(transInvalInfo != NULL && transInvalInfo->parent == NULL);
-
 	if (isCommit)
 	{
+		/* Must be at top of stack */
+		Assert(transInvalInfo != NULL && transInvalInfo->parent == NULL);
+
 		/*
 		 * Relcache init file invalidation requires processing both before
 		 * and after we send the SI messages.  However, we need not do
@@ -671,8 +677,11 @@ AtEOXact_Inval(bool isCommit)
 		if (transInvalInfo->RelcacheInitFileInval)
 			RelationCacheInitFileInvalidate(false);
 	}
-	else
+	else if (transInvalInfo != NULL)
 	{
+		/* Must be at top of stack */
+		Assert(transInvalInfo->parent == NULL);
+
 		ProcessInvalidationMessages(&transInvalInfo->PriorCmdInvalidMsgs,
 									LocalExecuteInvalidationMessage);
 	}
@@ -696,18 +705,21 @@ AtEOXact_Inval(bool isCommit)
  *
  * In any case, pop the transaction stack.	We need not physically free memory
  * here, since CurTransactionContext is about to be emptied anyway
- * (if aborting).
+ * (if aborting).  Beware of the possibility of aborting the same nesting
+ * level twice, though.
  */
 void
 AtEOSubXact_Inval(bool isCommit)
 {
+	int			my_level = GetCurrentTransactionNestLevel();
 	TransInvalidationInfo *myInfo = transInvalInfo;
 
-	/* Must be at non-top of stack */
-	Assert(myInfo != NULL && myInfo->parent != NULL);
-
 	if (isCommit)
 	{
+		/* Must be at non-top of stack */
+		Assert(myInfo != NULL && myInfo->parent != NULL);
+		Assert(myInfo->my_level == my_level);
+
 		/* If CurrentCmdInvalidMsgs still has anything, fix it */
 		CommandEndInvalidationMessages();
 
@@ -718,18 +730,27 @@ AtEOSubXact_Inval(bool isCommit)
 		/* Pending relcache inval becomes parent's problem too */
 		if (myInfo->RelcacheInitFileInval)
 			myInfo->parent->RelcacheInitFileInval = true;
+
+		/* Pop the transaction state stack */
+		transInvalInfo = myInfo->parent;
+
+		/* Need not free anything else explicitly */
+		pfree(myInfo);
 	}
-	else
+	else if (myInfo != NULL && myInfo->my_level == my_level)
 	{
+		/* Must be at non-top of stack */
+		Assert(myInfo->parent != NULL);
+
 		ProcessInvalidationMessages(&myInfo->PriorCmdInvalidMsgs,
 									LocalExecuteInvalidationMessage);
-	}
 
-	/* Pop the transaction state stack */
-	transInvalInfo = myInfo->parent;
+		/* Pop the transaction state stack */
+		transInvalInfo = myInfo->parent;
 
-	/* Need not free anything else explicitly */
-	pfree(myInfo);
+		/* Need not free anything else explicitly */
+		pfree(myInfo);
+	}
 }
 
 /*