1 files changed, 82 insertions, 99 deletions

diff --git a/src/backend/utils/mmgr/portalmem.c b/src/backend/utils/mmgr/portalmem.c
index e61094209f8..0ca5e110393 100644
--- a/src/backend/utils/mmgr/portalmem.c
+++ b/src/backend/utils/mmgr/portalmem.c
@@ -422,17 +422,27 @@ PortalDrop(Portal portal, bool isTopCommit)
 				 errmsg("cannot drop active portal \"%s\"", portal->name)));
 
 	/*
-	 * Remove portal from hash table.  Because we do this first, we will not
+	 * Allow portalcmds.c to clean up the state it knows about, in particular
+	 * shutting down the executor if still active.  This step potentially runs
+	 * user-defined code so failure has to be expected.  It's the cleanup
+	 * 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.
+	 */
+	if (PointerIsValid(portal->cleanup))
+	{
+		(*portal->cleanup) (portal);
+		portal->cleanup = NULL;
+	}
+
+	/*
+	 * Remove portal from hash table.  Because we do this here, we will not
 	 * come back to try to remove the portal again if there's any error in the
 	 * subsequent steps.  Better to leak a little memory than to get into an
 	 * infinite error-recovery loop.
 	 */
 	PortalHashTableDelete(portal);
 
-	/* let portalcmds.c clean up the state it knows about */
-	if (PointerIsValid(portal->cleanup))
-		(*portal->cleanup) (portal);
-
 	/* drop cached plan reference, if any */
 	PortalReleaseCachedPlan(portal);
 
@@ -522,8 +532,15 @@ PortalHashTableDeleteAll(void)
 	{
 		Portal		portal = hentry->portal;
 
-		if (portal->status != PORTAL_ACTIVE)
-			PortalDrop(portal, false);
+		/* Can't close the active portal (the one running the command) */
+		if (portal->status == PORTAL_ACTIVE)
+			continue;
+
+		PortalDrop(portal, false);
+
+		/* Restart the iteration in case that led to other drops */
+		hash_seq_term(&status);
+		hash_seq_init(&status, PortalHashTable);
 	}
 }
 
@@ -531,14 +548,17 @@ PortalHashTableDeleteAll(void)
 /*
  * Pre-commit processing for portals.
  *
- * Any holdable cursors created in this transaction need to be converted to
+ * Holdable cursors created in this transaction need to be converted to
  * materialized form, since we are going to close down the executor and
- * release locks.  Other portals are not touched yet.
+ * release locks.  Non-holdable portals created in this transaction are
+ * simply removed.  Portals remaining from prior transactions should be
+ * left untouched.
  *
- * Returns TRUE if any holdable cursors were processed, FALSE if not.
+ * Returns TRUE if any portals changed state (possibly causing user-defined
+ * code to be run), FALSE if not.
  */
 bool
-CommitHoldablePortals(void)
+PreCommit_Portals(bool isPrepare)
 {
 	bool		result = false;
 	HASH_SEQ_STATUS status;
@@ -550,6 +570,26 @@ CommitHoldablePortals(void)
 	{
 		Portal		portal = hentry->portal;
 
+		/*
+		 * There should be no pinned portals anymore. Complain if someone
+		 * leaked one.
+		 */
+		if (portal->portalPinned)
+			elog(ERROR, "cannot commit while a portal is pinned");
+
+		/*
+		 * Do not touch active portals --- this can only happen in the case of
+		 * a multi-transaction utility command, such as VACUUM.
+		 *
+		 * Note however that any resource owner attached to such a portal is
+		 * still going to go away, so don't leave a dangling pointer.
+		 */
+		if (portal->status == PORTAL_ACTIVE)
+		{
+			portal->resowner = NULL;
+			continue;
+		}
+
 		/* Is it a holdable portal created in the current xact? */
 		if ((portal->cursorOptions & CURSOR_OPT_HOLD) &&
 			portal->createSubid != InvalidSubTransactionId &&
@@ -560,6 +600,15 @@ CommitHoldablePortals(void)
 			 * Instead of dropping the portal, prepare it for access by later
 			 * transactions.
 			 *
+			 * However, if this is PREPARE TRANSACTION rather than COMMIT,
+			 * refuse PREPARE, because the semantics seem pretty unclear.
+			 */
+			if (isPrepare)
+				ereport(ERROR,
+						(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+						 errmsg("cannot PREPARE a transaction that has created a cursor WITH HOLD")));
+
+			/*
 			 * Note that PersistHoldablePortal() must release all resources
 			 * used by the portal that are local to the creating transaction.
 			 */
@@ -582,108 +631,36 @@ CommitHoldablePortals(void)
 			 */
 			portal->createSubid = InvalidSubTransactionId;
 
+			/* Report we changed state */
 			result = true;
 		}
-	}
-
-	return result;
-}
-
-/*
- * Pre-prepare processing for portals.
- *
- * Currently we refuse PREPARE if the transaction created any holdable
- * cursors, since it's quite unclear what to do with one.  However, this
- * has the same API as CommitHoldablePortals and is invoked in the same
- * way by xact.c, so that we can easily do something reasonable if anyone
- * comes up with something reasonable to do.
- *
- * Returns TRUE if any holdable cursors were processed, FALSE if not.
- */
-bool
-PrepareHoldablePortals(void)
-{
-	bool		result = false;
-	HASH_SEQ_STATUS status;
-	PortalHashEnt *hentry;
-
-	hash_seq_init(&status, PortalHashTable);
-
-	while ((hentry = (PortalHashEnt *) hash_seq_search(&status)) != NULL)
-	{
-		Portal		portal = hentry->portal;
-
-		/* Is it a holdable portal created in the current xact? */
-		if ((portal->cursorOptions & CURSOR_OPT_HOLD) &&
-			portal->createSubid != InvalidSubTransactionId &&
-			portal->status == PORTAL_READY)
+		else if (portal->createSubid == InvalidSubTransactionId)
 		{
 			/*
-			 * We are exiting the transaction that created a holdable cursor.
-			 * Can't do PREPARE.
+			 * Do nothing to cursors held over from a previous transaction
+			 * (including ones we just froze in a previous cycle of this loop)
 			 */
-			ereport(ERROR,
-					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-					 errmsg("cannot PREPARE a transaction that has created a cursor WITH HOLD")));
-		}
-	}
-
-	return result;
-}
-
-/*
- * Pre-commit processing for portals.
- *
- * Remove all non-holdable portals created in this transaction.
- * Portals remaining from prior transactions should be left untouched.
- */
-void
-AtCommit_Portals(void)
-{
-	HASH_SEQ_STATUS status;
-	PortalHashEnt *hentry;
-
-	hash_seq_init(&status, PortalHashTable);
-
-	while ((hentry = (PortalHashEnt *) hash_seq_search(&status)) != NULL)
-	{
-		Portal		portal = hentry->portal;
-
-		/*
-		 * Do not touch active portals --- this can only happen in the case of
-		 * a multi-transaction utility command, such as VACUUM.
-		 *
-		 * Note however that any resource owner attached to such a portal is
-		 * still going to go away, so don't leave a dangling pointer.
-		 */
-		if (portal->status == PORTAL_ACTIVE)
-		{
-			portal->resowner = NULL;
 			continue;
 		}
+		else
+		{
+			/* Zap all non-holdable portals */
+			PortalDrop(portal, true);
 
-		/*
-		 * There should be no pinned portals anymore. Complain if someone
-		 * leaked one.
-		 */
-		if (portal->portalPinned)
-			elog(ERROR, "cannot commit while a portal is pinned");
+			/* Report we changed state */
+			result = true;
+		}
 
 		/*
-		 * Do nothing to cursors held over from a previous transaction
-		 * (including holdable ones just frozen by CommitHoldablePortals).
+		 * After either freezing or dropping a portal, we have to restart
+		 * the iteration, because we could have invoked user-defined code
+		 * that caused a drop of the next portal in the hash chain.
 		 */
-		if (portal->createSubid == InvalidSubTransactionId)
-			continue;
-
-		/* Zap all non-holdable portals */
-		PortalDrop(portal, true);
-
-		/* Restart the iteration in case that led to other drops */
-		/* XXX is this really necessary? */
 		hash_seq_term(&status);
 		hash_seq_init(&status, PortalHashTable);
 	}
+
+	return result;
 }
 
 /*
@@ -785,6 +762,9 @@ AtCleanup_Portals(void)
 		if (portal->portalPinned)
 			portal->portalPinned = false;
 
+		/* We had better not be calling any user-defined code here */
+		Assert(portal->cleanup == NULL);
+
 		/* Zap it. */
 		PortalDrop(portal, false);
 	}
@@ -913,6 +893,9 @@ AtSubCleanup_Portals(SubTransactionId mySubid)
 		if (portal->portalPinned)
 			portal->portalPinned = false;
 
+		/* We had better not be calling any user-defined code here */
+		Assert(portal->cleanup == NULL);
+
 		/* Zap it. */
 		PortalDrop(portal, false);
 	}