Re: BUG #17320: A SEGV in optimizer

[ 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", Line: 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

diff --git a/src/backend/rewrite/rewriteSearchCycle.c b/src/backend/rewrite/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 = 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 subquery
+     * (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 the
+     * 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 = 1; rti <= list_length(rte2->subquery->rtable); rti++)
     {
         RangeTblEntry *e = rt_fetch(rti, rte2->subquery->rtable);

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

     newsubquery = copyObject(rte2->subquery);
     IncrementVarSublevelsUp((Node *) newsubquery, 1, 1);
diff --git a/src/test/regress/expected/with.out b/src/test/regress/expected/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 be 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 query "x" must be at the top level of its
right-handSELECT 
 -- 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 (