Refactor the executor's API to support data-modifying CTEs better.

The originally committed patch for modifying CTEs didn't interact well
with EXPLAIN, as noted by myself, and also had corner-case problems with
triggers, as noted by Dean Rasheed.  Those problems show it is really not
practical for ExecutorEnd to call any user-defined code; so split the
cleanup duties out into a new function ExecutorFinish, which must be called
between the last ExecutorRun call and ExecutorEnd.  Some Asserts have been
added to these functions to help verify correct usage.

It is no longer necessary for callers of the executor to call
AfterTriggerBeginQuery/AfterTriggerEndQuery for themselves, as this is now
done by ExecutorStart/ExecutorFinish respectively.  If you really need to
suppress that and do it for yourself, pass EXEC_FLAG_SKIP_TRIGGERS to
ExecutorStart.

Also, refactor portal commit processing to allow for the possibility that
PortalDrop will invoke user-defined code.  I think this is not actually
necessary just yet, since the portal-execution-strategy logic forces any
non-pure-SELECT query to be run to completion before we will consider
committing.  But it seems like good future-proofing.

1 files changed, 82 insertions, 99 deletions

diff --git a/src/backend/utils/mmgr/portalmem.c b/src/backend/utils/mmgr/portalmem.c
index e61094209f..0ca5e11039 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);
 	}