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.

contrib/auto_explain/auto_explain.c

@@ -40,6 +40,7 @@ static int	nesting_level = 0;
 /* Saved hook values in case of unload */
 static ExecutorStart_hook_type prev_ExecutorStart = NULL;
 static ExecutorRun_hook_type prev_ExecutorRun = NULL;
+static ExecutorFinish_hook_type prev_ExecutorFinish = NULL;
 static ExecutorEnd_hook_type prev_ExecutorEnd = NULL;
 
 #define auto_explain_enabled() \
@@ -53,6 +54,7 @@ static void explain_ExecutorStart(QueryDesc *queryDesc, int eflags);
 static void explain_ExecutorRun(QueryDesc *queryDesc,
 					ScanDirection direction,
 					long count);
+static void explain_ExecutorFinish(QueryDesc *queryDesc);
 static void explain_ExecutorEnd(QueryDesc *queryDesc);
 
 
@@ -132,6 +134,8 @@ _PG_init(void)
 	ExecutorStart_hook = explain_ExecutorStart;
 	prev_ExecutorRun = ExecutorRun_hook;
 	ExecutorRun_hook = explain_ExecutorRun;
+	prev_ExecutorFinish = ExecutorFinish_hook;
+	ExecutorFinish_hook = explain_ExecutorFinish;
 	prev_ExecutorEnd = ExecutorEnd_hook;
 	ExecutorEnd_hook = explain_ExecutorEnd;
 }
@@ -145,6 +149,7 @@ _PG_fini(void)
 	/* Uninstall hooks. */
 	ExecutorStart_hook = prev_ExecutorStart;
 	ExecutorRun_hook = prev_ExecutorRun;
+	ExecutorFinish_hook = prev_ExecutorFinish;
 	ExecutorEnd_hook = prev_ExecutorEnd;
 }
 
@@ -211,6 +216,29 @@ explain_ExecutorRun(QueryDesc *queryDesc, ScanDirection direction, long count)
 	PG_END_TRY();
 }
 
+/*
+ * ExecutorFinish hook: all we need do is track nesting depth
+ */
+static void
+explain_ExecutorFinish(QueryDesc *queryDesc)
+{
+	nesting_level++;
+	PG_TRY();
+	{
+		if (prev_ExecutorFinish)
+			prev_ExecutorFinish(queryDesc);
+		else
+			standard_ExecutorFinish(queryDesc);
+		nesting_level--;
+	}
+	PG_CATCH();
+	{
+		nesting_level--;
+		PG_RE_THROW();
+	}
+	PG_END_TRY();
+}
+
 /*
  * ExecutorEnd hook: log results if needed
  */

contrib/pg_stat_statements/pg_stat_statements.c

@@ -122,6 +122,7 @@ static int	nested_level = 0;
 static shmem_startup_hook_type prev_shmem_startup_hook = NULL;
 static ExecutorStart_hook_type prev_ExecutorStart = NULL;
 static ExecutorRun_hook_type prev_ExecutorRun = NULL;
+static ExecutorFinish_hook_type prev_ExecutorFinish = NULL;
 static ExecutorEnd_hook_type prev_ExecutorEnd = NULL;
 static ProcessUtility_hook_type prev_ProcessUtility = NULL;
 
@@ -173,6 +174,7 @@ static void pgss_ExecutorStart(QueryDesc *queryDesc, int eflags);
 static void pgss_ExecutorRun(QueryDesc *queryDesc,
 				 ScanDirection direction,
 				 long count);
+static void pgss_ExecutorFinish(QueryDesc *queryDesc);
 static void pgss_ExecutorEnd(QueryDesc *queryDesc);
 static void pgss_ProcessUtility(Node *parsetree,
 			  const char *queryString, ParamListInfo params, bool isTopLevel,
@@ -269,6 +271,8 @@ _PG_init(void)
 	ExecutorStart_hook = pgss_ExecutorStart;
 	prev_ExecutorRun = ExecutorRun_hook;
 	ExecutorRun_hook = pgss_ExecutorRun;
+	prev_ExecutorFinish = ExecutorFinish_hook;
+	ExecutorFinish_hook = pgss_ExecutorFinish;
 	prev_ExecutorEnd = ExecutorEnd_hook;
 	ExecutorEnd_hook = pgss_ExecutorEnd;
 	prev_ProcessUtility = ProcessUtility_hook;
@@ -285,6 +289,7 @@ _PG_fini(void)
 	shmem_startup_hook = prev_shmem_startup_hook;
 	ExecutorStart_hook = prev_ExecutorStart;
 	ExecutorRun_hook = prev_ExecutorRun;
+	ExecutorFinish_hook = prev_ExecutorFinish;
 	ExecutorEnd_hook = prev_ExecutorEnd;
 	ProcessUtility_hook = prev_ProcessUtility;
 }
@@ -549,6 +554,29 @@ pgss_ExecutorRun(QueryDesc *queryDesc, ScanDirection direction, long count)
 	PG_END_TRY();
 }
 
+/*
+ * ExecutorFinish hook: all we need do is track nesting depth
+ */
+static void
+pgss_ExecutorFinish(QueryDesc *queryDesc)
+{
+	nested_level++;
+	PG_TRY();
+	{
+		if (prev_ExecutorFinish)
+			prev_ExecutorFinish(queryDesc);
+		else
+			standard_ExecutorFinish(queryDesc);
+		nested_level--;
+	}
+	PG_CATCH();
+	{
+		nested_level--;
+		PG_RE_THROW();
+	}
+	PG_END_TRY();
+}
+
 /*
  * ExecutorEnd hook: store results if needed
  */

src/backend/access/transam/xact.c

@@ -1753,12 +1753,10 @@ CommitTransaction(void)
 	Assert(s->parent == NULL);
 
 	/*
-	 * Do pre-commit processing (most of this stuff requires database access,
-	 * and in fact could still cause an error...)
-	 *
-	 * It is possible for CommitHoldablePortals to invoke functions that queue
-	 * deferred triggers, and it's also possible that triggers create holdable
-	 * cursors.  So we have to loop until there's nothing left to do.
+	 * Do pre-commit processing that involves calling user-defined code, such
+	 * as triggers.  Since closing cursors could queue trigger actions,
+	 * triggers could open cursors, etc, we have to keep looping until there's
+	 * nothing left to do.
 	 */
 	for (;;)
 	{
@@ -1768,19 +1766,23 @@ CommitTransaction(void)
 		AfterTriggerFireDeferred();
 
 		/*
-		 * Convert any open holdable cursors into static portals.  If there
-		 * weren't any, we are done ... otherwise loop back to check if they
-		 * queued deferred triggers.  Lather, rinse, repeat.
+		 * Close open portals (converting holdable ones into static portals).
+		 * If there weren't any, we are done ... otherwise loop back to check
+		 * if they queued deferred triggers.  Lather, rinse, repeat.
 		 */
-		if (!CommitHoldablePortals())
+		if (!PreCommit_Portals(false))
 			break;
 	}
 
-	/* Now we can shut down the deferred-trigger manager */
-	AfterTriggerEndXact(true);
+	/*
+	 * The remaining actions cannot call any user-defined code, so it's
+	 * safe to start shutting down within-transaction services.  But note
+	 * that most of this stuff could still throw an error, which would
+	 * switch us into the transaction-abort path.
+	 */
 
-	/* Close any open regular cursors */
-	AtCommit_Portals();
+	/* Shut down the deferred-trigger manager */
+	AfterTriggerEndXact(true);
 
 	/*
 	 * Let ON COMMIT management do its thing (must happen after closing
@@ -1954,12 +1956,10 @@ PrepareTransaction(void)
 	Assert(s->parent == NULL);
 
 	/*
-	 * Do pre-commit processing (most of this stuff requires database access,
-	 * and in fact could still cause an error...)
-	 *
-	 * It is possible for PrepareHoldablePortals to invoke functions that
-	 * queue deferred triggers, and it's also possible that triggers create
-	 * holdable cursors.  So we have to loop until there's nothing left to do.
+	 * Do pre-commit processing that involves calling user-defined code, such
+	 * as triggers.  Since closing cursors could queue trigger actions,
+	 * triggers could open cursors, etc, we have to keep looping until there's
+	 * nothing left to do.
 	 */
 	for (;;)
 	{
@@ -1969,19 +1969,23 @@ PrepareTransaction(void)
 		AfterTriggerFireDeferred();
 
 		/*
-		 * Convert any open holdable cursors into static portals.  If there
-		 * weren't any, we are done ... otherwise loop back to check if they
-		 * queued deferred triggers.  Lather, rinse, repeat.
+		 * Close open portals (converting holdable ones into static portals).
+		 * If there weren't any, we are done ... otherwise loop back to check
+		 * if they queued deferred triggers.  Lather, rinse, repeat.
 		 */
-		if (!PrepareHoldablePortals())
+		if (!PreCommit_Portals(true))
 			break;
 	}
 
-	/* Now we can shut down the deferred-trigger manager */
-	AfterTriggerEndXact(true);
+	/*
+	 * The remaining actions cannot call any user-defined code, so it's
+	 * safe to start shutting down within-transaction services.  But note
+	 * that most of this stuff could still throw an error, which would
+	 * switch us into the transaction-abort path.
+	 */
 
-	/* Close any open regular cursors */
-	AtCommit_Portals();
+	/* Shut down the deferred-trigger manager */
+	AfterTriggerEndXact(true);
 
 	/*
 	 * Let ON COMMIT management do its thing (must happen after closing

src/backend/commands/copy.c

@@ -1463,6 +1463,7 @@ EndCopyTo(CopyState cstate)
 	if (cstate->queryDesc != NULL)
 	{
 		/* Close down the query and free resources. */
+		ExecutorFinish(cstate->queryDesc);
 		ExecutorEnd(cstate->queryDesc);
 		FreeQueryDesc(cstate->queryDesc);
 		PopActiveSnapshot();

src/backend/commands/discard.c

@@ -61,10 +61,11 @@ DiscardAll(bool isTopLevel)
 	 */
 	PreventTransactionChain(isTopLevel, "DISCARD ALL");
 
+	/* Closing portals might run user-defined code, so do that first. */
+	PortalHashTableDeleteAll();
 	SetPGVariable("session_authorization", NIL, false);
 	ResetAllOptions();
 	DropAllPreparedStatements();
-	PortalHashTableDeleteAll();
 	Async_UnlistenAll();
 	LockReleaseAll(USER_LOCKMETHOD, true);
 	ResetPlanCache();

src/backend/commands/explain.c

@@ -360,6 +360,8 @@ ExplainOnePlan(PlannedStmt *plannedstmt, ExplainState *es,
 	if (es->buffers)
 		instrument_option |= INSTRUMENT_BUFFERS;
 
+	INSTR_TIME_SET_CURRENT(starttime);
+
 	/*
 	 * Use a snapshot with an updated command ID to ensure this query sees
 	 * results of any previously executed queries.
@@ -371,12 +373,6 @@ ExplainOnePlan(PlannedStmt *plannedstmt, ExplainState *es,
 								GetActiveSnapshot(), InvalidSnapshot,
 								None_Receiver, params, instrument_option);
 
-	INSTR_TIME_SET_CURRENT(starttime);
-
-	/* If analyzing, we need to cope with queued triggers */
-	if (es->analyze)
-		AfterTriggerBeginQuery();
-
 	/* Select execution options */
 	if (es->analyze)
 		eflags = 0;				/* default run-to-completion flags */
@@ -392,7 +388,10 @@ ExplainOnePlan(PlannedStmt *plannedstmt, ExplainState *es,
 		/* run the plan */
 		ExecutorRun(queryDesc, ForwardScanDirection, 0L);
 
-		/* We can't clean up 'till we're done printing the stats... */
+		/* run cleanup too */
+		ExecutorFinish(queryDesc);
+
+		/* We can't run ExecutorEnd 'till we're done printing the stats... */
 		totaltime += elapsed_time(&starttime);
 	}
 
@@ -401,18 +400,6 @@ ExplainOnePlan(PlannedStmt *plannedstmt, ExplainState *es,
 	/* Create textual dump of plan tree */
 	ExplainPrintPlan(es, queryDesc);
 
-	/*
-	 * If we ran the command, run any AFTER triggers it queued.  (Note this
-	 * will not include DEFERRED triggers; since those don't run until end of
-	 * transaction, we can't measure them.)  Include into total runtime.
-	 */
-	if (es->analyze)
-	{
-		INSTR_TIME_SET_CURRENT(starttime);
-		AfterTriggerEndQuery(queryDesc->estate);
-		totaltime += elapsed_time(&starttime);
-	}
-
 	/* Print info about runtime of triggers */
 	if (es->analyze)
 	{

src/backend/commands/extension.c

@@ -726,10 +726,9 @@ execute_sql_string(const char *sql, const char *filename)
 										GetActiveSnapshot(), NULL,
 										dest, NULL, 0);
 
-				AfterTriggerBeginQuery();
 				ExecutorStart(qdesc, 0);
 				ExecutorRun(qdesc, ForwardScanDirection, 0);
-				AfterTriggerEndQuery(qdesc->estate);
+				ExecutorFinish(qdesc);
 				ExecutorEnd(qdesc);
 
 				FreeQueryDesc(qdesc);

src/backend/commands/portalcmds.c

@@ -254,7 +254,14 @@ PortalCleanup(Portal portal)
 	queryDesc = PortalGetQueryDesc(portal);
 	if (queryDesc)
 	{
+		/*
+		 * Reset the queryDesc before anything else.  This prevents us
+		 * from trying to shut down the executor twice, in case of an
+		 * error below.  The transaction abort mechanisms will take care
+		 * of resource cleanup in such a case.
+		 */
 		portal->queryDesc = NULL;
+
 		if (portal->status != PORTAL_FAILED)
 		{
 			ResourceOwner saveResourceOwner;
@@ -264,7 +271,7 @@ PortalCleanup(Portal portal)
 			PG_TRY();
 			{
 				CurrentResourceOwner = portal->resowner;
-				/* we do not need AfterTriggerEndQuery() here */
+				ExecutorFinish(queryDesc);
 				ExecutorEnd(queryDesc);
 				FreeQueryDesc(queryDesc);
 			}
@@ -371,7 +378,7 @@ PersistHoldablePortal(Portal portal)
 		 * Now shut down the inner executor.
 		 */
 		portal->queryDesc = NULL;		/* prevent double shutdown */
-		/* we do not need AfterTriggerEndQuery() here */
+		ExecutorFinish(queryDesc);
 		ExecutorEnd(queryDesc);
 		FreeQueryDesc(queryDesc);
 

src/backend/commands/trigger.c

@@ -3676,9 +3676,9 @@ AfterTriggerBeginQuery(void)
  *	we invoke all AFTER IMMEDIATE trigger events queued by the query, and
  *	transfer deferred trigger events to the global deferred-trigger list.
  *
- *	Note that this should be called just BEFORE closing down the executor
+ *	Note that this must be called BEFORE closing down the executor
  *	with ExecutorEnd, because we make use of the EState's info about
- *	target relations.
+ *	target relations.  Normally it is called from ExecutorFinish.
  * ----------
  */
 void

src/backend/executor/README

@@ -95,12 +95,17 @@ This is a sketch of control flow for full query processing:
 			CreateExprContext
 				creates per-tuple context
 			ExecInitExpr
+		AfterTriggerBeginQuery
 
 	ExecutorRun
 		ExecProcNode --- recursively called in per-query context
 			ExecEvalExpr --- called in per-tuple context
 			ResetExprContext --- to free memory
 
+	ExecutorFinish
+		ExecPostprocessPlan --- run any unfinished ModifyTable nodes
+		AfterTriggerEndQuery
+
 	ExecutorEnd
 		ExecEndNode --- recursively releases resources
 		FreeExecutorState

src/backend/executor/execMain.c

@@ -6,20 +6,25 @@
  * INTERFACE ROUTINES
  *	ExecutorStart()
  *	ExecutorRun()
+ *	ExecutorFinish()
  *	ExecutorEnd()
  *
- *	The old ExecutorMain() has been replaced by ExecutorStart(),
- *	ExecutorRun() and ExecutorEnd()
- *
- *	These three procedures are the external interfaces to the executor.
+ *	These four procedures are the external interface to the executor.
  *	In each case, the query descriptor is required as an argument.
  *
- *	ExecutorStart() must be called at the beginning of execution of any
- *	query plan and ExecutorEnd() should always be called at the end of
- *	execution of a plan.
+ *	ExecutorStart must be called at the beginning of execution of any
+ *	query plan and ExecutorEnd must always be called at the end of
+ *	execution of a plan (unless it is aborted due to error).
  *
  *	ExecutorRun accepts direction and count arguments that specify whether
  *	the plan is to be executed forwards, backwards, and for how many tuples.
+ *	In some cases ExecutorRun may be called multiple times to process all
+ *	the tuples for a plan.  It is also acceptable to stop short of executing
+ *	the whole plan (but only if it is a SELECT).
+ *
+ *	ExecutorFinish must be called after the final ExecutorRun call and
+ *	before ExecutorEnd.  This can be omitted only in case of EXPLAIN,
+ *	which should also omit ExecutorRun.
  *
  * Portions Copyright (c) 1996-2011, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
@@ -58,9 +63,10 @@
 #include "utils/tqual.h"
 
 
-/* Hooks for plugins to get control in ExecutorStart/Run/End() */
+/* Hooks for plugins to get control in ExecutorStart/Run/Finish/End */
 ExecutorStart_hook_type ExecutorStart_hook = NULL;
 ExecutorRun_hook_type ExecutorRun_hook = NULL;
+ExecutorFinish_hook_type ExecutorFinish_hook = NULL;
 ExecutorEnd_hook_type ExecutorEnd_hook = NULL;
 
 /* Hook for plugin to get control in ExecCheckRTPerms() */
@@ -96,8 +102,8 @@ static void intorel_destroy(DestReceiver *self);
  *		This routine must be called at the beginning of any execution of any
  *		query plan
  *
- * Takes a QueryDesc previously created by CreateQueryDesc (it's not real
- * clear why we bother to separate the two functions, but...).	The tupDesc
+ * Takes a QueryDesc previously created by CreateQueryDesc (which is separate
+ * only because some places use QueryDescs for utility commands).  The tupDesc
  * field of the QueryDesc is filled in to describe the tuples that will be
  * returned, and the internal fields (estate and planstate) are set up.
  *
@@ -170,6 +176,15 @@ standard_ExecutorStart(QueryDesc *queryDesc, int eflags)
 				queryDesc->plannedstmt->rowMarks != NIL ||
 				queryDesc->plannedstmt->hasModifyingCTE)
 				estate->es_output_cid = GetCurrentCommandId(true);
+
+			/*
+			 * A SELECT without modifying CTEs can't possibly queue triggers,
+			 * so force skip-triggers mode. This is just a marginal efficiency
+			 * hack, since AfterTriggerBeginQuery/AfterTriggerEndQuery aren't
+			 * all that expensive, but we might as well do it.
+			 */
+			if (!queryDesc->plannedstmt->hasModifyingCTE)
+				eflags |= EXEC_FLAG_SKIP_TRIGGERS;
 			break;
 
 		case CMD_INSERT:
@@ -189,6 +204,7 @@ standard_ExecutorStart(QueryDesc *queryDesc, int eflags)
 	 */
 	estate->es_snapshot = RegisterSnapshot(queryDesc->snapshot);
 	estate->es_crosscheck_snapshot = RegisterSnapshot(queryDesc->crosscheck_snapshot);
+	estate->es_top_eflags = eflags;
 	estate->es_instrument = queryDesc->instrument_options;
 
 	/*
@@ -196,6 +212,13 @@ standard_ExecutorStart(QueryDesc *queryDesc, int eflags)
 	 */
 	InitPlan(queryDesc, eflags);
 
+	/*
+	 * Set up an AFTER-trigger statement context, unless told not to, or
+	 * unless it's EXPLAIN-only mode (when ExecutorFinish won't be called).
+	 */
+	if (!(eflags & (EXEC_FLAG_SKIP_TRIGGERS | EXEC_FLAG_EXPLAIN_ONLY)))
+		AfterTriggerBeginQuery();
+
 	MemoryContextSwitchTo(oldcontext);
 }
 
@@ -252,13 +275,14 @@ standard_ExecutorRun(QueryDesc *queryDesc,
 	estate = queryDesc->estate;
 
 	Assert(estate != NULL);
+	Assert(!(estate->es_top_eflags & EXEC_FLAG_EXPLAIN_ONLY));
 
 	/*
 	 * Switch into per-query memory context
 	 */
 	oldcontext = MemoryContextSwitchTo(estate->es_query_cxt);
 
-	/* Allow instrumentation of ExecutorRun overall runtime */
+	/* Allow instrumentation of Executor overall runtime */
 	if (queryDesc->totaltime)
 		InstrStartNode(queryDesc->totaltime);
 
@@ -304,6 +328,68 @@ standard_ExecutorRun(QueryDesc *queryDesc,
 	MemoryContextSwitchTo(oldcontext);
 }
 
+/* ----------------------------------------------------------------
+ *		ExecutorFinish
+ *
+ *		This routine must be called after the last ExecutorRun call.
+ *		It performs cleanup such as firing AFTER triggers.  It is
+ *		separate from ExecutorEnd because EXPLAIN ANALYZE needs to
+ *		include these actions in the total runtime.
+ *
+ *		We provide a function hook variable that lets loadable plugins
+ *		get control when ExecutorFinish is called.  Such a plugin would
+ *		normally call standard_ExecutorFinish().
+ *
+ * ----------------------------------------------------------------
+ */
+void
+ExecutorFinish(QueryDesc *queryDesc)
+{
+	if (ExecutorFinish_hook)
+		(*ExecutorFinish_hook) (queryDesc);
+	else
+		standard_ExecutorFinish(queryDesc);
+}
+
+void
+standard_ExecutorFinish(QueryDesc *queryDesc)
+{
+	EState	   *estate;
+	MemoryContext oldcontext;
+
+	/* sanity checks */
+	Assert(queryDesc != NULL);
+
+	estate = queryDesc->estate;
+
+	Assert(estate != NULL);
+	Assert(!(estate->es_top_eflags & EXEC_FLAG_EXPLAIN_ONLY));
+
+	/* This should be run once and only once per Executor instance */
+	Assert(!estate->es_finished);
+
+	/* Switch into per-query memory context */
+	oldcontext = MemoryContextSwitchTo(estate->es_query_cxt);
+
+	/* Allow instrumentation of Executor overall runtime */
+	if (queryDesc->totaltime)
+		InstrStartNode(queryDesc->totaltime);
+
+	/* Run ModifyTable nodes to completion */
+	ExecPostprocessPlan(estate);
+
+	/* Execute queued AFTER triggers, unless told not to */
+	if (!(estate->es_top_eflags & EXEC_FLAG_SKIP_TRIGGERS))
+		AfterTriggerEndQuery(estate);
+
+	if (queryDesc->totaltime)
+		InstrStopNode(queryDesc->totaltime, 0);
+
+	MemoryContextSwitchTo(oldcontext);
+
+	estate->es_finished = true;
+}
+
 /* ----------------------------------------------------------------
  *		ExecutorEnd
  *
@@ -312,19 +398,13 @@ standard_ExecutorRun(QueryDesc *queryDesc,
  *
  *		We provide a function hook variable that lets loadable plugins
  *		get control when ExecutorEnd is called.  Such a plugin would
- *		normally call standard_ExecutorEnd().  Because such hooks expect
- *		to execute after all plan execution is done, we run
- *		ExecPostprocessPlan before invoking the hook.
+ *		normally call standard_ExecutorEnd().
  *
  * ----------------------------------------------------------------
  */
 void
 ExecutorEnd(QueryDesc *queryDesc)
 {
-	/* Let plan nodes do any final processing required */
-	ExecPostprocessPlan(queryDesc->estate);
-
-	/* Now close down */
 	if (ExecutorEnd_hook)
 		(*ExecutorEnd_hook) (queryDesc);
 	else
@@ -344,6 +424,14 @@ standard_ExecutorEnd(QueryDesc *queryDesc)
 
 	Assert(estate != NULL);
 
+	/*
+	 * Check that ExecutorFinish was called, unless in EXPLAIN-only mode.
+	 * This Assert is needed because ExecutorFinish is new as of 9.1, and
+	 * callers might forget to call it.
+	 */
+	Assert(estate->es_finished ||
+		   (estate->es_top_eflags & EXEC_FLAG_EXPLAIN_ONLY));
+
 	/*
 	 * Switch into per-query memory context to run ExecEndPlan
 	 */
@@ -1141,14 +1229,8 @@ ExecContextForcesOids(PlanState *planstate, bool *hasoids)
 static void
 ExecPostprocessPlan(EState *estate)
 {
-	MemoryContext oldcontext;
 	ListCell   *lc;
 
-	/*
-	 * Switch into per-query memory context
-	 */
-	oldcontext = MemoryContextSwitchTo(estate->es_query_cxt);
-
 	/*
 	 * Make sure nodes run forward.
 	 */
@@ -1176,8 +1258,6 @@ ExecPostprocessPlan(EState *estate)
 				break;
 		}
 	}
-
-	MemoryContextSwitchTo(oldcontext);
 }
 
 /* ----------------------------------------------------------------
@@ -2081,10 +2161,11 @@ EvalPlanQualStart(EPQState *epqstate, EState *parentestate, Plan *planTree)
 	estate->es_result_relation_info = parentestate->es_result_relation_info;
 	/* es_trig_target_relations must NOT be copied */
 	estate->es_rowMarks = parentestate->es_rowMarks;
+	estate->es_top_eflags = parentestate->es_top_eflags;
 	estate->es_instrument = parentestate->es_instrument;
 	estate->es_select_into = parentestate->es_select_into;
 	estate->es_into_oids = parentestate->es_into_oids;
-	estate->es_auxmodifytables = NIL;
+	/* es_auxmodifytables must NOT be copied */
 
 	/*
 	 * The external param list is simply shared from parent.  The internal

src/backend/executor/execUtils.c

@@ -137,9 +137,11 @@ CreateExecutorState(void)
 	estate->es_processed = 0;
 	estate->es_lastoid = InvalidOid;
 
-	estate->es_instrument = false;
+	estate->es_top_eflags = 0;
+	estate->es_instrument = 0;
 	estate->es_select_into = false;
 	estate->es_into_oids = false;
+	estate->es_finished = false;
 
 	estate->es_exprcontexts = NIL;
 

src/backend/executor/functions.c

@@ -205,7 +205,8 @@ init_execution_state(List *queryTree_list,
 
 			if (ps->commandType == CMD_SELECT &&
 				ps->utilityStmt == NULL &&
-				ps->intoClause == NULL)
+				ps->intoClause == NULL &&
+				!ps->hasModifyingCTE)
 				fcache->lazyEval = lasttages->lazyEval = true;
 		}
 	}
@@ -431,14 +432,19 @@ postquel_start(execution_state *es, SQLFunctionCachePtr fcache)
 	if (es->qd->utilitystmt == NULL)
 	{
 		/*
-		 * Only set up to collect queued triggers if it's not a SELECT. This
-		 * isn't just an optimization, but is necessary in case a SELECT
-		 * returns multiple rows to caller --- we mustn't exit from the
-		 * function execution with a stacked AfterTrigger level still active.
+		 * In lazyEval mode, do not let the executor set up an AfterTrigger
+		 * context.  This is necessary not just an optimization, because we
+		 * mustn't exit from the function execution with a stacked
+		 * AfterTrigger level still active.  We are careful not to select
+		 * lazyEval mode for any statement that could possibly queue triggers.
 		 */
-		if (es->qd->operation != CMD_SELECT)
-			AfterTriggerBeginQuery();
-		ExecutorStart(es->qd, 0);
+		int			eflags;
+
+		if (es->lazyEval)
+			eflags = EXEC_FLAG_SKIP_TRIGGERS;
+		else
+			eflags = 0;			/* default run-to-completion flags */
+		ExecutorStart(es->qd, eflags);
 	}
 
 	es->status = F_EXEC_RUN;
@@ -499,8 +505,7 @@ postquel_end(execution_state *es)
 		/* Make our snapshot the active one for any called functions */
 		PushActiveSnapshot(es->qd->snapshot);
 
-		if (es->qd->operation != CMD_SELECT)
-			AfterTriggerEndQuery(es->qd->estate);
+		ExecutorFinish(es->qd);
 		ExecutorEnd(es->qd);
 
 		PopActiveSnapshot();

src/backend/executor/spi.c

@@ -2041,6 +2041,7 @@ static int
 _SPI_pquery(QueryDesc *queryDesc, bool fire_triggers, long tcount)
 {
 	int			operation = queryDesc->operation;
+	int			eflags;
 	int			res;
 
 	switch (operation)
@@ -2084,10 +2085,13 @@ _SPI_pquery(QueryDesc *queryDesc, bool fire_triggers, long tcount)
 		ResetUsage();
 #endif
 
+	/* Select execution options */
 	if (fire_triggers)
-		AfterTriggerBeginQuery();
+		eflags = 0;				/* default run-to-completion flags */
+	else
+		eflags = EXEC_FLAG_SKIP_TRIGGERS;
 
-	ExecutorStart(queryDesc, 0);
+	ExecutorStart(queryDesc, eflags);
 
 	ExecutorRun(queryDesc, ForwardScanDirection, tcount);
 
@@ -2101,10 +2105,7 @@ _SPI_pquery(QueryDesc *queryDesc, bool fire_triggers, long tcount)
 			elog(ERROR, "consistency check on SPI tuple count failed");
 	}
 
-	/* Take care of any queued AFTER triggers */
-	if (fire_triggers)
-		AfterTriggerEndQuery(queryDesc->estate);
-
+	ExecutorFinish(queryDesc);
 	ExecutorEnd(queryDesc);
 	/* FreeQueryDesc is done by the caller */
 

src/backend/tcop/pquery.c

@@ -181,11 +181,6 @@ ProcessQuery(PlannedStmt *plan,
 								GetActiveSnapshot(), InvalidSnapshot,
 								dest, params, 0);
 
-	/*
-	 * Set up to collect AFTER triggers
-	 */
-	AfterTriggerBeginQuery();
-
 	/*
 	 * Call ExecutorStart to prepare the plan for execution
 	 */
@@ -231,17 +226,15 @@ ProcessQuery(PlannedStmt *plan,
 		}
 	}
 
-	/* Now take care of any queued AFTER triggers */
-	AfterTriggerEndQuery(queryDesc->estate);
-
-	PopActiveSnapshot();
-
 	/*
 	 * Now, we close down all the scans and free allocated resources.
 	 */
+	ExecutorFinish(queryDesc);
 	ExecutorEnd(queryDesc);
 
 	FreeQueryDesc(queryDesc);
+
+	PopActiveSnapshot();
 }
 
 /*
@@ -530,13 +523,6 @@ PortalStart(Portal portal, ParamListInfo params, Snapshot snapshot)
 											params,
 											0);
 
-				/*
-				 * We do *not* call AfterTriggerBeginQuery() here.	We assume
-				 * that a SELECT cannot queue any triggers.  It would be messy
-				 * to support triggers since the execution of the portal may
-				 * be interleaved with other queries.
-				 */
-
 				/*
 				 * If it's a scrollable cursor, executor needs to support
 				 * REWIND and backwards scan.

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)
+		/* 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;
+			continue;
 		}
-
-/*
- * 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)
+		else
 		{
-		Portal		portal = hentry->portal;
+			/* Zap all non-holdable portals */
+			PortalDrop(portal, true);
 
-		/*
-		 * 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;
+			/* Report we changed state */
+			result = 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");
-
-		/*
-		 * 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);
 	}

src/include/executor/executor.h

@@ -44,11 +44,17 @@
  *
  * MARK indicates that the plan node must support Mark/Restore calls.
  * When this is not passed, no Mark/Restore will occur.
+ *
+ * SKIP_TRIGGERS tells ExecutorStart/ExecutorFinish to skip calling
+ * AfterTriggerBeginQuery/AfterTriggerEndQuery.  This does not necessarily
+ * mean that the plan can't queue any AFTER triggers; just that the caller
+ * is responsible for there being a trigger context for them to be queued in.
  */
 #define EXEC_FLAG_EXPLAIN_ONLY	0x0001	/* EXPLAIN, no ANALYZE */
 #define EXEC_FLAG_REWIND		0x0002	/* need efficient rescan */
 #define EXEC_FLAG_BACKWARD		0x0004	/* need backward scan */
 #define EXEC_FLAG_MARK			0x0008	/* need mark/restore */
+#define EXEC_FLAG_SKIP_TRIGGERS	0x0010	/* skip AfterTrigger calls */
 
 
 /*
@@ -70,6 +76,10 @@ typedef void (*ExecutorRun_hook_type) (QueryDesc *queryDesc,
 												   long count);
 extern PGDLLIMPORT ExecutorRun_hook_type ExecutorRun_hook;
 
+/* Hook for plugins to get control in ExecutorFinish() */
+typedef void (*ExecutorFinish_hook_type) (QueryDesc *queryDesc);
+extern PGDLLIMPORT ExecutorFinish_hook_type ExecutorFinish_hook;
+
 /* Hook for plugins to get control in ExecutorEnd() */
 typedef void (*ExecutorEnd_hook_type) (QueryDesc *queryDesc);
 extern PGDLLIMPORT ExecutorEnd_hook_type ExecutorEnd_hook;
@@ -159,6 +169,8 @@ extern void ExecutorRun(QueryDesc *queryDesc,
 			ScanDirection direction, long count);
 extern void standard_ExecutorRun(QueryDesc *queryDesc,
 					 ScanDirection direction, long count);
+extern void ExecutorFinish(QueryDesc *queryDesc);
+extern void standard_ExecutorFinish(QueryDesc *queryDesc);
 extern void ExecutorEnd(QueryDesc *queryDesc);
 extern void standard_ExecutorEnd(QueryDesc *queryDesc);
 extern void ExecutorRewind(QueryDesc *queryDesc);

src/include/nodes/execnodes.h

@@ -370,9 +370,11 @@ typedef struct EState
 	uint32		es_processed;	/* # of tuples processed */
 	Oid			es_lastoid;		/* last oid processed (by INSERT) */
 
+	int			es_top_eflags;	/* eflags passed to ExecutorStart */
 	int			es_instrument;	/* OR of InstrumentOption flags */
 	bool		es_select_into; /* true if doing SELECT INTO */
 	bool		es_into_oids;	/* true to generate OIDs in SELECT INTO */
+	bool		es_finished;	/* true when ExecutorFinish is done */
 
 	List	   *es_exprcontexts;	/* List of ExprContexts within EState */
 

src/include/utils/portal.h

@@ -192,9 +192,7 @@ typedef struct PortalData
 
 /* Prototypes for functions in utils/mmgr/portalmem.c */
 extern void EnablePortalManager(void);
-extern bool CommitHoldablePortals(void);
-extern bool PrepareHoldablePortals(void);
-extern void AtCommit_Portals(void);
+extern bool PreCommit_Portals(bool isPrepare);
 extern void AtAbort_Portals(void);
 extern void AtCleanup_Portals(void);
 extern void AtSubCommit_Portals(SubTransactionId mySubid,

src/test/regress/expected/with.out

@@ -1678,16 +1678,14 @@ WITH t AS (
         (33)
     RETURNING *
 )
-SELECT * FROM t;
+SELECT * FROM t LIMIT 1;
 NOTICE:  y_trigger: a = 31
 NOTICE:  y_trigger: a = 32
 NOTICE:  y_trigger: a = 33
  a  
 ----
  31
- 32
- 33
-(3 rows)
+(1 row)
 
 SELECT * FROM y;
  a  

src/test/regress/sql/with.sql

@@ -707,7 +707,7 @@ WITH t AS (
         (33)
     RETURNING *
 )
-SELECT * FROM t;
+SELECT * FROM t LIMIT 1;
 
 SELECT * FROM y;