On Wed, 26 Nov 2025 at 21:57, Tom Lane <[email protected]> wrote:
>
> I was toying with the idea of decoupling rewriting of WITHs from
> the main recursion. This would look roughly like
>
> 1. Pull out RewriteQuery's first loop into a new function, say
> RewriteQueryCTEs. Call this from QueryRewrite just before calling
> RewriteQuery.
>
> 2. Also apply it to a rule action's CTE list in fireRules, before
> we merge the original query's CTE list into that list.
Yes, I think that would work, but I think a simpler solution would be
to just have RewriteQuery() track which CTEs it had already rewritten,
which can be done just by passing around a list of CTE names.
Something like the attached.
I think this makes it more obvious how CTEs only get processed once,
and has the advantage of being a smaller, more localised change.
> given the lack of complaints
> to date, maybe a HEAD-only fix is acceptable.
I was thinking that we should try to create a back-patchable fix for
this. If it had been some complex query involving rules, then perhaps
a HEAD-only fix would be OK, but the original reproducer is a pretty
simple combination of more commonly-used features.
> There's an awful lot of unfinished work about CTEs in this module,
> eg the two different random implementation restrictions in the
> same stanza that's combining the CTE lists, or the one at the
> bottom of RewriteQuery. I wonder if we have the ambition to
> actually do anything about all that. People tend to see the
> rules system as a deprecated backwater, so maybe not.
Ugh, yeah, those limitations are not great. Something else I noticed
while trying to produce the test case with rules was that you can't
refer to the OLD or NEW pseudo-relations from within a CTE query in a
rule action. That pretty-much makes data-modifying CTEs in rule
actions useless, I think. But I tend to think of rules as a feature
that should be avoided at all costs in any real applications. In my
mind, that just makes them a maintenance burden, and I wouldn't want
to expend any effort trying to improve them, except if that made them
easier to maintain. That said, I know of real applications that do use
them (apparently without problems), so I can imagine we'd get pushback
if we tried to remove support for them.
Regards,
Dean
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
new file mode 100644
index adc9e76..bf3e517
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -3872,9 +3872,13 @@ rewriteTargetView(Query *parsetree, Rela
* orig_rt_length is the length of the originating query's rtable, for product
* queries created by fireRules(), and 0 otherwise. This is used to skip any
* already-processed VALUES RTEs from the original query.
+ *
+ * rewritten_ctes is a list of the names of any CTEs already rewritten. When
+ * recursing, we use this to avoid rewriting a CTE more than once.
*/
static List *
-RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length)
+RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length,
+ List *rewritten_ctes)
{
CmdType event = parsetree->commandType;
bool instead = false;
@@ -3884,21 +3888,55 @@ RewriteQuery(Query *parsetree, List *rew
List *rewritten = NIL;
ListCell *lc1;
+ /* Need a local copy of rewritten_ctes to scribble on */
+ rewritten_ctes = list_copy(rewritten_ctes);
+
/*
* First, recursively process any insert/update/delete/merge statements in
* WITH clauses. (We have to do this first because the WITH clauses may
* get copied into rule actions below.)
+ *
+ * Any new WITH clauses from rule actions are processed when we recurse
+ * into product queries below. However, when recursing, we must take care
+ * to avoid rewriting any CTE query more than once (because expanding
+ * generated columns in the targetlist more than once would fail).
*/
foreach(lc1, parsetree->cteList)
{
CommonTableExpr *cte = lfirst_node(CommonTableExpr, lc1);
Query *ctequery = castNode(Query, cte->ctequery);
+ bool found;
List *newstuff;
if (ctequery->commandType == CMD_SELECT)
continue;
- newstuff = RewriteQuery(ctequery, rewrite_events, 0);
+ /* Skip CTEs that have already been rewritten. */
+ found = false;
+ foreach_ptr(char, ctename, rewritten_ctes)
+ {
+ if (strcmp(ctename, cte->ctename) == 0)
+ {
+ found = true;
+ break;
+ }
+ }
+ if (found)
+ continue;
+
+ /*
+ * Recursively process this CTE query. None of its CTEs have been
+ * processed, so we pass rewritten_ctes = NIL here. (In fact, it
+ * shouldn't contain any data-modifying CTEs anyway, because they are
+ * only allowed at the top level.)
+ */
+ newstuff = RewriteQuery(ctequery, rewrite_events, 0, NIL);
+
+ /*
+ * Add this CTE to the list so that it is skipped when we rewrite
+ * product queries below.
+ */
+ rewritten_ctes = lappend(rewritten_ctes, cte->ctename);
/*
* Currently we can only handle unconditional, single-statement DO
@@ -4289,7 +4327,8 @@ RewriteQuery(Query *parsetree, List *rew
newstuff = RewriteQuery(pt, rewrite_events,
pt == parsetree ?
orig_rt_length :
- product_orig_rt_length);
+ product_orig_rt_length,
+ rewritten_ctes);
rewritten = list_concat(rewritten, newstuff);
}
@@ -4564,7 +4603,7 @@ QueryRewrite(Query *parsetree)
*
* Apply all non-SELECT rules possibly getting 0 or many queries
*/
- querylist = RewriteQuery(parsetree, NIL, 0);
+ querylist = RewriteQuery(parsetree, NIL, 0, NIL);
/*
* Step 2
diff --git a/src/test/regress/expected/with.out b/src/test/regress/expected/with.out
new file mode 100644
index f4caedf..e71c3cc
--- a/src/test/regress/expected/with.out
+++ b/src/test/regress/expected/with.out
@@ -2872,6 +2872,19 @@ SELECT * FROM bug6051_3;
---
(0 rows)
+-- check that recursive CTE processing doesn't rewrite a CTE more than once
+-- (must not try to expand IDENTITY column more than once)
+CREATE TEMP TABLE id_alw (i int GENERATED ALWAYS AS IDENTITY);
+CREATE TEMP TABLE id_alw_base (i int);
+CREATE TEMP VIEW id_alw_view AS SELECT * FROM id_alw_base;
+WITH t_cte AS (INSERT INTO id_alw DEFAULT VALUES RETURNING i)
+ INSERT INTO id_alw_view SELECT i FROM t_cte;
+SELECT * from id_alw_view;
+ i
+---
+ 1
+(1 row)
+
-- check case where CTE reference is removed due to optimization
EXPLAIN (VERBOSE, COSTS OFF)
SELECT q1 FROM
diff --git a/src/test/regress/sql/with.sql b/src/test/regress/sql/with.sql
new file mode 100644
index cd25a5e..efeb977
--- a/src/test/regress/sql/with.sql
+++ b/src/test/regress/sql/with.sql
@@ -1360,6 +1360,17 @@ COMMIT;
SELECT * FROM bug6051_3;
+-- check that recursive CTE processing doesn't rewrite a CTE more than once
+-- (must not try to expand IDENTITY column more than once)
+CREATE TEMP TABLE id_alw (i int GENERATED ALWAYS AS IDENTITY);
+CREATE TEMP TABLE id_alw_base (i int);
+CREATE TEMP VIEW id_alw_view AS SELECT * FROM id_alw_base;
+
+WITH t_cte AS (INSERT INTO id_alw DEFAULT VALUES RETURNING i)
+ INSERT INTO id_alw_view SELECT i FROM t_cte;
+
+SELECT * from id_alw_view;
+
-- check case where CTE reference is removed due to optimization
EXPLAIN (VERBOSE, COSTS OFF)
SELECT q1 FROM
