From 083258e535c58c97e52ade7b0b68b5ed1879a678 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 6 Sep 2004 23:33:48 +0000 Subject: [PATCH] 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 | 22 +++++++++-- src/backend/commands/trigger.c | 64 +++++++++++++++++--------------- src/backend/storage/ipc/sinval.c | 17 ++++++--- src/backend/utils/cache/inval.c | 53 ++++++++++++++++++-------- 4 files changed, 102 insertions(+), 54 deletions(-) diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c index f9d257d1a1..f25b857045 100644 --- a/src/backend/commands/async.c +++ b/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); + } } /* diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 7e73f6b000..18260e6272 100644 --- a/src/backend/commands/trigger.c +++ b/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); } diff --git a/src/backend/storage/ipc/sinval.c b/src/backend/storage/ipc/sinval.c index 830d45169a..5c4db3da80 100644 --- a/src/backend/storage/ipc/sinval.c +++ b/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); } diff --git a/src/backend/utils/cache/inval.c b/src/backend/utils/cache/inval.c index 3c85b05dee..8dd806bb0d 100644 --- a/src/backend/utils/cache/inval.c +++ b/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; + + /* Need not free anything else explicitly */ + pfree(myInfo); } - - /* Pop the transaction state stack */ - transInvalInfo = myInfo->parent; - - /* Need not free anything else explicitly */ - pfree(myInfo); } /*