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

Richard Guo <guofenglinux@gmail.com> writes:
> Here is v3 patch which is v2 + fix for this issue.

This seems not quite right yet: we need to pass the correct
parent-namespaces list to set_deparse_for_query, else set_rtable_names
might make unexpected choices.  I think the net effect of what you
have would only be to make generated table-alias names more unique
than necessary (i.e, avoiding collisions with names that are not
really in scope), but still this could be confusingly inconsistent.
So we should do more like the attached.

I set about back-patching this, and discovered that your deparse
test case exposes additional problems in the back branches.  We
get "record type has not been registered" failures in deparsing,
or even in trying to parse the view to begin with, unless we
back-patch d57534740 into pre-v14 branches and also 8b7a0f1d1
into pre-v13 branches.  At the time I'd thought d57534740's bug
could not be exposed without SEARCH BREADTH FIRST, but that was
clearly a failure of imagination.  As for 8b7a0f1d1, I'd judged
it too narrow of a corner case to be worth back-patching, and
maybe it still is: I don't think it's reachable without attempting
to fetch a ".fN" field out of an anonymous record type.  Still,
we do document that ".fN" is what the generated names are, so
it seems like people ought to be able to use them.  On balance,
therefore, I'm inclined to back-patch both of those.

Thoughts?

            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/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 97b0ef22ac..68f301484e 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -7820,22 +7820,28 @@ get_name_for_var_field(Var *var, int fieldno,
                          * Recurse into the sub-select to see what its Var
                          * refers to. We have to build an additional level of
                          * namespace to keep in step with varlevelsup in the
-                         * subselect.
+                         * subselect; furthermore, the subquery RTE might be
+                         * from an outer query level, in which case the
+                         * namespace for the subselect must have that outer
+                         * level as parent namespace.
                          */
+                        List       *save_nslist = context->namespaces;
+                        List       *parent_namespaces;
                         deparse_namespace mydpns;
                         const char *result;

+                        parent_namespaces = list_copy_tail(context->namespaces,
+                                                           netlevelsup);
+
                         set_deparse_for_query(&mydpns, rte->subquery,
-                                              context->namespaces);
+                                              parent_namespaces);

-                        context->namespaces = lcons(&mydpns,
-                                                    context->namespaces);
+                        context->namespaces = lcons(&mydpns, parent_namespaces);

                         result = get_name_for_var_field((Var *) expr, fieldno,
                                                         0, context);

-                        context->namespaces =
-                            list_delete_first(context->namespaces);
+                        context->namespaces = save_nslist;

                         return result;
                     }
@@ -7927,7 +7933,7 @@ get_name_for_var_field(Var *var, int fieldno,
                                                         attnum);

                     if (ste == NULL || ste->resjunk)
-                        elog(ERROR, "subquery %s does not have attribute %d",
+                        elog(ERROR, "CTE %s does not have attribute %d",
                              rte->eref->aliasname, attnum);
                     expr = (Node *) ste->expr;
                     if (IsA(expr, Var))
@@ -7935,21 +7941,22 @@ get_name_for_var_field(Var *var, int fieldno,
                         /*
                          * Recurse into the CTE to see what its Var refers to.
                          * We have to build an additional level of namespace
-                         * to keep in step with varlevelsup in the CTE.
-                         * Furthermore it could be an outer CTE, so we may
-                         * have to delete some levels of namespace.
+                         * to keep in step with varlevelsup in the CTE;
+                         * furthermore it could be an outer CTE (compare
+                         * SUBQUERY case above).
                          */
                         List       *save_nslist = context->namespaces;
-                        List       *new_nslist;
+                        List       *parent_namespaces;
                         deparse_namespace mydpns;
                         const char *result;

+                        parent_namespaces = list_copy_tail(context->namespaces,
+                                                           ctelevelsup);
+
                         set_deparse_for_query(&mydpns, ctequery,
-                                              context->namespaces);
+                                              parent_namespaces);

-                        new_nslist = list_copy_tail(context->namespaces,
-                                                    ctelevelsup);
-                        context->namespaces = lcons(&mydpns, new_nslist);
+                        context->namespaces = lcons(&mydpns, parent_namespaces);

                         result = get_name_for_var_field((Var *) expr, fieldno,
                                                         0, context);
diff --git a/src/test/regress/expected/rowtypes.out b/src/test/regress/expected/rowtypes.out
index 981ee0811a..8f3c153bac 100644
--- a/src/test/regress/expected/rowtypes.out
+++ b/src/test/regress/expected/rowtypes.out
@@ -1240,6 +1240,66 @@ 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)
+
+-- Also check deparsing of such cases
+create view composite_v as
+with cte(c) as materialized (select row(1, 2)),
+     cte2(c) as (select * from cte)
+select 1 as one from cte2 as t
+where (select * from (select c as c1) s
+       where (select (c1).f1 > 0)) is not null;
+select pg_get_viewdef('composite_v', true);
+                     pg_get_viewdef
+--------------------------------------------------------
+  WITH cte(c) AS MATERIALIZED (                        +
+          SELECT ROW(1, 2) AS "row"                    +
+         ), cte2(c) AS (                               +
+          SELECT cte.c                                 +
+            FROM cte                                   +
+         )                                             +
+  SELECT 1 AS one                                      +
+    FROM cte2 t                                        +
+   WHERE (( SELECT s.c1                                +
+            FROM ( SELECT t.c AS c1) s                 +
+           WHERE ( SELECT (s.c1).f1 > 0))) IS NOT NULL;
+(1 row)
+
+drop view composite_v;
 --
 -- Tests for component access / FieldSelect
 --
diff --git a/src/test/regress/sql/rowtypes.sql b/src/test/regress/sql/rowtypes.sql
index 565e6249d5..fd47dc9e0f 100644
--- a/src/test/regress/sql/rowtypes.sql
+++ b/src/test/regress/sql/rowtypes.sql
@@ -494,6 +494,31 @@ 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;
+
+-- Also check deparsing of such cases
+create view composite_v as
+with cte(c) as materialized (select row(1, 2)),
+     cte2(c) as (select * from cte)
+select 1 as one from cte2 as t
+where (select * from (select c as c1) s
+       where (select (c1).f1 > 0)) is not null;
+select pg_get_viewdef('composite_v', true);
+drop view composite_v;

 --
 -- Tests for component access / FieldSelect