Force READY portals into FAILED state when a transaction or subtransaction

is aborted, if they were created within the failed xact.  This prevents
ExecutorEnd from being run on them, which is a good idea because they may
contain references to tables or other objects that no longer exist.
In particular this is hazardous when auto_explain is active, but it's
really rather surprising that nobody has seen an issue with this before.
I'm back-patching this to 8.4, since that's the first version that contains
auto_explain or an ExecutorEnd hook, but I wonder whether we shouldn't
back-patch further.

diff --git a/src/backend/utils/mmgr/portalmem.c b/src/backend/utils/mmgr/portalmem.c
index 889d4653e14fa00628001cc48509f28930ea0925..58d9da4301f213e13bed5ff92f8c63f84289e388 100644 (file)
--- a/src/backend/utils/mmgr/portalmem.c
+++ b/src/backend/utils/mmgr/portalmem.c
@@ -12,7 +12,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/utils/mmgr/portalmem.c,v 1.116 2010/01/18 02:30:25 tgl Exp $
+ *       $PostgreSQL: pgsql/src/backend/utils/mmgr/portalmem.c,v 1.117 2010/02/18 03:06:46 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -668,6 +668,7 @@ AtAbort_Portals(void)
        {
                Portal          portal = hentry->portal;
 
+               /* Any portal that was actually running has to be considered broken */
                if (portal->status == PORTAL_ACTIVE)
                        portal->status = PORTAL_FAILED;
 
@@ -677,6 +678,15 @@ AtAbort_Portals(void)
                if (portal->createSubid == InvalidSubTransactionId)
                        continue;
 
+               /*
+                * If it was created in the current transaction, we can't do normal
+                * shutdown on a READY portal either; it might refer to objects
+                * created in the failed transaction.  See comments in
+                * AtSubAbort_Portals.
+                */
+               if (portal->status == PORTAL_READY)
+                       portal->status = PORTAL_FAILED;
+
                /* let portalcmds.c clean up the state it knows about */
                if (PointerIsValid(portal->cleanup))
                {
@@ -789,61 +799,41 @@ AtSubAbort_Portals(SubTransactionId mySubid,
                        continue;
 
                /*
-                * Force any active portals of my own transaction into FAILED state.
-                * This is mostly to ensure that a portal running a FETCH will go
-                * FAILED if the underlying cursor fails.  (Note we do NOT want to do
-                * this to upper-level portals, since they may be able to continue.)
-                *
-                * This is only needed to dodge the sanity check in PortalDrop.
+                * 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.)
                 */
-               if (portal->status == PORTAL_ACTIVE)
+               if (portal->status == PORTAL_READY ||
+                       portal->status == PORTAL_ACTIVE)
                        portal->status = PORTAL_FAILED;
 
-               /*
-                * If the portal is READY then allow it to survive into the parent
-                * transaction; otherwise shut it down.
-                *
-                * Currently, we can't actually support that because the portal's
-                * query might refer to objects created or changed in the failed
-                * subtransaction, leading to crashes if execution is resumed. So,
-                * even READY portals are deleted.      It would be nice to detect whether
-                * the query actually depends on any such object, instead.
-                */
-#ifdef NOT_USED
-               if (portal->status == PORTAL_READY)
+               /* let portalcmds.c clean up the state it knows about */
+               if (PointerIsValid(portal->cleanup))
                {
-                       portal->createSubid = parentSubid;
-                       if (portal->resowner)
-                               ResourceOwnerNewParent(portal->resowner, parentXactOwner);
+                       (*portal->cleanup) (portal);
+                       portal->cleanup = NULL;
                }
-               else
-#endif
-               {
-                       /* let portalcmds.c clean up the state it knows about */
-                       if (PointerIsValid(portal->cleanup))
-                       {
-                               (*portal->cleanup) (portal);
-                               portal->cleanup = NULL;
-                       }
 
-                       /* drop cached plan reference, if any */
-                       PortalReleaseCachedPlan(portal);
+               /* drop cached plan reference, if any */
+               PortalReleaseCachedPlan(portal);
 
-                       /*
-                        * Any resources belonging to the portal will be released in the
-                        * upcoming transaction-wide cleanup; they will be gone before we
-                        * run PortalDrop.
-                        */
-                       portal->resowner = NULL;
+               /*
+                * Any resources belonging to the portal will be released in the
+                * upcoming transaction-wide cleanup; they will be gone before we
+                * run PortalDrop.
+                */
+               portal->resowner = NULL;
 
-                       /*
-                        * Although we can't delete the portal data structure proper, we
-                        * can release any memory in subsidiary contexts, such as executor
-                        * state.  The cleanup hook was the last thing that might have
-                        * needed data there.
-                        */
-                       MemoryContextDeleteChildren(PortalGetHeapMemory(portal));
-               }
+               /*
+                * Although we can't delete the portal data structure proper, we
+                * can release any memory in subsidiary contexts, such as executor
+                * state.  The cleanup hook was the last thing that might have
+                * needed data there.
+                */
+               MemoryContextDeleteChildren(PortalGetHeapMemory(portal));
        }
 }