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));
    }
 }