From: Tom Lane Date: Wed, 15 Feb 2012 21:18:39 +0000 (-0500) Subject: Run a portal's cleanup hook immediately when pushing it to FAILED state. X-Git-Tag: REL9_1_3~16 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=04a0231e56c4f401b681fe8a87829b4d11cd18ce;p=postgresql Run a portal's cleanup hook immediately when pushing it to FAILED state. This extends the changes of commit 6252c4f9e201f619e5eebda12fa867acd4e4200e so that we run the cleanup hook earlier for failure cases as well as success cases. As before, the point is to avoid an assertion failure from an Assert I added in commit a874fe7b4c890d1fe3455215a83ca777867beadd, which was meant to check that no user-written code can be called during portal cleanup. This fixes a case reported by Pavan Deolasee in which the Assert could be triggered during backend exit (see the new regression test case), and also prevents the possibility that the cleanup hook is run after portions of the portal's state have already been recycled. That doesn't really matter in current usage, but it foreseeably could matter in the future. Back-patch to 9.1 where the Assert in question was added. --- diff --git a/src/backend/commands/portalcmds.c b/src/backend/commands/portalcmds.c index 89086aa371..656a61cbde 100644 --- a/src/backend/commands/portalcmds.c +++ b/src/backend/commands/portalcmds.c @@ -224,7 +224,7 @@ PerformPortalClose(const char *name) } /* - * Note: PortalCleanup is called as a side-effect + * Note: PortalCleanup is called as a side-effect, if not already done. */ PortalDrop(portal, false); } @@ -234,6 +234,10 @@ PerformPortalClose(const char *name) * * Clean up a portal when it's dropped. This is the standard cleanup hook * for portals. + * + * Note: if portal->status is PORTAL_FAILED, we are probably being called + * during error abort, and must be careful to avoid doing anything that + * is likely to fail again. */ void PortalCleanup(Portal portal) @@ -420,7 +424,7 @@ PersistHoldablePortal(Portal portal) PG_CATCH(); { /* Uncaught error while executing portal: mark it dead */ - portal->status = PORTAL_FAILED; + MarkPortalFailed(portal); /* Restore global vars and propagate error */ ActivePortal = saveActivePortal; diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c index b7649c68fc..0e2494fa8a 100644 --- a/src/backend/tcop/pquery.c +++ b/src/backend/tcop/pquery.c @@ -608,7 +608,7 @@ PortalStart(Portal portal, ParamListInfo params, Snapshot snapshot) PG_CATCH(); { /* Uncaught error while executing portal: mark it dead */ - portal->status = PORTAL_FAILED; + MarkPortalFailed(portal); /* Restore global vars and propagate error */ ActivePortal = saveActivePortal; @@ -830,7 +830,7 @@ PortalRun(Portal portal, long count, bool isTopLevel, PG_CATCH(); { /* Uncaught error while executing portal: mark it dead */ - portal->status = PORTAL_FAILED; + MarkPortalFailed(portal); /* Restore global vars and propagate error */ if (saveMemoryContext == saveTopTransactionContext) @@ -1447,7 +1447,7 @@ PortalRunFetch(Portal portal, PG_CATCH(); { /* Uncaught error while executing portal: mark it dead */ - portal->status = PORTAL_FAILED; + MarkPortalFailed(portal); /* Restore global vars and propagate error */ ActivePortal = saveActivePortal; diff --git a/src/backend/utils/mmgr/portalmem.c b/src/backend/utils/mmgr/portalmem.c index 186548dcba..9a257e7351 100644 --- a/src/backend/utils/mmgr/portalmem.c +++ b/src/backend/utils/mmgr/portalmem.c @@ -404,6 +404,8 @@ UnpinPortal(Portal portal) /* * MarkPortalDone * Transition a portal from ACTIVE to DONE state. + * + * NOTE: never set portal->status = PORTAL_DONE directly; call this instead. */ void MarkPortalDone(Portal portal) @@ -417,8 +419,36 @@ MarkPortalDone(Portal portal) * well do that now, since the portal can't be executed any more. * * In some cases involving execution of a ROLLBACK command in an already - * aborted transaction, this prevents an assertion failure from reaching - * AtCleanup_Portals with the cleanup hook still unexecuted. + * aborted transaction, this prevents an assertion failure caused by + * reaching AtCleanup_Portals with the cleanup hook still unexecuted. + */ + if (PointerIsValid(portal->cleanup)) + { + (*portal->cleanup) (portal); + portal->cleanup = NULL; + } +} + +/* + * MarkPortalFailed + * Transition a portal into FAILED state. + * + * NOTE: never set portal->status = PORTAL_FAILED directly; call this instead. + */ +void +MarkPortalFailed(Portal portal) +{ + /* Perform the state transition */ + Assert(portal->status != PORTAL_DONE); + portal->status = PORTAL_FAILED; + + /* + * Allow portalcmds.c to clean up the state it knows about. We might as + * well do that now, since the portal can't be executed any more. + * + * In some cases involving cleanup of an already aborted transaction, this + * prevents an assertion failure caused by reaching AtCleanup_Portals with + * the cleanup hook still unexecuted. */ if (PointerIsValid(portal->cleanup)) { @@ -454,6 +484,9 @@ PortalDrop(Portal portal, bool isTopCommit) * hook's responsibility to not try to do that more than once, in the case * that failure occurs and then we come back to drop the portal again * during transaction abort. + * + * Note: in most paths of control, this will have been done already in + * MarkPortalDone or MarkPortalFailed. We're just making sure. */ if (PointerIsValid(portal->cleanup)) { @@ -712,7 +745,7 @@ AtAbort_Portals(void) /* Any portal that was actually running has to be considered broken */ if (portal->status == PORTAL_ACTIVE) - portal->status = PORTAL_FAILED; + MarkPortalFailed(portal); /* * Do nothing else to cursors held over from a previous transaction. @@ -727,9 +760,12 @@ AtAbort_Portals(void) * AtSubAbort_Portals. */ if (portal->status == PORTAL_READY) - portal->status = PORTAL_FAILED; + MarkPortalFailed(portal); - /* let portalcmds.c clean up the state it knows about */ + /* + * Allow portalcmds.c to clean up the state it knows about, if we + * haven't already. + */ if (PointerIsValid(portal->cleanup)) { (*portal->cleanup) (portal); @@ -861,9 +897,12 @@ AtSubAbort_Portals(SubTransactionId mySubid, */ if (portal->status == PORTAL_READY || portal->status == PORTAL_ACTIVE) - portal->status = PORTAL_FAILED; + MarkPortalFailed(portal); - /* let portalcmds.c clean up the state it knows about */ + /* + * Allow portalcmds.c to clean up the state it knows about, if we + * haven't already. + */ if (PointerIsValid(portal->cleanup)) { (*portal->cleanup) (portal); diff --git a/src/include/utils/portal.h b/src/include/utils/portal.h index 6af1cd5b10..cf50655e12 100644 --- a/src/include/utils/portal.h +++ b/src/include/utils/portal.h @@ -207,6 +207,7 @@ extern Portal CreateNewPortal(void); extern void PinPortal(Portal portal); extern void UnpinPortal(Portal portal); extern void MarkPortalDone(Portal portal); +extern void MarkPortalFailed(Portal portal); extern void PortalDrop(Portal portal, bool isTopCommit); extern Portal GetPortalByName(const char *name); extern void PortalDefineQuery(Portal portal, diff --git a/src/test/regress/expected/transactions.out b/src/test/regress/expected/transactions.out index f49ec0effe..e9d3908b1a 100644 --- a/src/test/regress/expected/transactions.out +++ b/src/test/regress/expected/transactions.out @@ -601,28 +601,11 @@ fetch from foo; (1 row) abort; --- tests for the "tid" type -SELECT '(3, 3)'::tid = '(3, 4)'::tid; - ?column? ----------- - f -(1 row) - -SELECT '(3, 3)'::tid = '(3, 3)'::tid; - ?column? ----------- - t -(1 row) - -SELECT '(3, 3)'::tid <> '(3, 3)'::tid; - ?column? ----------- - f -(1 row) - -SELECT '(3, 3)'::tid <> '(3, 4)'::tid; - ?column? ----------- - t -(1 row) - +-- Test for successful cleanup of an aborted transaction at session exit. +-- THIS MUST BE THE LAST TEST IN THIS FILE. +begin; +select 1/0; +ERROR: division by zero +rollback to X; +ERROR: no such savepoint +-- DO NOT ADD ANYTHING HERE. diff --git a/src/test/regress/sql/transactions.sql b/src/test/regress/sql/transactions.sql index 23271c8eab..faf6a9bf93 100644 --- a/src/test/regress/sql/transactions.sql +++ b/src/test/regress/sql/transactions.sql @@ -368,8 +368,11 @@ fetch from foo; abort; --- tests for the "tid" type -SELECT '(3, 3)'::tid = '(3, 4)'::tid; -SELECT '(3, 3)'::tid = '(3, 3)'::tid; -SELECT '(3, 3)'::tid <> '(3, 3)'::tid; -SELECT '(3, 3)'::tid <> '(3, 4)'::tid; +-- Test for successful cleanup of an aborted transaction at session exit. +-- THIS MUST BE THE LAST TEST IN THIS FILE. + +begin; +select 1/0; +rollback to X; + +-- DO NOT ADD ANYTHING HERE.