diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index c6182360b22..be3155abdee 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -4101,6 +4101,7 @@ AbortSubTransaction(void) AfterTriggerEndSubXact(false); AtSubAbort_Portals(s->subTransactionId, s->parent->subTransactionId, + s->curTransactionOwner, s->parent->curTransactionOwner); AtEOSubXact_LargeObject(false, s->subTransactionId, s->parent->subTransactionId); diff --git a/src/backend/commands/portalcmds.c b/src/backend/commands/portalcmds.c index db4b2601343..93d7606b70c 100644 --- a/src/backend/commands/portalcmds.c +++ b/src/backend/commands/portalcmds.c @@ -329,6 +329,7 @@ PersistHoldablePortal(Portal portal) (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("portal \"%s\" cannot be run", portal->name))); portal->status = PORTAL_ACTIVE; + portal->activeSubid = GetCurrentSubTransactionId(); /* * Set up global portal context pointers. diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c index 09135370c78..16cd11e4d0b 100644 --- a/src/backend/tcop/pquery.c +++ b/src/backend/tcop/pquery.c @@ -748,6 +748,7 @@ PortalRun(Portal portal, long count, bool isTopLevel, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("portal \"%s\" cannot be run", portal->name))); portal->status = PORTAL_ACTIVE; + portal->activeSubid = GetCurrentSubTransactionId(); /* * Set up global portal context pointers. @@ -1372,6 +1373,7 @@ PortalRunFetch(Portal portal, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("portal \"%s\" cannot be run", portal->name))); portal->status = PORTAL_ACTIVE; + portal->activeSubid = GetCurrentSubTransactionId(); /* * Set up global portal context pointers. diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c index 81595092f14..dedb24d43f6 100644 --- a/src/backend/utils/cache/relcache.c +++ b/src/backend/utils/cache/relcache.c @@ -1855,7 +1855,9 @@ RelationClearRelation(Relation relation, bool rebuild) { /* * As per notes above, a rel to be rebuilt MUST have refcnt > 0; while of - * course it would be a bad idea to blow away one with nonzero refcnt. + * course it would be an equally bad idea to blow away one with nonzero + * refcnt, since that would leave someone somewhere with a dangling + * pointer. All callers are expected to have verified that this holds. */ Assert(rebuild ? !RelationHasReferenceCountZero(relation) : @@ -2345,11 +2347,25 @@ AtEOXact_RelationCache(bool isCommit) { if (isCommit) relation->rd_createSubid = InvalidSubTransactionId; - else + else if (RelationHasReferenceCountZero(relation)) { RelationClearRelation(relation, false); continue; } + else + { + /* + * Hmm, somewhere there's a (leaked?) reference to the + * relation. We daren't remove the entry for fear of + * dereferencing a dangling pointer later. Bleat, and mark it + * as not belonging to the current transaction. Hopefully + * it'll get cleaned up eventually. This must be just a + * WARNING to avoid error-during-error-recovery loops. + */ + relation->rd_createSubid = InvalidSubTransactionId; + elog(WARNING, "cannot remove relcache entry for \"%s\" because it has nonzero refcount", + RelationGetRelationName(relation)); + } } /* @@ -2410,11 +2426,25 @@ AtEOSubXact_RelationCache(bool isCommit, SubTransactionId mySubid, { if (isCommit) relation->rd_createSubid = parentSubid; - else + else if (RelationHasReferenceCountZero(relation)) { RelationClearRelation(relation, false); continue; } + else + { + /* + * Hmm, somewhere there's a (leaked?) reference to the + * relation. We daren't remove the entry for fear of + * dereferencing a dangling pointer later. Bleat, and + * transfer it to the parent subtransaction so we can try + * again later. This must be just a WARNING to avoid + * error-during-error-recovery loops. + */ + relation->rd_createSubid = parentSubid; + elog(WARNING, "cannot remove relcache entry for \"%s\" because it has nonzero refcount", + RelationGetRelationName(relation)); + } } /* diff --git a/src/backend/utils/mmgr/portalmem.c b/src/backend/utils/mmgr/portalmem.c index 66fe97913bc..04c42aaa27a 100644 --- a/src/backend/utils/mmgr/portalmem.c +++ b/src/backend/utils/mmgr/portalmem.c @@ -231,6 +231,7 @@ CreatePortal(const char *name, bool allowDup, bool dupSilent) portal->status = PORTAL_NEW; portal->cleanup = PortalCleanup; portal->createSubid = GetCurrentSubTransactionId(); + portal->activeSubid = portal->createSubid; portal->strategy = PORTAL_MULTI_QUERY; portal->cursorOptions = CURSOR_OPT_NO_SCROLL; portal->atStart = true; @@ -581,6 +582,7 @@ CommitHoldablePortals(void) * not belonging to this transaction. */ portal->createSubid = InvalidSubTransactionId; + portal->activeSubid = InvalidSubTransactionId; result = true; } @@ -793,8 +795,8 @@ AtCleanup_Portals(void) /* * Pre-subcommit processing for portals. * - * Reassign the portals created in the current subtransaction to the parent - * subtransaction. + * Reassign portals created or used in the current subtransaction to the + * parent subtransaction. */ void AtSubCommit_Portals(SubTransactionId mySubid, @@ -816,14 +818,16 @@ AtSubCommit_Portals(SubTransactionId mySubid, if (portal->resowner) ResourceOwnerNewParent(portal->resowner, parentXactOwner); } + if (portal->activeSubid == mySubid) + portal->activeSubid = parentSubid; } } /* * Subtransaction abort handling for portals. * - * Deactivate portals created during the failed subtransaction. - * Note that per AtSubCommit_Portals, this will catch portals created + * Deactivate portals created or used during the failed subtransaction. + * Note that per AtSubCommit_Portals, this will catch portals created/used * in descendants of the subtransaction too. * * We don't destroy any portals here; that's done in AtSubCleanup_Portals. @@ -831,6 +835,7 @@ AtSubCommit_Portals(SubTransactionId mySubid, void AtSubAbort_Portals(SubTransactionId mySubid, SubTransactionId parentSubid, + ResourceOwner myXactOwner, ResourceOwner parentXactOwner) { HASH_SEQ_STATUS status; @@ -842,16 +847,66 @@ AtSubAbort_Portals(SubTransactionId mySubid, { Portal portal = hentry->portal; + /* Was it created in this subtransaction? */ if (portal->createSubid != mySubid) + { + /* No, but maybe it was used in this subtransaction? */ + if (portal->activeSubid == mySubid) + { + /* Maintain activeSubid until the portal is removed */ + portal->activeSubid = parentSubid; + + /* + * Upper-level portals that failed while running in this + * subtransaction must be forced into FAILED state, for the + * same reasons discussed below. + * + * We assume we can get away without forcing upper-level READY + * portals to fail, even if they were run and then suspended. + * In theory a suspended upper-level portal could have + * acquired some references to objects that are about to be + * destroyed, but there should be sufficient defenses against + * such cases: the portal's original query cannot contain such + * references, and any references within, say, cached plans of + * PL/pgSQL functions are not from active queries and should + * be protected by revalidation logic. + */ + if (portal->status == PORTAL_ACTIVE) + { + portal->status = PORTAL_FAILED; + /* let portalcmds.c clean up the state it knows about */ + if (PointerIsValid(portal->cleanup)) + { + (*portal->cleanup) (portal); + portal->cleanup = NULL; + } + } + + /* + * Also, if we failed it during the current subtransaction + * (either just above, or earlier), reattach its resource + * owner to the current subtransaction's resource owner, so + * that any resources it still holds will be released while + * cleaning up this subtransaction. This prevents some corner + * cases wherein we might get Asserts or worse while cleaning + * up objects created during the current subtransaction + * (because they're still referenced within this portal). + */ + if (portal->status == PORTAL_FAILED && portal->resowner) + { + ResourceOwnerNewParent(portal->resowner, myXactOwner); + portal->resowner = NULL; + } + } + /* Done if it wasn't created in this subtransaction */ continue; + } /* * Force any live portals of my own subtransaction into FAILED state. * We have to do this because they might refer to objects created or - * changed in the failed subtransaction, leading to crashes if - * execution is resumed, or even if we just try to run ExecutorEnd. - * (Note we do NOT do this to upper-level portals, since they cannot - * have such references and hence may be able to continue.) + * changed in the failed subtransaction, leading to crashes within + * ExecutorEnd when portalcmds.c tries to close down the portal. */ if (portal->status == PORTAL_READY || portal->status == PORTAL_ACTIVE) diff --git a/src/include/utils/portal.h b/src/include/utils/portal.h index 42df8546d92..f926638a8af 100644 --- a/src/include/utils/portal.h +++ b/src/include/utils/portal.h @@ -112,12 +112,15 @@ typedef struct PortalData MemoryContext heap; /* subsidiary memory for portal */ ResourceOwner resowner; /* resources owned by portal */ void (*cleanup) (Portal portal); /* cleanup hook */ - SubTransactionId createSubid; /* the ID of the creating subxact */ /* - * if createSubid is InvalidSubTransactionId, the portal is held over from - * a previous transaction + * State data for remembering which subtransaction(s) the portal was + * created or used in. If the portal is held over from a previous + * transaction, both subxids are InvalidSubTransactionId. Otherwise, + * createSubid is the creating subxact and activeSubid is the last subxact + * in which we ran the portal. */ + SubTransactionId createSubid; /* the creating subxact */ /* The query or queries the portal will execute */ const char *sourceText; /* text of query (as of 8.4, never NULL) */ @@ -168,6 +171,13 @@ typedef struct PortalData /* Presentation data, primarily used by the pg_cursors system view */ TimestampTz creation_time; /* time at which this portal was defined */ bool visible; /* include this portal in pg_cursors? */ + + /* + * This field belongs with createSubid, but in pre-9.5 branches, add it + * at the end to avoid creating an ABI break for extensions that examine + * Portal structs. + */ + SubTransactionId activeSubid; /* the last subxact with activity */ } PortalData; /* @@ -196,6 +206,7 @@ extern void AtSubCommit_Portals(SubTransactionId mySubid, ResourceOwner parentXactOwner); extern void AtSubAbort_Portals(SubTransactionId mySubid, SubTransactionId parentSubid, + ResourceOwner myXactOwner, ResourceOwner parentXactOwner); extern void AtSubCleanup_Portals(SubTransactionId mySubid); extern Portal CreatePortal(const char *name, bool allowDup, bool dupSilent); diff --git a/src/test/regress/expected/transactions.out b/src/test/regress/expected/transactions.out index c4f8965fd1e..90fd2a5decb 100644 --- a/src/test/regress/expected/transactions.out +++ b/src/test/regress/expected/transactions.out @@ -527,6 +527,52 @@ fetch from foo; (1 row) abort; +-- Test for proper cleanup after a failure in a cursor portal +-- that was created in an outer subtransaction +CREATE FUNCTION invert(x float8) RETURNS float8 LANGUAGE plpgsql AS +$$ begin return 1/x; end $$; +CREATE FUNCTION create_temp_tab() RETURNS text +LANGUAGE plpgsql AS $$ +BEGIN + CREATE TEMP TABLE new_table (f1 float8); + -- case of interest is that we fail while holding an open + -- relcache reference to new_table + INSERT INTO new_table SELECT invert(0.0); + RETURN 'foo'; +END $$; +BEGIN; +DECLARE ok CURSOR FOR SELECT * FROM int8_tbl; +DECLARE ctt CURSOR FOR SELECT create_temp_tab(); +FETCH ok; + q1 | q2 +-----+----- + 123 | 456 +(1 row) + +SAVEPOINT s1; +FETCH ok; -- should work + q1 | q2 +-----+------------------ + 123 | 4567890123456789 +(1 row) + +FETCH ctt; -- error occurs here +ERROR: division by zero +CONTEXT: PL/pgSQL function "invert" line 1 at RETURN +SQL statement "INSERT INTO new_table SELECT invert(0.0)" +PL/pgSQL function "create_temp_tab" line 5 at SQL statement +ROLLBACK TO s1; +FETCH ok; -- should work + q1 | q2 +------------------+----- + 4567890123456789 | 123 +(1 row) + +FETCH ctt; -- must be rejected +ERROR: portal "ctt" cannot be run +COMMIT; +DROP FUNCTION create_temp_tab(); +DROP FUNCTION invert(x float8); -- tests for the "tid" type SELECT '(3, 3)'::tid = '(3, 4)'::tid; ?column? diff --git a/src/test/regress/sql/transactions.sql b/src/test/regress/sql/transactions.sql index c670ae18d0c..12aa6906593 100644 --- a/src/test/regress/sql/transactions.sql +++ b/src/test/regress/sql/transactions.sql @@ -326,6 +326,38 @@ fetch from foo; abort; + +-- Test for proper cleanup after a failure in a cursor portal +-- that was created in an outer subtransaction +CREATE FUNCTION invert(x float8) RETURNS float8 LANGUAGE plpgsql AS +$$ begin return 1/x; end $$; + +CREATE FUNCTION create_temp_tab() RETURNS text +LANGUAGE plpgsql AS $$ +BEGIN + CREATE TEMP TABLE new_table (f1 float8); + -- case of interest is that we fail while holding an open + -- relcache reference to new_table + INSERT INTO new_table SELECT invert(0.0); + RETURN 'foo'; +END $$; + +BEGIN; +DECLARE ok CURSOR FOR SELECT * FROM int8_tbl; +DECLARE ctt CURSOR FOR SELECT create_temp_tab(); +FETCH ok; +SAVEPOINT s1; +FETCH ok; -- should work +FETCH ctt; -- error occurs here +ROLLBACK TO s1; +FETCH ok; -- should work +FETCH ctt; -- must be rejected +COMMIT; + +DROP FUNCTION create_temp_tab(); +DROP FUNCTION invert(x float8); + + -- tests for the "tid" type SELECT '(3, 3)'::tid = '(3, 4)'::tid; SELECT '(3, 3)'::tid = '(3, 3)'::tid;