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 ab8b55c1d2e96da7742d0c5e523dcbe4c2e70da5..1c7a1c3a33f44caa0d79fafb908734c4a59bd54d 100644 (file)
--- 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 f1504fac0d56fb372af7064d4bd9046e03bf6765..42a0fb0f1f26e400b7949b0be0a388d6b290d38d 100644 (file)
--- 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 89891ba9131a14e270319ea37d54b02d91f13eaf..cfb73c1b09057145752ea9b67126269898e047b5 100644 (file)
--- 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 b257d774c49dd2b3163da4947761a3948a432926..48339426549ae782f7baf1745f0bb7694752fdb6 100644 (file)
--- 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 f49ec0effee49b2b422edfb04e4976b6fc1d4a5c..e9d3908b1ab53d69fc833774a82375902b548610 100644 (file)
--- 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 23271c8eabaf41d61a1fe5a4c11b1d22f5642dc7..faf6a9bf9380444c9ab38bf642fc0a7f6e68e539 100644 (file)
--- 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.