Ensure that plpgsql cleans up cleanly during parallel-worker exit.

tglsfdc · tglsfdc · commit a54a87320782 · 2020-03-26T18:06:55.000-04:00
plpgsql_xact_cb ought to treat events XACT_EVENT_PARALLEL_COMMIT and XACT_EVENT_PARALLEL_ABORT like XACT_EVENT_COMMIT and XACT_EVENT_ABORT respectively, since its goal is to do process-local cleanup. This oversight caused plpgsql's end-of-transaction cleanup to not get done in parallel workers. Since a parallel worker will exit just after the transaction cleanup, the effects of this are limited. I couldn't find any case in the core code with user-visible effects, but perhaps there are some in extensions. In any case it's wrong, so let's fix it before it bites us not after. In passing, add some comments around the handling of expression evaluation resources in DO blocks. There's no live bug there, but it's quite unobvious what's happening; at least I thought so. This isn't related to the other issue, except that I found both things while poking at expression-evaluation performance. Back-patch the plpgsql_xact_cb fix to 9.5 where those event types were introduced, and the DO-block commentary to v11 where DO blocks gained the ability to issue COMMIT/ROLLBACK. Discussion: https://postgr.es/m/10353.1585247879@sss.pgh.pa.us
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
@@ -84,6 +84,13 @@ typedef struct
  * has its own simple-expression EState, which is cleaned up at exit from
  * plpgsql_inline_handler().  DO blocks still use the simple_econtext_stack,
  * though, so that subxact abort cleanup does the right thing.
+ *
+ * (However, if a DO block executes COMMIT or ROLLBACK, then exec_stmt_commit
+ * or exec_stmt_rollback will unlink it from the DO's simple-expression EState
+ * and create a new shared EState that will be used thenceforth.  The original
+ * EState will be cleaned up when we get back to plpgsql_inline_handler.  This
+ * is a bit ugly, but it isn't worth doing better, since scenarios like this
+ * can't result in indefinite accumulation of state trees.)
  */
 typedef struct SimpleEcontextStackEntry
 {
@@ -2317,8 +2324,8 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
 	else
 	{
 		/*
-		 * If we are in a new transaction after the call, we need to reset
-		 * some internal state.
+		 * If we are in a new transaction after the call, we need to build new
+		 * simple-expression infrastructure.
 		 */
 		estate->simple_eval_estate = NULL;
 		plpgsql_create_econtext(estate);
@@ -4827,6 +4834,10 @@ exec_stmt_commit(PLpgSQL_execstate *estate, PLpgSQL_stmt_commit *stmt)
 		SPI_start_transaction();
 	}
 
+	/*
+	 * We need to build new simple-expression infrastructure, since the old
+	 * data structures are gone.
+	 */
 	estate->simple_eval_estate = NULL;
 	plpgsql_create_econtext(estate);
 
@@ -4849,6 +4860,10 @@ exec_stmt_rollback(PLpgSQL_execstate *estate, PLpgSQL_stmt_rollback *stmt)
 		SPI_start_transaction();
 	}
 
+	/*
+	 * We need to build new simple-expression infrastructure, since the old
+	 * data structures are gone.
+	 */
 	estate->simple_eval_estate = NULL;
 	plpgsql_create_econtext(estate);
 
@@ -8201,8 +8216,13 @@ plpgsql_create_econtext(PLpgSQL_execstate *estate)
 	 * one already in the current transaction.  The EState is made a child of
 	 * TopTransactionContext so it will have the right lifespan.
 	 *
-	 * Note that this path is never taken when executing a DO block; the
-	 * required EState was already made by plpgsql_inline_handler.
+	 * Note that this path is never taken when beginning a DO block; the
+	 * required EState was already made by plpgsql_inline_handler.  However,
+	 * if the DO block executes COMMIT or ROLLBACK, then we'll come here and
+	 * make a shared EState to use for the rest of the DO block.  That's OK;
+	 * see the comments for shared_simple_eval_estate.  (Note also that a DO
+	 * block will continue to use its private cast hash table for the rest of
+	 * the block.  That's okay for now, but it might cause problems someday.)
 	 */
 	if (estate->simple_eval_estate == NULL)
 	{
@@ -8274,15 +8294,18 @@ plpgsql_xact_cb(XactEvent event, void *arg)
 	 * expect the regular abort recovery procedures to release everything of
 	 * interest.
 	 */
-	if (event == XACT_EVENT_COMMIT || event == XACT_EVENT_PREPARE)
+	if (event == XACT_EVENT_COMMIT ||
+		event == XACT_EVENT_PARALLEL_COMMIT ||
+		event == XACT_EVENT_PREPARE)
 	{
 		simple_econtext_stack = NULL;
 
 		if (shared_simple_eval_estate)
 			FreeExecutorState(shared_simple_eval_estate);
 		shared_simple_eval_estate = NULL;
 	}
-	else if (event == XACT_EVENT_ABORT)
+	else if (event == XACT_EVENT_ABORT ||
+			 event == XACT_EVENT_PARALLEL_ABORT)
 	{
 		simple_econtext_stack = NULL;
 		shared_simple_eval_estate = NULL;
diff --git a/src/pl/plpgsql/src/pl_handler.c b/src/pl/plpgsql/src/pl_handler.c
@@ -330,7 +330,14 @@ plpgsql_inline_handler(PG_FUNCTION_ARGS)
 	flinfo.fn_oid = InvalidOid;
 	flinfo.fn_mcxt = CurrentMemoryContext;
 
-	/* Create a private EState for simple-expression execution */
+	/*
+	 * Create a private EState for simple-expression execution.  Notice that
+	 * this is NOT tied to transaction-level resources; it must survive any
+	 * COMMIT/ROLLBACK the DO block executes, since we will unconditionally
+	 * try to clean it up below.  (Hence, be wary of adding anything that
+	 * could fail between here and the PG_TRY block.)  See the comments for
+	 * shared_simple_eval_estate.
+	 */
 	simple_eval_estate = CreateExecutorState();
 
 	/* And run the function */
