Re: BUG #14924: Subquery in VALUES inside recursive CTE - Mailing list pgsql-bugs

I wrote:
> christianmduta@gmail.com writes:
>> When working with recursive CTEs, I had the following happen:

> Thanks for the report!  Seems not to be specific to CTEs:

I traced this problem to this old hack in nodeValuesscan.c:

        /*
         * Pass NULL, not my plan node, because we don't want anything in this
         * transient state linking into permanent state.  The only possibility
         * is a SubPlan, and there shouldn't be any (any subselects in the
         * VALUES list should be InitPlans).
         */
        exprstatelist = ExecInitExprList(exprlist, NULL);

That was okay when written (in commit 0dfb595d7), but with the
introduction of LATERAL, it's possible for a VALUES list to contain
a sub-select that converts to a SubPlan rather than an InitPlan.

I don't particularly want to abandon the hack of discarding expression
eval state after each VALUES row; without that, a very large VALUES
is going to eat a lot of memory.  The next best answer seems to be
to go ahead and pass the Values node as parent to the expressions,
but to prevent the expressions from actually hooking into its subPlan
list, as per the attached patch.

The main problem with this is that in future, child expressions might
possibly try to attach to fields of the parent plan node other than
the subPlan list.  We could extend this hack to deal with that, but
I can't think of any forcing function to ensure that we'd notice the
need to.  Still, that's a pretty hypothetical problem, and we do have
a live bug here.

            regards, tom lane

diff --git a/src/backend/executor/nodeValuesscan.c b/src/backend/executor/nodeValuesscan.c
index 1a72bfe..a8aa01f 100644
*** a/src/backend/executor/nodeValuesscan.c
--- b/src/backend/executor/nodeValuesscan.c
*************** ValuesNext(ValuesScanState *node)
*** 92,97 ****
--- 92,98 ----
      if (exprlist)
      {
          MemoryContext oldContext;
+         List       *oldsubplans;
          List       *exprstatelist;
          Datum       *values;
          bool       *isnull;
*************** ValuesNext(ValuesScanState *node)
*** 114,125 ****
          oldContext = MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);

          /*
!          * Pass NULL, not my plan node, because we don't want anything in this
!          * transient state linking into permanent state.  The only possibility
!          * is a SubPlan, and there shouldn't be any (any subselects in the
!          * VALUES list should be InitPlans).
           */
!         exprstatelist = ExecInitExprList(exprlist, NULL);

          /* parser should have checked all sublists are the same length */
          Assert(list_length(exprstatelist) == slot->tts_tupleDescriptor->natts);
--- 115,136 ----
          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;
!
!         exprstatelist = ExecInitExprList(exprlist, &node->ss.ps);
!
!         node->ss.ps.subPlan = oldsubplans;

          /* parser should have checked all sublists are the same length */
          Assert(list_length(exprstatelist) == slot->tts_tupleDescriptor->natts);
diff --git a/src/test/regress/expected/subselect.out b/src/test/regress/expected/subselect.out
index 992d29b..4b89385 100644
*** a/src/test/regress/expected/subselect.out
--- b/src/test/regress/expected/subselect.out
*************** select exists(select * from nocolumns);
*** 841,846 ****
--- 841,876 ----
  (1 row)

  --
+ -- Check behavior with a SubPlan in VALUES (bug #14924)
+ --
+ select val.x
+   from generate_series(1,10) as s(i),
+   lateral (
+     values ((select s.i + 1)), (s.i + 101)
+   ) as val(x)
+ where s.i < 10 and (select val.x) < 110;
+   x
+ -----
+    2
+  102
+    3
+  103
+    4
+  104
+    5
+  105
+    6
+  106
+    7
+  107
+    8
+  108
+    9
+  109
+   10
+ (17 rows)
+
+ --
  -- Check sane behavior with nested IN SubLinks
  --
  explain (verbose, costs off)
diff --git a/src/test/regress/sql/subselect.sql b/src/test/regress/sql/subselect.sql
index 0d2dd2f..0100367 100644
*** a/src/test/regress/sql/subselect.sql
--- b/src/test/regress/sql/subselect.sql
*************** create temp table nocolumns();
*** 473,478 ****
--- 473,488 ----
  select exists(select * from nocolumns);

  --
+ -- Check behavior with a SubPlan in VALUES (bug #14924)
+ --
+ select val.x
+   from generate_series(1,10) as s(i),
+   lateral (
+     values ((select s.i + 1)), (s.i + 101)
+   ) as val(x)
+ where s.i < 10 and (select val.x) < 110;
+
+ --
  -- Check sane behavior with nested IN SubLinks
  --
  explain (verbose, costs off)