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 89086aa371738620f049993a328525bc88209a65..656a61cbdeb70d1d973237332cdea61751ef810e 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 b7649c68fcf26471c45c8d61643305e4b53acf52..0e2494fa8a509db14d8ad08854bee5d9ee069362 100644 (file)
--- 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 186548dcba5ee16703801e474830d1d5d35cb257..9a257e7351b4280c1d2c80e3b191f58164a12c8b 100644 (file)
--- 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 6af1cd5b10690795ae16eae7d6cfeca383594c49..cf50655e1252cb2185561aaf522dda97794cff99 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.