From ae13f8166dc372a94e596f423790a67abd7bf68c Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 15 Sep 2023 17:01:26 -0400 Subject: [PATCH] Track nesting depth correctly when drilling down into RECORD Vars. expandRecordVariable() failed to adjust the parse nesting structure correctly when recursing to inspect an outer-level Var. This could result in assertion failures or core dumps in corner cases. Likewise, get_name_for_var_field() failed to adjust the deparse namespace stack correctly when recursing to inspect an outer-level Var. In this case the likely result was a "bogus varno" error while deparsing a view. Per bug #18077 from Jingzhou Fu. Back-patch to all supported branches. Richard Guo, with some adjustments by me Discussion: https://postgr.es/m/18077-b9db97c6e0ab45d8@postgresql.org --- src/backend/parser/parse_target.c | 20 ++++++--- src/backend/utils/adt/ruleutils.c | 41 ++++++++++-------- src/test/regress/expected/rowtypes.out | 60 ++++++++++++++++++++++++++ src/test/regress/sql/rowtypes.sql | 25 +++++++++++ 4 files changed, 122 insertions(+), 24 deletions(-) diff --git a/src/backend/parser/parse_target.c b/src/backend/parser/parse_target.c index 9ce3a0de96..cf373c5888 100644 --- a/src/backend/parser/parse_target.c +++ b/src/backend/parser/parse_target.c @@ -1503,7 +1503,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) @@ -1591,11 +1592,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; + ParseState mypstate = {0}; + Index levelsup; - MemSet(&mypstate, 0, sizeof(mypstate)); + /* 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 */ @@ -1646,12 +1653,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 d13476d7ba..b0551dba67 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -7616,22 +7616,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; - set_deparse_for_query(&mydpns, rte->subquery, - context->namespaces); + parent_namespaces = list_copy_tail(context->namespaces, + netlevelsup); - context->namespaces = lcons(&mydpns, - context->namespaces); + set_deparse_for_query(&mydpns, rte->subquery, + parent_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; } @@ -7723,7 +7729,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)) @@ -7731,21 +7737,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; - set_deparse_for_query(&mydpns, ctequery, - context->namespaces); + parent_namespaces = list_copy_tail(context->namespaces, + ctelevelsup); - new_nslist = list_copy_tail(context->namespaces, - ctelevelsup); - context->namespaces = lcons(&mydpns, new_nslist); + set_deparse_for_query(&mydpns, ctequery, + parent_namespaces); + + 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 4a05a730d9..52de8b2de8 100644 --- a/src/test/regress/expected/rowtypes.out +++ b/src/test/regress/expected/rowtypes.out @@ -1209,6 +1209,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 2ea5e25525..da2e74d8a5 100644 --- a/src/test/regress/sql/rowtypes.sql +++ b/src/test/regress/sql/rowtypes.sql @@ -487,6 +487,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