From 4c77cbb272948e96ce3ed02d444a944eb45d45e3 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 4 Oct 2004 21:52:15 +0000 Subject: [PATCH] PortalRun must guard against the possibility that the portal it's running contains VACUUM or a similar command that will internally start and commit transactions. In such a case, the original caller values of CurrentMemoryContext and CurrentResourceOwner will point to objects that will be destroyed by the internal commit. We must restore these pointers to point to the newly-manufactured transaction context and resource owner, rather than possibly pointing to deleted memory. Also tweak xact.c so that AbortTransaction and AbortSubTransaction forcibly restore a sane value for CurrentResourceOwner, much as they have always done for CurrentMemoryContext. I'm not certain this is necessary but I'm feeling paranoid today. Responds to Sean Chittenden's bug report of 4-Oct. --- src/backend/access/transam/xact.c | 36 +++++++++++++++++++++++--- src/backend/tcop/pquery.c | 42 +++++++++++++++++++++++++------ 2 files changed, 68 insertions(+), 10 deletions(-) diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index 17db7dd78d..321a86f30c 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -10,7 +10,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/access/transam/xact.c,v 1.190 2004/09/16 20:17:16 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/access/transam/xact.c,v 1.191 2004/10/04 21:52:14 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -205,6 +205,7 @@ static void AssignSubTransactionId(TransactionState s); static void AbortTransaction(void); static void AtAbort_Memory(void); static void AtCleanup_Memory(void); +static void AtAbort_ResourceOwner(void); static void AtCommit_LocalCache(void); static void AtCommit_Memory(void); static void AtStart_Cache(void); @@ -229,6 +230,7 @@ static void PopTransaction(void); static void AtSubAbort_Memory(void); static void AtSubCleanup_Memory(void); +static void AtSubAbort_ResourceOwner(void); static void AtSubCommit_Memory(void); static void AtSubStart_Memory(void); static void AtSubStart_ResourceOwner(void); @@ -1103,7 +1105,6 @@ AtAbort_Memory(void) MemoryContextSwitchTo(TopMemoryContext); } - /* * AtSubAbort_Memory */ @@ -1115,6 +1116,33 @@ AtSubAbort_Memory(void) MemoryContextSwitchTo(TopTransactionContext); } + +/* + * AtAbort_ResourceOwner + */ +static void +AtAbort_ResourceOwner(void) +{ + /* + * Make sure we have a valid ResourceOwner, if possible (else it + * will be NULL, which is OK) + */ + CurrentResourceOwner = TopTransactionResourceOwner; +} + +/* + * AtSubAbort_ResourceOwner + */ +static void +AtSubAbort_ResourceOwner(void) +{ + TransactionState s = CurrentTransactionState; + + /* Make sure we have a valid ResourceOwner */ + CurrentResourceOwner = s->curTransactionOwner; +} + + /* * AtSubAbort_childXids */ @@ -1598,8 +1626,9 @@ AbortTransaction(void) */ s->state = TRANS_ABORT; - /* Make sure we are in a valid memory context */ + /* Make sure we have a valid memory context and resource owner */ AtAbort_Memory(); + AtAbort_ResourceOwner(); /* * Reset user id which might have been changed transiently. We cannot @@ -3338,6 +3367,7 @@ AbortSubTransaction(void) * do abort processing */ AtSubAbort_Memory(); + AtSubAbort_ResourceOwner(); /* * We can skip all this stuff if the subxact failed before creating diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c index 5a1c8b4867..2c4c2f3c7e 100644 --- a/src/backend/tcop/pquery.c +++ b/src/backend/tcop/pquery.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/tcop/pquery.c,v 1.87 2004/09/13 20:07:05 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/tcop/pquery.c,v 1.88 2004/10/04 21:52:15 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -491,12 +491,14 @@ PortalRun(Portal portal, long count, char *completionTag) { bool result; + ResourceOwner saveTopTransactionResourceOwner; + MemoryContext saveTopTransactionContext; Portal saveActivePortal; Snapshot saveActiveSnapshot; ResourceOwner saveResourceOwner; MemoryContext savePortalContext; MemoryContext saveQueryContext; - MemoryContext oldContext; + MemoryContext saveMemoryContext; AssertArg(PortalIsValid(portal)); @@ -523,12 +525,26 @@ PortalRun(Portal portal, long count, /* * Set up global portal context pointers. + * + * We have to play a special game here to support utility commands like + * VACUUM and CLUSTER, which internally start and commit transactions. + * When we are called to execute such a command, CurrentResourceOwner + * will be pointing to the TopTransactionResourceOwner --- which will + * be destroyed and replaced in the course of the internal commit and + * restart. So we need to be prepared to restore it as pointing to + * the exit-time TopTransactionResourceOwner. (Ain't that ugly? This + * idea of internally starting whole new transactions is not good.) + * CurrentMemoryContext has a similar problem, but the other pointers + * we save here will be NULL or pointing to longer-lived objects. */ + saveTopTransactionResourceOwner = TopTransactionResourceOwner; + saveTopTransactionContext = TopTransactionContext; saveActivePortal = ActivePortal; saveActiveSnapshot = ActiveSnapshot; saveResourceOwner = CurrentResourceOwner; savePortalContext = PortalContext; saveQueryContext = QueryContext; + saveMemoryContext = CurrentMemoryContext; PG_TRY(); { ActivePortal = portal; @@ -537,7 +553,7 @@ PortalRun(Portal portal, long count, PortalContext = PortalGetHeapMemory(portal); QueryContext = portal->queryContext; - oldContext = MemoryContextSwitchTo(PortalContext); + MemoryContextSwitchTo(PortalContext); switch (portal->strategy) { @@ -620,9 +636,16 @@ PortalRun(Portal portal, long count, portal->status = PORTAL_FAILED; /* Restore global vars and propagate error */ + if (saveMemoryContext == saveTopTransactionContext) + MemoryContextSwitchTo(TopTransactionContext); + else + MemoryContextSwitchTo(saveMemoryContext); ActivePortal = saveActivePortal; ActiveSnapshot = saveActiveSnapshot; - CurrentResourceOwner = saveResourceOwner; + if (saveResourceOwner == saveTopTransactionResourceOwner) + CurrentResourceOwner = TopTransactionResourceOwner; + else + CurrentResourceOwner = saveResourceOwner; PortalContext = savePortalContext; QueryContext = saveQueryContext; @@ -630,11 +653,16 @@ PortalRun(Portal portal, long count, } PG_END_TRY(); - MemoryContextSwitchTo(oldContext); - + if (saveMemoryContext == saveTopTransactionContext) + MemoryContextSwitchTo(TopTransactionContext); + else + MemoryContextSwitchTo(saveMemoryContext); ActivePortal = saveActivePortal; ActiveSnapshot = saveActiveSnapshot; - CurrentResourceOwner = saveResourceOwner; + if (saveResourceOwner == saveTopTransactionResourceOwner) + CurrentResourceOwner = TopTransactionResourceOwner; + else + CurrentResourceOwner = saveResourceOwner; PortalContext = savePortalContext; QueryContext = saveQueryContext;