From: Tom Lane <tgl@sss.pgh.pa.us>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
cc: 253540651@qq.com, pgsql-bugs@lists.postgresql.org,
        Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Subject: Re: BUG #17320: A SEGV in optimizer
In-reply-to: <20211207.172546.1329872704555357126.horikyota.ntt@gmail.com>
References: <17320-70e37868182512ab@postgresql.org>
 <20211207.172546.1329872704555357126.horikyota.ntt@gmail.com>
Comments: In-reply-to Kyotaro Horiguchi <horikyota.ntt@gmail.com>
	message dated "Tue, 07 Dec 2021 17:25:46 +0900"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <3774796.1650663748.0@sss.pgh.pa.us>
Date: Fri, 22 Apr 2022 17:43:43 -0400
Message-ID: <3774893.1650663823@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/3774893.1650663823%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <3774796.1650663748.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable

[ this'd been on my to-do list for a long time, I finally got to it ]

Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> If I tried to execute the following query, I got the follwoing
> error. This looks like rooted from the same mistake.

> WITH RECURSIVE x ( x ) AS
>  (SELECT 1
>   UNION
>   (WITH x AS (SELECT * FROM x)
>    SELECT * FROM x)
>  ) SEARCH BREADTH FIRST BY x SET NCHAR
> SELECT * FROM x;

> ERROR:  could not find attribute 2 in subquery targetlist

> By the way, if I executed the following query, I get the following
> another assertion failure.

> WITH RECURSIVE x ( x ) AS
>  (SELECT 1
>   UNION
>   (WITH y AS (SELECT * FROM x)
>    SELECT * FROM y)
>  ) SEARCH BREADTH FIRST BY x SET NCHAR
> SELECT * FROM x;

> TRAP: FailedAssertion("cte_rtindex > 0", File: "rewriteSearchCycle.c", L=
ine: 398, PID: 31288)

This is pretty remarkable, because those two queries *should* be
semantically identical.  I guessed that something is getting confused
by the duplicated CTE name, and it didn't take long to see what:
rewriteSearchAndCycle() is failing to check ctelevelsup while looking
for a match to the recursive CTE's name.  Thus, it thinks that the
reference to the lower WITH item (either "x" or "y" in these two
examples) is the recursive reference it seeks, which it ain't.

Eventually perhaps we should support this, since a recursive query
of this form is allowed in other contexts.  But I'm content to
throw error for now, as per the attached patch.  This catches all
the cases shown in this thread, as well as the seemingly unrelated
error shown in bug #17318.

(I looked for other places that might be carelessly matching on
ctename without checking levelsup, and found none.)

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff;
	name="detect-incorrect-recursive-ref-in-SEARCH-and-CYCLE.patch";
	charset="us-ascii"
Content-ID: <3774796.1650663748.2@sss.pgh.pa.us>
Content-Description: detect-incorrect-recursive-ref-in-SEARCH-and-CYCLE.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/rewrite/rewriteSearchCycle.c b/src/backend/rewrit=
e/rewriteSearchCycle.c
index dc408404eb..58f684cd52 100644
--- a/src/backend/rewrite/rewriteSearchCycle.c
+++ b/src/backend/rewrite/rewriteSearchCycle.c
@@ -383,19 +383,32 @@ rewriteSearchAndCycle(CommonTableExpr *cte)
 	newrte->eref =3D newrte->alias;
 =

 	/*
-	 * Find the reference to our CTE in the range table
+	 * Find the reference to the recursive CTE in the right UNION subquery's
+	 * range table.  We expect it to be two levels up from the UNION subquer=
y
+	 * (and must check that to avoid being fooled by sub-WITHs with the same
+	 * CTE name).  There will not be more than one such reference, because t=
he
+	 * parser would have rejected that (see checkWellFormedRecursion() in
+	 * parse_cte.c).  However, the parser doesn't insist that the reference
+	 * appear in the UNION subquery's topmost range table, so we might fail =
to
+	 * find it at all.  That's an unimplemented case for the moment.
 	 */
 	for (int rti =3D 1; rti <=3D list_length(rte2->subquery->rtable); rti++)
 	{
 		RangeTblEntry *e =3D rt_fetch(rti, rte2->subquery->rtable);
 =

-		if (e->rtekind =3D=3D RTE_CTE && strcmp(cte->ctename, e->ctename) =3D=3D=
 0)
+		if (e->rtekind =3D=3D RTE_CTE &&
+			strcmp(cte->ctename, e->ctename) =3D=3D 0 &&
+			e->ctelevelsup =3D=3D 2)
 		{
 			cte_rtindex =3D rti;
 			break;
 		}
 	}
-	Assert(cte_rtindex > 0);
+	if (cte_rtindex <=3D 0)
+		ereport(ERROR,
+				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+				 errmsg("with a SEARCH or CYCLE clause, the recursive reference to WI=
TH query \"%s\" must be at the top level of its right-hand SELECT",
+						cte->ctename)));
 =

 	newsubquery =3D copyObject(rte2->subquery);
 	IncrementVarSublevelsUp((Node *) newsubquery, 1, 1);
diff --git a/src/test/regress/expected/with.out b/src/test/regress/expecte=
d/with.out
index 32f90d7375..d731604374 100644
--- a/src/test/regress/expected/with.out
+++ b/src/test/regress/expected/with.out
@@ -846,6 +846,16 @@ with recursive search_graph(f, t, label) as (
 ) search depth first by f, t set seq
 select * from search_graph order by seq;
 ERROR:  with a SEARCH or CYCLE clause, the right side of the UNION must b=
e a SELECT
+-- check that we distinguish same CTE name used at different levels
+-- (this case could be supported, perhaps, but it isn't today)
+with recursive x(col) as (
+	select 1
+	union
+	(with x as (select * from x)
+	 select * from x)
+) search depth first by col set seq
+select * from x;
+ERROR:  with a SEARCH or CYCLE clause, the recursive reference to WITH qu=
ery "x" must be at the top level of its right-hand SELECT
 -- test ruleutils and view expansion
 create temp view v_search as
 with recursive search_graph(f, t, label) as (
diff --git a/src/test/regress/sql/with.sql b/src/test/regress/sql/with.sql
index 7e430e859e..3251c29584 100644
--- a/src/test/regress/sql/with.sql
+++ b/src/test/regress/sql/with.sql
@@ -464,6 +464,16 @@ with recursive search_graph(f, t, label) as (
 ) search depth first by f, t set seq
 select * from search_graph order by seq;
 =

+-- check that we distinguish same CTE name used at different levels
+-- (this case could be supported, perhaps, but it isn't today)
+with recursive x(col) as (
+	select 1
+	union
+	(with x as (select * from x)
+	 select * from x)
+) search depth first by col set seq
+select * from x;
+
 -- test ruleutils and view expansion
 create temp view v_search as
 with recursive search_graph(f, t, label) as (

------- =_aaaaaaaaaa0--