Re: BUG #16213: segfault when running a query - Mailing list pgsql-hackers

I wrote:
> PG Bug reporting form <noreply@postgresql.org> writes:
>> The above query produces an error in the server log:
>> LOG:  server process (PID 108) was terminated by signal 11: Segmentation
>> fault

> The direct cause of the crash is that by the time we get to ExecutorEnd,
> there are dangling pointers in the es_tupleTable list.

After further reflection, I'm totally dissatisfied with the quick-hack
patch I posted last night.  I think that what this example demonstrates
is that my fix (in commit 9b63c13f0) for bug #14924 in fact does not
work: the subPlan list is not the only way in which a SubPlan connects
up to the outer plan level.  I have no faith that the es_tupleTable list
is the only other way, either, or that we won't create more in future.

I think what we have to do here is revert 9b63c13f0, going back to
the previous policy of passing down parent = NULL to the transient
subexpressions, so that there is a strong guarantee that there aren't
any unwanted connections between short-lived and longer-lived state.
And then we need some other solution for making SubPlans in VALUES
lists work.  The best bet seems to be what I speculated about in that
commit message: initialize the expressions for VALUES rows that contain
SubPlans normally at executor startup, and use the trick with
short-lived expression state only for VALUES rows that don't contain any
SubPlans.  I think that the case we're worried about with long VALUES
lists is not likely to involve any SubPlans, so that this shouldn't be
too awful for memory consumption.

Another benefit of doing it like this is that SubPlans in the VALUES
lists are reported normally by EXPLAIN, while the previous hack caused
them to be missing from the output.

Objections, better ideas?

            regards, tom lane

diff --git a/src/backend/executor/nodeValuesscan.c b/src/backend/executor/nodeValuesscan.c
index 8d916a0..8866121 100644
--- a/src/backend/executor/nodeValuesscan.c
+++ b/src/backend/executor/nodeValuesscan.c
@@ -26,6 +26,7 @@
 #include "executor/executor.h"
 #include "executor/nodeValuesscan.h"
 #include "jit/jit.h"
+#include "optimizer/clauses.h"
 #include "utils/expandeddatum.h"


@@ -50,7 +51,7 @@ ValuesNext(ValuesScanState *node)
     EState       *estate;
     ExprContext *econtext;
     ScanDirection direction;
-    List       *exprlist;
+    int            curr_idx;

     /*
      * get information from the estate and scan state
@@ -67,19 +68,11 @@ ValuesNext(ValuesScanState *node)
     {
         if (node->curr_idx < node->array_len)
             node->curr_idx++;
-        if (node->curr_idx < node->array_len)
-            exprlist = node->exprlists[node->curr_idx];
-        else
-            exprlist = NIL;
     }
     else
     {
         if (node->curr_idx >= 0)
             node->curr_idx--;
-        if (node->curr_idx >= 0)
-            exprlist = node->exprlists[node->curr_idx];
-        else
-            exprlist = NIL;
     }

     /*
@@ -90,16 +83,16 @@ ValuesNext(ValuesScanState *node)
      */
     ExecClearTuple(slot);

-    if (exprlist)
+    curr_idx = node->curr_idx;
+    if (curr_idx >= 0 && curr_idx < node->array_len)
     {
+        List       *exprlist = node->exprlists[curr_idx];
+        List       *exprstatelist = node->exprstatelists[curr_idx];
         MemoryContext oldContext;
-        List       *oldsubplans;
-        List       *exprstatelist;
         Datum       *values;
         bool       *isnull;
         ListCell   *lc;
         int            resind;
-        int            saved_jit_flags;

         /*
          * Get rid of any prior cycle's leftovers.  We use ReScanExprContext
@@ -109,38 +102,32 @@ ValuesNext(ValuesScanState *node)
         ReScanExprContext(econtext);

         /*
-         * Build the expression eval state in the econtext's per-tuple memory.
-         * This is a tad unusual, but we want to delete the eval state again
-         * when we move to the next row, to avoid growth of memory
-         * requirements over a long values list.
+         * Do per-VALUES-row work in the per-tuple context.
          */
         oldContext = MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);

         /*
-         * The expressions might contain SubPlans (this is currently only
-         * possible if there's a sub-select containing a LATERAL reference,
-         * otherwise sub-selects in a VALUES list should be InitPlans). Those
-         * subplans will want to hook themselves into our subPlan list, which
-         * would result in a corrupted list after we delete the eval state. We
-         * can work around this by saving and restoring the subPlan list.
-         * (There's no need for the functionality that would be enabled by
-         * having the list entries, since the SubPlans aren't going to be
-         * re-executed anyway.)
-         */
-        oldsubplans = node->ss.ps.subPlan;
-        node->ss.ps.subPlan = NIL;
-
-        /*
-         * As the expressions are only ever used once, disable JIT for them.
-         * This is worthwhile because it's common to insert significant
-         * amounts of data via VALUES().
+         * Unless we already made the expression eval state for this row,
+         * build it in the econtext's per-tuple memory.  This is a tad
+         * unusual, but we want to delete the eval state again when we move to
+         * the next row, to avoid growth of memory requirements over a long
+         * values list.  For rows in which that won't work, we already built
+         * the eval state at plan startup.
          */
-        saved_jit_flags = econtext->ecxt_estate->es_jit_flags;
-        econtext->ecxt_estate->es_jit_flags = PGJIT_NONE;
-        exprstatelist = ExecInitExprList(exprlist, &node->ss.ps);
-        econtext->ecxt_estate->es_jit_flags = saved_jit_flags;
-
-        node->ss.ps.subPlan = oldsubplans;
+        if (exprstatelist == NIL)
+        {
+            /*
+             * Pass parent as NULL, not my plan node, because we don't want
+             * anything in this transient state linking into permanent state.
+             * The only expression type that might wish to do so is a SubPlan,
+             * and we already checked that there aren't any.
+             *
+             * Note that passing parent = NULL also disables JIT compilation
+             * of the expressions, which is a win, because they're only going
+             * to be used once under normal circumstances.
+             */
+            exprstatelist = ExecInitExprList(exprlist, NULL);
+        }

         /* parser should have checked all sublists are the same length */
         Assert(list_length(exprstatelist) == slot->tts_tupleDescriptor->natts);
@@ -281,13 +268,52 @@ ExecInitValuesScan(ValuesScan *node, EState *estate, int eflags)
     scanstate->curr_idx = -1;
     scanstate->array_len = list_length(node->values_lists);

-    /* convert list of sublists into array of sublists for easy addressing */
+    /*
+     * Convert the list of expression sublists into an array for easier
+     * addressing at runtime.  Also, detect whether any sublists contain
+     * SubPlans; for just those sublists, go ahead and do expression
+     * initialization.  (This avoids problems with SubPlans wanting to connect
+     * themselves up to the outer plan tree.  Notably, EXPLAIN won't see the
+     * subplans otherwise; also we will have troubles with dangling pointers
+     * and/or leaked resources if we try to handle SubPlans the same as
+     * simpler expressions.)
+     */
     scanstate->exprlists = (List **)
         palloc(scanstate->array_len * sizeof(List *));
+    scanstate->exprstatelists = (List **)
+        palloc0(scanstate->array_len * sizeof(List *));
     i = 0;
     foreach(vtl, node->values_lists)
     {
-        scanstate->exprlists[i++] = (List *) lfirst(vtl);
+        List       *exprs = castNode(List, lfirst(vtl));
+
+        scanstate->exprlists[i] = exprs;
+
+        /*
+         * We can avoid the cost of a contain_subplans() scan in the simple
+         * case where there are no SubPlans anywhere.
+         */
+        if (estate->es_subplanstates &&
+            contain_subplans((Node *) exprs))
+        {
+            int            saved_jit_flags;
+
+            /*
+             * As these expressions are only used once, disable JIT for them.
+             * This is worthwhile because it's common to insert significant
+             * amounts of data via VALUES().  Note that this doesn't prevent
+             * use of JIT *within* a subplan, since that's initialized
+             * separately; this just affects the upper-level subexpressions.
+             */
+            saved_jit_flags = estate->es_jit_flags;
+            estate->es_jit_flags = PGJIT_NONE;
+
+            scanstate->exprstatelists[i] = ExecInitExprList(exprs,
+                                                            &scanstate->ss.ps);
+
+            estate->es_jit_flags = saved_jit_flags;
+        }
+        i++;
     }

     return scanstate;
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index eaea1f3..1f6f5bb 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -1670,7 +1670,8 @@ typedef struct FunctionScanState
  *
  *        rowcontext            per-expression-list context
  *        exprlists            array of expression lists being evaluated
- *        array_len            size of array
+ *        exprstatelists        array of expression state lists, for SubPlans only
+ *        array_len            size of above arrays
  *        curr_idx            current array index (0-based)
  *
  *    Note: ss.ps.ps_ExprContext is used to evaluate any qual or projection
@@ -1678,6 +1679,12 @@ typedef struct FunctionScanState
  *    rowcontext, in which to build the executor expression state for each
  *    Values sublist.  Resetting this context lets us get rid of expression
  *    state for each row, avoiding major memory leakage over a long values list.
+ *    However, that doesn't work for sublists containing SubPlans, because a
+ *    SubPlan has to be connected up to the outer plan tree to work properly.
+ *    Therefore, for only those sublists containing SubPlans, we do expression
+ *    state construction at executor start, and store those pointers in
+ *    exprstatelists[].  NULL entries in that array correspond to simple
+ *    subexpressions that are handled as described above.
  * ----------------
  */
 typedef struct ValuesScanState
@@ -1685,6 +1692,7 @@ typedef struct ValuesScanState
     ScanState    ss;                /* its first field is NodeTag */
     ExprContext *rowcontext;
     List      **exprlists;
+    List      **exprstatelists;
     int            array_len;
     int            curr_idx;
 } ValuesScanState;
diff --git a/src/test/regress/expected/subselect.out b/src/test/regress/expected/subselect.out
index 5476861..71a677b 100644
--- a/src/test/regress/expected/subselect.out
+++ b/src/test/regress/expected/subselect.out
@@ -926,6 +926,33 @@ where s.i < 10 and (select val.x) < 110;
   10
 (17 rows)

+-- another variant of that (bug #16213)
+explain (verbose, costs off)
+select * from
+(values
+  (3 not in (select * from (values (1), (2)) ss1)),
+  (false)
+) ss;
+               QUERY PLAN
+----------------------------------------
+ Values Scan on "*VALUES*"
+   Output: "*VALUES*".column1
+   SubPlan 1
+     ->  Values Scan on "*VALUES*_1"
+           Output: "*VALUES*_1".column1
+(5 rows)
+
+select * from
+(values
+  (3 not in (select * from (values (1), (2)) ss1)),
+  (false)
+) ss;
+ column1
+---------
+ t
+ f
+(2 rows)
+
 --
 -- Check sane behavior with nested IN SubLinks
 --
diff --git a/src/test/regress/sql/subselect.sql b/src/test/regress/sql/subselect.sql
index 3e119ce..bd8d2f6 100644
--- a/src/test/regress/sql/subselect.sql
+++ b/src/test/regress/sql/subselect.sql
@@ -518,6 +518,20 @@ select val.x
   ) as val(x)
 where s.i < 10 and (select val.x) < 110;

+-- another variant of that (bug #16213)
+explain (verbose, costs off)
+select * from
+(values
+  (3 not in (select * from (values (1), (2)) ss1)),
+  (false)
+) ss;
+
+select * from
+(values
+  (3 not in (select * from (values (1), (2)) ss1)),
+  (false)
+) ss;
+
 --
 -- Check sane behavior with nested IN SubLinks
 --