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

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
index 35a37d250151ec7c0dbdfa70fd29aa52ae9d446a..86547f0499f6c79da149d12573243637d2823574 100644 (file)
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -85,6 +85,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
 {
@@ -2297,8 +2304,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);
@@ -4801,6 +4808,10 @@ exec_stmt_commit(PLpgSQL_execstate *estate, PLpgSQL_stmt_commit *stmt)
    SPI_commit();
    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);
 
@@ -4818,6 +4829,10 @@ exec_stmt_rollback(PLpgSQL_execstate *estate, PLpgSQL_stmt_rollback *stmt)
    SPI_rollback();
    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);
 
@@ -8082,8 +8097,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)
    {
@@ -8155,7 +8175,9 @@ 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;
 
@@ -8163,7 +8185,8 @@ plpgsql_xact_cb(XactEvent event, void *arg)
            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
index 61452d9f7fd6ddda7d7f1b26aebd7fa86c8241d7..a79524c7a30ede84e050d7ddab0840a587a39f9f 100644 (file)
--- a/src/pl/plpgsql/src/pl_handler.c
+++ b/src/pl/plpgsql/src/pl_handler.c
@@ -326,7 +326,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 */