Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1qcAxB-00E8aX-Qo for pgsql-bugs@arkaria.postgresql.org; Fri, 01 Sep 2023 20:41:17 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1qcAxA-00HOSq-7z for pgsql-bugs@arkaria.postgresql.org; Fri, 01 Sep 2023 20:41:16 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1qcAx9-00HOSZ-L5 for pgsql-bugs@lists.postgresql.org; Fri, 01 Sep 2023 20:41:15 +0000 Received: from sss.pgh.pa.us ([68.162.161.243]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1qcAx6-002DUz-8w for pgsql-bugs@lists.postgresql.org; Fri, 01 Sep 2023 20:41:14 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.15.2/8.15.2) with ESMTP id 381KfAJT694542; Fri, 1 Sep 2023 16:41:10 -0400 From: Tom Lane To: Richard Guo cc: 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: References: <18077-b9db97c6e0ab45d8@postgresql.org> Comments: In-reply-to Richard Guo message dated "Wed, 30 Aug 2023 20:03:55 +0800" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <694524.1693600865.0@sss.pgh.pa.us> Date: Fri, 01 Sep 2023 16:41:10 -0400 Message-ID: <694541.1693600870@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="UTF-8" Content-ID: <694524.1693600865.1@sss.pgh.pa.us> Content-Transfer-Encoding: quoted-printable Richard Guo writes: > On Wed, Aug 30, 2023 at 7:42=E2=80=AFPM Richard Guo 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 w= e >> 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 ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="v2-0001-Fix-expanding-Var-of-type-RECORD.patch"; charset="us-ascii" Content-ID: <694524.1693600865.2@sss.pgh.pa.us> Content-Description: v2-0001-Fix-expanding-Var-of-type-RECORD.patch Content-Transfer-Encoding: quoted-printable 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 inf= er * the tupdesc from it. We ereport if we can't determine the tupdesc. * - * levelsup is an extra offset to interpret the Var's varlevelsup correct= ly. + * levelsup is an extra offset to interpret the Var's varlevelsup correct= ly + * 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 =3D {0}; + Index levelsup; = + /* this loop must work, since GetRTEByRangeTablePosn did */ + for (levelsup =3D 0; levelsup < netlevelsup; levelsup++) + pstate =3D pstate->parentParseState; mypstate.parentParseState =3D pstate; mypstate.p_rtable =3D 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 =3D {0}; Index levelsup; = - MemSet(&mypstate, 0, sizeof(mypstate)); /* this loop must work, since GetCTEForRTE did */ for (levelsup =3D 0; levelsup < rte->ctelevelsup + netlevelsup; diff --git a/src/test/regress/expected/rowtypes.out b/src/test/regress/exp= ected/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 isn= otnull 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/rowt= ypes.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 ------- =_aaaaaaaaaa0--