diff --git a/src/backend/commands/portalcmds.c b/src/backend/commands/portalcmds.c index ab8b55c1d2..1c7a1c3a33 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 f1504fac0d..42a0fb0f1f 100644 --- a/src/backend/tcop/pquery.c +++ b/src/backend/tcop/pquery.c @@ -607,7 +607,7 @@ PortalStart(Portal portal, ParamListInfo params, bool use_active_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; @@ -829,7 +829,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) @@ -1446,7 +1446,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 89891ba913..cfb73c1b09 100644 --- a/src/backend/utils/mmgr/portalmem.c +++ b/src/backend/utils/mmgr/portalmem.c @@ -405,6 +405,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) @@ -418,8 +420,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)) { @@ -455,6 +485,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)) { @@ -713,7 +746,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. @@ -728,9 +761,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); @@ -862,9 +898,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 b257d774c4..4833942654 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.