Re: BUG #18077: PostgreSQL server subprocess crashed by a SELECT statement with WITH clause

Richard Guo <guofenglinux@gmail.com> writes:
> On Wed, Aug 30, 2023 at 7:42 PM Richard Guo <guofenglinux@gmail.com> wrote:
>> When we expand Var 'c1' from func(c1), we figure out that it comes from
>> subquery 's'.  When we recurse into subquery 's', we just build an
>> additional level of ParseState atop the current ParseState, which seems
>> not correct.  Shouldn't we climb up by the nesting depth first before we
>> build the additional level of ParseState?  Something like
>> ...

> Here is the patch.

Yeah, I think your diagnosis is correct.  The existing regression tests
reach this code path, but not with netlevelsup different from zero.
I noted from the code coverage report that the same is true of the
nearby RTE_CTE code path: that does have a loop to crawl up the pstate
stack, but it isn't getting iterated.  The attached improved patch
extends the test case so it also covers that.

I would have liked to also cover the RTE_JOIN case, which the code
coverage report shows to be completely untested.  However, I failed
to make a test case that reached that.  I think it might be a lot
harder to reach in the wake of 9ce77d75c, which narrowed the cases
in which join alias Vars are created.

I also spent a little bit of effort on improving the comments and
removing cosmetic differences between the SUBQUERY and CTE cases.

            regards, tom lane

diff --git a/src/backend/parser/parse_target.c b/src/backend/parser/parse_target.c
index 57247de363..3bc62ac3ba 100644
--- a/src/backend/parser/parse_target.c
+++ b/src/backend/parser/parse_target.c
@@ -1499,7 +1499,8 @@ ExpandRowReference(ParseState *pstate, Node *expr,
  * drill down to find the ultimate defining expression and attempt to infer
  * the tupdesc from it.  We ereport if we can't determine the tupdesc.
  *
- * levelsup is an extra offset to interpret the Var's varlevelsup correctly.
+ * levelsup is an extra offset to interpret the Var's varlevelsup correctly
+ * when recursing.  Outside callers should pass zero.
  */
 TupleDesc
 expandRecordVariable(ParseState *pstate, Var *var, int levelsup)
@@ -1587,10 +1588,17 @@ expandRecordVariable(ParseState *pstate, Var *var, int levelsup)
                     /*
                      * Recurse into the sub-select to see what its Var refers
                      * to.  We have to build an additional level of ParseState
-                     * to keep in step with varlevelsup in the subselect.
+                     * to keep in step with varlevelsup in the subselect;
+                     * furthermore, the subquery RTE might be from an outer
+                     * query level, in which case the ParseState for the
+                     * subselect must have that outer level as parent.
                      */
                     ParseState    mypstate = {0};
+                    Index        levelsup;

+                    /* this loop must work, since GetRTEByRangeTablePosn did */
+                    for (levelsup = 0; levelsup < netlevelsup; levelsup++)
+                        pstate = pstate->parentParseState;
                     mypstate.parentParseState = pstate;
                     mypstate.p_rtable = rte->subquery->rtable;
                     /* don't bother filling the rest of the fake pstate */
@@ -1641,12 +1649,11 @@ expandRecordVariable(ParseState *pstate, Var *var, int levelsup)
                      * Recurse into the CTE to see what its Var refers to. We
                      * have to build an additional level of ParseState to keep
                      * in step with varlevelsup in the CTE; furthermore it
-                     * could be an outer CTE.
+                     * could be an outer CTE (compare SUBQUERY case above).
                      */
-                    ParseState    mypstate;
+                    ParseState    mypstate = {0};
                     Index        levelsup;

-                    MemSet(&mypstate, 0, sizeof(mypstate));
                     /* this loop must work, since GetCTEForRTE did */
                     for (levelsup = 0;
                          levelsup < rte->ctelevelsup + netlevelsup;
diff --git a/src/test/regress/expected/rowtypes.out b/src/test/regress/expected/rowtypes.out
index 981ee0811a..d735a95bdc 100644
--- a/src/test/regress/expected/rowtypes.out
+++ b/src/test/regress/expected/rowtypes.out
@@ -1240,6 +1240,42 @@ select r, r is null as isnull, r is not null as isnotnull from r;
  (,)         | t      | f
 (6 rows)

+--
+-- Check parsing of indirect references to composite values (bug #18077)
+--
+explain (verbose, costs off)
+with cte(c) as materialized (select row(1, 2)),
+     cte2(c) as (select * from cte)
+select * from cte2 as t
+where (select * from (select c as c1) s
+       where (select (c1).f1 > 0)) is not null;
+                 QUERY PLAN
+--------------------------------------------
+ CTE Scan on cte
+   Output: cte.c
+   Filter: ((SubPlan 3) IS NOT NULL)
+   CTE cte
+     ->  Result
+           Output: '(1,2)'::record
+   SubPlan 3
+     ->  Result
+           Output: cte.c
+           One-Time Filter: $2
+           InitPlan 2 (returns $2)
+             ->  Result
+                   Output: ((cte.c).f1 > 0)
+(13 rows)
+
+with cte(c) as materialized (select row(1, 2)),
+     cte2(c) as (select * from cte)
+select * from cte2 as t
+where (select * from (select c as c1) s
+       where (select (c1).f1 > 0)) is not null;
+   c
+-------
+ (1,2)
+(1 row)
+
 --
 -- Tests for component access / FieldSelect
 --
diff --git a/src/test/regress/sql/rowtypes.sql b/src/test/regress/sql/rowtypes.sql
index 565e6249d5..11bfcdee3a 100644
--- a/src/test/regress/sql/rowtypes.sql
+++ b/src/test/regress/sql/rowtypes.sql
@@ -494,6 +494,21 @@ with r(a,b) as materialized
           (null,row(1,2)), (null,row(null,null)), (null,null) )
 select r, r is null as isnull, r is not null as isnotnull from r;

+--
+-- Check parsing of indirect references to composite values (bug #18077)
+--
+explain (verbose, costs off)
+with cte(c) as materialized (select row(1, 2)),
+     cte2(c) as (select * from cte)
+select * from cte2 as t
+where (select * from (select c as c1) s
+       where (select (c1).f1 > 0)) is not null;
+
+with cte(c) as materialized (select row(1, 2)),
+     cte2(c) as (select * from cte)
+select * from cte2 as t
+where (select * from (select c as c1) s
+       where (select (c1).f1 > 0)) is not null;

 --
 -- Tests for component access / FieldSelect