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
Date: Sun, 14 Jul 2024 13:30:55 -0400
Message-ID: <2946840.1720978255@sss.pgh.pa.us> (raw)
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>
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
Attachments:v2-fix-bug-18536.patch (text/x-diff, 5.6K) downloaddiff --git a/src/backend/parser/parse_cte.c b/src/backend/parser/parse_cte.cindex 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 = i;- cstate->innerwiths = NIL;- cstate->selfrefcount = 0;- cstate->context = RECURSION_NONRECURSIVETERM;- checkWellFormedRecursionWalker((Node *) stmt->larg, cstate);- Assert(cstate->innerwiths == NIL);-- /* Right-hand operand should contain one reference in a valid place */- cstate->curitem = i;- cstate->innerwiths = NIL;- cstate->selfrefcount = 0;- cstate->context = RECURSION_OK;- checkWellFormedRecursionWalker((Node *) stmt->rarg, cstate);- Assert(cstate->innerwiths == NIL);- if (cstate->selfrefcount != 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 = 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 = i;+ cstate->innerwiths = NIL;+ cstate->selfrefcount = 0;+ cstate->context = RECURSION_NONRECURSIVETERM;+ checkWellFormedRecursionWalker((Node *) stmt->larg, cstate);+ Assert(cstate->innerwiths == NIL);++ /* Right-hand operand should contain one reference in a valid place */+ cstate->curitem = i;+ cstate->innerwiths = NIL;+ cstate->selfrefcount = 0;+ cstate->context = RECURSION_OK;+ checkWellFormedRecursionWalker((Node *) stmt->rarg, cstate);+ Assert(cstate->innerwiths == NIL);+ if (cstate->selfrefcount != 1) /* shouldn't happen */+ elog(ERROR, "missing recursive reference");
}
}
diff --git a/src/test/regress/expected/with.out b/src/test/regress/expected/with.outindex 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 SELECT 1)
ERROR: recursive reference to query "x" must not appear within its non-recursive 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 subquery+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-recursive 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.sqlindex 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);
=
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, exclusion@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18536: Using WITH inside WITH RECURSIVE triggers a "shouldn't happen" error
In-Reply-To: <2946840.1720978255@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