From: Tom Lane <tgl@sss.pgh.pa.us>
To: exclusion@gmail.com
cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18536: Using WITH inside WITH RECURSIVE triggers a
 "shouldn't happen" error
In-reply-to: <2866155.1720976492@sss.pgh.pa.us>
References: <18536-0a342ec07901203e@postgresql.org>
 <2781444.1720969777@sss.pgh.pa.us> <2866155.1720976492@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Sun, 14 Jul 2024 13:01:32 -0400"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <2946719.1720978172.0@sss.pgh.pa.us>
Date: Sun, 14 Jul 2024 13:30:55 -0400
Message-ID: <2946840.1720978255@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/2946840.1720978255%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2946719.1720978172.1@sss.pgh.pa.us>

I wrote:
> Hmm, that is probably too strong: it will break some queries we've
> historically accepted.  What we need is just to forbid self-references
> within the WITH clause.  The code actually does that already, it's
> just doing it too late; so we can fix this with a simple re-ordering
> of the error checks, as attached.

Oh ...

regression=# WITH RECURSIVE x(n) AS (
select 0 union select 1 order by (select n from x)) select * from x;
ERROR:  missing recursive reference

We have to move *all* of those subsidiary-clause checks to before
the tests of the UNION proper.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="v2-fix-bug-18536.patch";
	charset="us-ascii"
Content-ID: <2946719.1720978172.2@sss.pgh.pa.us>
Content-Description: v2-fix-bug-18536.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/parser/parse_cte.c b/src/backend/parser/parse_cte=
.c
index 6826d4f36a..de9ae9b483 100644
--- a/src/backend/parser/parse_cte.c
+++ b/src/backend/parser/parse_cte.c
@@ -877,25 +877,14 @@ checkWellFormedRecursion(CteState *cstate)
 							cte->ctename),
 					 parser_errposition(cstate->pstate, cte->location)));
 =

-		/* The left-hand operand mustn't contain self-reference at all */
-		cstate->curitem =3D i;
-		cstate->innerwiths =3D NIL;
-		cstate->selfrefcount =3D 0;
-		cstate->context =3D RECURSION_NONRECURSIVETERM;
-		checkWellFormedRecursionWalker((Node *) stmt->larg, cstate);
-		Assert(cstate->innerwiths =3D=3D NIL);
-
-		/* Right-hand operand should contain one reference in a valid place */
-		cstate->curitem =3D i;
-		cstate->innerwiths =3D NIL;
-		cstate->selfrefcount =3D 0;
-		cstate->context =3D RECURSION_OK;
-		checkWellFormedRecursionWalker((Node *) stmt->rarg, cstate);
-		Assert(cstate->innerwiths =3D=3D NIL);
-		if (cstate->selfrefcount !=3D 1)	/* shouldn't happen */
-			elog(ERROR, "missing recursive reference");
-
-		/* WITH mustn't contain self-reference, either */
+		/*
+		 * Really, we should insist that there not be a top-level WITH, since
+		 * syntactically that would enclose the UNION.  However, we've not
+		 * done so in the past and it's probably too late to change.  Settle
+		 * for insisting that WITH not contain a self-reference.  Test this
+		 * before examining the UNION arms, to avoid issuing confusing errors
+		 * in such cases.
+		 */
 		if (stmt->withClause)
 		{
 			cstate->curitem =3D i;
@@ -912,7 +901,9 @@ checkWellFormedRecursion(CteState *cstate)
 		 * don't make sense because it's impossible to figure out what they
 		 * mean when we have only part of the recursive query's results. (If
 		 * we did allow them, we'd have to check for recursive references
-		 * inside these subtrees.)
+		 * inside these subtrees.  As for WITH, we have to do this before
+		 * examining the UNION arms, to avoid issuing confusing errors if
+		 * there is a recursive reference here.)
 		 */
 		if (stmt->sortClause)
 			ereport(ERROR,
@@ -938,6 +929,28 @@ checkWellFormedRecursion(CteState *cstate)
 					 errmsg("FOR UPDATE/SHARE in a recursive query is not implemented"),
 					 parser_errposition(cstate->pstate,
 										exprLocation((Node *) stmt->lockingClause))));
+
+		/*
+		 * Now we can get on with checking the UNION operands themselves.
+		 *
+		 * The left-hand operand mustn't contain a self-reference at all.
+		 */
+		cstate->curitem =3D i;
+		cstate->innerwiths =3D NIL;
+		cstate->selfrefcount =3D 0;
+		cstate->context =3D RECURSION_NONRECURSIVETERM;
+		checkWellFormedRecursionWalker((Node *) stmt->larg, cstate);
+		Assert(cstate->innerwiths =3D=3D NIL);
+
+		/* Right-hand operand should contain one reference in a valid place */
+		cstate->curitem =3D i;
+		cstate->innerwiths =3D NIL;
+		cstate->selfrefcount =3D 0;
+		cstate->context =3D RECURSION_OK;
+		checkWellFormedRecursionWalker((Node *) stmt->rarg, cstate);
+		Assert(cstate->innerwiths =3D=3D NIL);
+		if (cstate->selfrefcount !=3D 1)	/* shouldn't happen */
+			elog(ERROR, "missing recursive reference");
 	}
 }
 =

diff --git a/src/test/regress/expected/with.out b/src/test/regress/expecte=
d/with.out
index b4f3121751..08cfa5463f 100644
--- a/src/test/regress/expected/with.out
+++ b/src/test/regress/expected/with.out
@@ -2029,6 +2029,46 @@ WITH RECURSIVE x(n) AS (SELECT n FROM x UNION ALL S=
ELECT 1)
 ERROR:  recursive reference to query "x" must not appear within its non-r=
ecursive term
 LINE 1: WITH RECURSIVE x(n) AS (SELECT n FROM x UNION ALL SELECT 1)
                                               ^
+-- allow this, because we historically have
+WITH RECURSIVE x(n) AS (
+  WITH x1 AS (SELECT 1 AS n)
+    SELECT 0
+    UNION
+    SELECT * FROM x1)
+	SELECT * FROM x;
+ n =

+---
+ 0
+ 1
+(2 rows)
+
+-- but this should be rejected
+WITH RECURSIVE x(n) AS (
+  WITH x1 AS (SELECT 1 FROM x)
+    SELECT 0
+    UNION
+    SELECT * FROM x1)
+	SELECT * FROM x;
+ERROR:  recursive reference to query "x" must not appear within a subquer=
y
+LINE 2:   WITH x1 AS (SELECT 1 FROM x)
+                                    ^
+-- and this too
+WITH RECURSIVE x(n) AS (
+  (WITH x1 AS (SELECT 1 FROM x) SELECT * FROM x1)
+  UNION
+  SELECT 0)
+	SELECT * FROM x;
+ERROR:  recursive reference to query "x" must not appear within its non-r=
ecursive term
+LINE 2:   (WITH x1 AS (SELECT 1 FROM x) SELECT * FROM x1)
+                                     ^
+-- and this
+WITH RECURSIVE x(n) AS (
+  SELECT 0 UNION SELECT 1
+  ORDER BY (SELECT n FROM x))
+	SELECT * FROM x;
+ERROR:  ORDER BY in a recursive query is not implemented
+LINE 3:   ORDER BY (SELECT n FROM x))
+                   ^
 CREATE TEMPORARY TABLE y (a INTEGER);
 INSERT INTO y SELECT generate_series(1, 10);
 -- LEFT JOIN
diff --git a/src/test/regress/sql/with.sql b/src/test/regress/sql/with.sql
index 3fb0b33fce..8f6e6c0b40 100644
--- a/src/test/regress/sql/with.sql
+++ b/src/test/regress/sql/with.sql
@@ -908,6 +908,35 @@ WITH RECURSIVE x(n) AS (SELECT n FROM x)
 WITH RECURSIVE x(n) AS (SELECT n FROM x UNION ALL SELECT 1)
 	SELECT * FROM x;
 =

+-- allow this, because we historically have
+WITH RECURSIVE x(n) AS (
+  WITH x1 AS (SELECT 1 AS n)
+    SELECT 0
+    UNION
+    SELECT * FROM x1)
+	SELECT * FROM x;
+
+-- but this should be rejected
+WITH RECURSIVE x(n) AS (
+  WITH x1 AS (SELECT 1 FROM x)
+    SELECT 0
+    UNION
+    SELECT * FROM x1)
+	SELECT * FROM x;
+
+-- and this too
+WITH RECURSIVE x(n) AS (
+  (WITH x1 AS (SELECT 1 FROM x) SELECT * FROM x1)
+  UNION
+  SELECT 0)
+	SELECT * FROM x;
+
+-- and this
+WITH RECURSIVE x(n) AS (
+  SELECT 0 UNION SELECT 1
+  ORDER BY (SELECT n FROM x))
+	SELECT * FROM x;
+
 CREATE TEMPORARY TABLE y (a INTEGER);
 INSERT INTO y SELECT generate_series(1, 10);
 =


------- =_aaaaaaaaaa0--