From: Tom Lane <tgl@sss.pgh.pa.us>
To: Richard Guo <guofenglinux@gmail.com>
Cc: fuboat@outlook.com
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18077: PostgreSQL server subprocess crashed by a SELECT statement with WITH clause
Date: Fri, 01 Sep 2023 16:41:10 -0400
Message-ID: <694541.1693600870@sss.pgh.pa.us> (raw)
In-Reply-To: <CAMbWs4_zxFFSy8+6-FG2ZdJc91TQGM+yG0Yh7gydhjTQfb=9fg@mail.gmail.com>
References: <18077-b9db97c6e0ab45d8@postgresql.org>
<CAMbWs4-4mDc9j5t+d3MbOSFSTqCDAHQKVV8zE8yiwsaXFGHL9Q@mail.gmail.com>
<CAMbWs4_zxFFSy8+6-FG2ZdJc91TQGM+yG0Yh7gydhjTQfb=9fg@mail.gmail.com>
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
Attachments:
[text/x-diff] v2-0001-Fix-expanding-Var-of-type-RECORD.patch (4.5K, ../694541.1693600870@sss.pgh.pa.us/2-v2-0001-Fix-expanding-Var-of-type-RECORD.patch)
download | inline diff:diff --git a/src/backend/parser/parse_target.c b/src/backend/parser/parse_target.cindex 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.outindex 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.sqlindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, guofenglinux@gmail.com, fuboat@outlook.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18077: PostgreSQL server subprocess crashed by a SELECT statement with WITH clause
In-Reply-To: <694541.1693600870@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox