Fix parse_cte.c's failure to examine sub-WITHs in DML statements.

makeDependencyGraphWalker thought that only SelectStmt nodes could
contain a WithClause.  Which was true in our original implementation
of WITH, but astonishingly we missed updating this code when we added
the ability to attach WITH to INSERT/UPDATE/DELETE (and later MERGE).
Moreover, since it was coded to deliberately block recursion to a
WithClause, even updating raw_expression_tree_walker didn't save it.

The upshot of this was that we didn't see references to outer CTE
names appearing within an inner WITH, and would neither complain about
disallowed recursion nor account for such references when sorting CTEs
into a usable order.  The lack of complaints about this is perhaps not
so surprising, because typical usage of WITH wouldn't hit either case.
Still, it's pretty broken; failing to detect recursion here leads to
assert failures or worse later on.

Fix by factoring out the processing of sub-WITHs into a new function
WalkInnerWith, and invoking that for all the statement types that
can have WITH.

Bug: #18878
Reported-by: Yu Liang <luy70@psu.edu>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/18878-a26fa5ab6be2f2cf@postgresql.org
Backpatch-through: 13

diff --git a/src/backend/parser/parse_cte.c b/src/backend/parser/parse_cte.c
index 68bb5e0919c58950078c2293c930d03c72e67b5a..81bed0c5f233fb169dc8821e825ca7c5e86092c1 100644 (file)
--- a/src/backend/parser/parse_cte.c
+++ b/src/backend/parser/parse_cte.c
@@ -88,6 +88,7 @@ static void analyzeCTE(ParseState *pstate, CommonTableExpr *cte);
 /* Dependency processing functions */
 static void makeDependencyGraph(CteState *cstate);
 static bool makeDependencyGraphWalker(Node *node, CteState *cstate);
+static void WalkInnerWith(Node *stmt, WithClause *withClause, CteState *cstate);
 static void TopologicalSort(ParseState *pstate, CteItem *items, int numitems);
 
 /* Recursion validity checker functions */
@@ -731,58 +732,69 @@ makeDependencyGraphWalker(Node *node, CteState *cstate)
        if (IsA(node, SelectStmt))
        {
                SelectStmt *stmt = (SelectStmt *) node;
-               ListCell   *lc;
 
                if (stmt->withClause)
                {
-                       if (stmt->withClause->recursive)
-                       {
-                               /*
-                                * In the RECURSIVE case, all query names of the WITH are
-                                * visible to all WITH items as well as the main query. So
-                                * push them all on, process, pop them all off.
-                                */
-                               cstate->innerwiths = lcons(stmt->withClause->ctes,
-                                                                                  cstate->innerwiths);
-                               foreach(lc, stmt->withClause->ctes)
-                               {
-                                       CommonTableExpr *cte = (CommonTableExpr *) lfirst(lc);
+                       /* Examine the WITH clause and the SelectStmt */
+                       WalkInnerWith(node, stmt->withClause, cstate);
+                       /* We're done examining the SelectStmt */
+                       return false;
+               }
+               /* if no WITH clause, just fall through for normal processing */
+       }
+       else if (IsA(node, InsertStmt))
+       {
+               InsertStmt *stmt = (InsertStmt *) node;
 
-                                       (void) makeDependencyGraphWalker(cte->ctequery, cstate);
-                               }
-                               (void) raw_expression_tree_walker(node,
-                                                                                                 makeDependencyGraphWalker,
-                                                                                                 (void *) cstate);
-                               cstate->innerwiths = list_delete_first(cstate->innerwiths);
-                       }
-                       else
-                       {
-                               /*
-                                * In the non-RECURSIVE case, query names are visible to the
-                                * WITH items after them and to the main query.
-                                */
-                               cstate->innerwiths = lcons(NIL, cstate->innerwiths);
-                               foreach(lc, stmt->withClause->ctes)
-                               {
-                                       CommonTableExpr *cte = (CommonTableExpr *) lfirst(lc);
-                                       ListCell   *cell1;
+               if (stmt->withClause)
+               {
+                       /* Examine the WITH clause and the InsertStmt */
+                       WalkInnerWith(node, stmt->withClause, cstate);
+                       /* We're done examining the InsertStmt */
+                       return false;
+               }
+               /* if no WITH clause, just fall through for normal processing */
+       }
+       else if (IsA(node, DeleteStmt))
+       {
+               DeleteStmt *stmt = (DeleteStmt *) node;
 
-                                       (void) makeDependencyGraphWalker(cte->ctequery, cstate);
-                                       /* note that recursion could mutate innerwiths list */
-                                       cell1 = list_head(cstate->innerwiths);
-                                       lfirst(cell1) = lappend((List *) lfirst(cell1), cte);
-                               }
-                               (void) raw_expression_tree_walker(node,
-                                                                                                 makeDependencyGraphWalker,
-                                                                                                 (void *) cstate);
-                               cstate->innerwiths = list_delete_first(cstate->innerwiths);
-                       }
-                       /* We're done examining the SelectStmt */
+               if (stmt->withClause)
+               {
+                       /* Examine the WITH clause and the DeleteStmt */
+                       WalkInnerWith(node, stmt->withClause, cstate);
+                       /* We're done examining the DeleteStmt */
                        return false;
                }
                /* if no WITH clause, just fall through for normal processing */
        }
-       if (IsA(node, WithClause))
+       else if (IsA(node, UpdateStmt))
+       {
+               UpdateStmt *stmt = (UpdateStmt *) node;
+
+               if (stmt->withClause)
+               {
+                       /* Examine the WITH clause and the UpdateStmt */
+                       WalkInnerWith(node, stmt->withClause, cstate);
+                       /* We're done examining the UpdateStmt */
+                       return false;
+               }
+               /* if no WITH clause, just fall through for normal processing */
+       }
+       else if (IsA(node, MergeStmt))
+       {
+               MergeStmt  *stmt = (MergeStmt *) node;
+
+               if (stmt->withClause)
+               {
+                       /* Examine the WITH clause and the MergeStmt */
+                       WalkInnerWith(node, stmt->withClause, cstate);
+                       /* We're done examining the MergeStmt */
+                       return false;
+               }
+               /* if no WITH clause, just fall through for normal processing */
+       }
+       else if (IsA(node, WithClause))
        {
                /*
                 * Prevent raw_expression_tree_walker from recursing directly into a
@@ -796,6 +808,60 @@ makeDependencyGraphWalker(Node *node, CteState *cstate)
                                                                          (void *) cstate);
 }
 
+/*
+ * makeDependencyGraphWalker's recursion into a statement having a WITH clause.
+ *
+ * This subroutine is concerned with updating the innerwiths list correctly
+ * based on the visibility rules for CTE names.
+ */
+static void
+WalkInnerWith(Node *stmt, WithClause *withClause, CteState *cstate)
+{
+       ListCell   *lc;
+
+       if (withClause->recursive)
+       {
+               /*
+                * In the RECURSIVE case, all query names of the WITH are visible to
+                * all WITH items as well as the main query.  So push them all on,
+                * process, pop them all off.
+                */
+               cstate->innerwiths = lcons(withClause->ctes, cstate->innerwiths);
+               foreach(lc, withClause->ctes)
+               {
+                       CommonTableExpr *cte = (CommonTableExpr *) lfirst(lc);
+
+                       (void) makeDependencyGraphWalker(cte->ctequery, cstate);
+               }
+               (void) raw_expression_tree_walker(stmt,
+                                                                                 makeDependencyGraphWalker,
+                                                                                 (void *) cstate);
+               cstate->innerwiths = list_delete_first(cstate->innerwiths);
+       }
+       else
+       {
+               /*
+                * In the non-RECURSIVE case, query names are visible to the WITH
+                * items after them and to the main query.
+                */
+               cstate->innerwiths = lcons(NIL, cstate->innerwiths);
+               foreach(lc, withClause->ctes)
+               {
+                       CommonTableExpr *cte = (CommonTableExpr *) lfirst(lc);
+                       ListCell   *cell1;
+
+                       (void) makeDependencyGraphWalker(cte->ctequery, cstate);
+                       /* note that recursion could mutate innerwiths list */
+                       cell1 = list_head(cstate->innerwiths);
+                       lfirst(cell1) = lappend((List *) lfirst(cell1), cte);
+               }
+               (void) raw_expression_tree_walker(stmt,
+                                                                                 makeDependencyGraphWalker,
+                                                                                 (void *) cstate);
+               cstate->innerwiths = list_delete_first(cstate->innerwiths);
+       }
+}
+
 /*
  * Sort by dependencies, using a standard topological sort operation
  */

diff --git a/src/test/regress/expected/with.out b/src/test/regress/expected/with.out
index 670d98784226a17791acabf9c9569c40d9f99a04..ea85a175252bfb38daef65e3ce5aa50a2b164f34 100644 (file)
--- a/src/test/regress/expected/with.out
+++ b/src/test/regress/expected/with.out
@@ -2031,6 +2031,14 @@ WITH RECURSIVE x(n) AS (
 ERROR:  ORDER BY in a recursive query is not implemented
 LINE 3:   ORDER BY (SELECT n FROM x))
                    ^
+-- and this
+WITH RECURSIVE x(n) AS (
+  WITH sub_cte AS (SELECT * FROM x)
+  DELETE FROM graph RETURNING f)
+       SELECT * FROM x;
+ERROR:  recursive query "x" must not contain data-modifying statements
+LINE 1: WITH RECURSIVE x(n) AS (
+                       ^
 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 befa3575a166669fa9799c2e439a1353c61a9b18..d93f16f362b508c2828d2380b5d0e7b75f1c169d 100644 (file)
--- a/src/test/regress/sql/with.sql
+++ b/src/test/regress/sql/with.sql
@@ -930,6 +930,13 @@ WITH RECURSIVE x(n) AS (
   ORDER BY (SELECT n FROM x))
        SELECT * FROM x;
 
+-- and this
+WITH RECURSIVE x(n) AS (
+  WITH sub_cte AS (SELECT * FROM x)
+  DELETE FROM graph RETURNING f)
+       SELECT * FROM x;
+
+
 CREATE TEMPORARY TABLE y (a INTEGER);
 INSERT INTO y SELECT generate_series(1, 10);