Dean Rasheed <[email protected]> wrote: > 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 was playing around with an idea I had based on my first attempt to fix this, when I got your email.
FWIW, I think I found a way to fix my bug that doesn't break your rules example. I added a bool to RewriteQuery, and used this to stop reprocessing updatable view CTEs. This leaves the rules recursion path unchanged. Which means rule CTEs might still be processed repeatedly. But as they probably can't be data-modifiable, I think rewriteTargetListIU is effectively idempotent? The advantage of your approach is it's consistent for all views, and it doesn't rely on improbable data-modifiability for idempotency. Plus I'm still very uncertain if there's more traps in rules I'm unaware of. I also took another look at rewriteTargetListIU. From what I can make out, stored generated columns rely on SetToDefault for idempotency. But identity can't use this, so this is why a change to querytree could be needed? Thanks, Bernice
From: Bernice Southey <[email protected]> Date: Tue, 25 Nov 2025 15:35:41 +0000 Subject: [PATCH] avoid rewriting data-modifying CTEs more than once A repeat rewrite of a CTE can cause an error because rewriteTargetListIU is not idempotent for generated identity columns. This patch prevents updatable view recursion from reprocessing CTEs. It leaves rules recursion unchanged as they don't have data-modifying CTEs. --- src/backend/rewrite/rewriteHandler.c | 17 +++++++--- src/test/regress/expected/with.out | 46 ++++++++++++++++++++++++++++ src/test/regress/sql/with.sql | 33 ++++++++++++++++++++ 3 files changed, 92 insertions(+), 4 deletions(-) diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c index adc9e7600e1..366e3bdd235 100644 --- a/src/backend/rewrite/rewriteHandler.c +++ b/src/backend/rewrite/rewriteHandler.c @@ -3872,9 +3872,14 @@ rewriteTargetView(Query *parsetree, Relation view) * 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. + * + * process_cte controls if the cteList should processed. When recursing, we + * use this to avoid rewriting a CTE more than once for updatable views. + * Rules shouldn't contain any data-modifying CTEs and so can be reprocessed idempotently. */ static List * -RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length) +RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length, + bool process_cte) { CmdType event = parsetree->commandType; bool instead = false; @@ -3889,6 +3894,8 @@ RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length) * WITH clauses. (We have to do this first because the WITH clauses may * get copied into rule actions below.) */ + if (process_cte) + { foreach(lc1, parsetree->cteList) { CommonTableExpr *cte = lfirst_node(CommonTableExpr, lc1); @@ -3898,7 +3905,7 @@ RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length) if (ctequery->commandType == CMD_SELECT) continue; - newstuff = RewriteQuery(ctequery, rewrite_events, 0); + newstuff = RewriteQuery(ctequery, rewrite_events, 0, true); /* * Currently we can only handle unconditional, single-statement DO @@ -3958,6 +3965,7 @@ RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length) errmsg("multi-statement DO INSTEAD rules are not supported for data-modifying statements in WITH"))); } } + } /* * If the statement is an insert, update, delete, or merge, adjust its @@ -4289,7 +4297,8 @@ RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length) newstuff = RewriteQuery(pt, rewrite_events, pt == parsetree ? orig_rt_length : - product_orig_rt_length); + product_orig_rt_length, + pt != parsetree); rewritten = list_concat(rewritten, newstuff); } @@ -4564,7 +4573,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, true); /* * Step 2 diff --git a/src/test/regress/expected/with.out b/src/test/regress/expected/with.out index f4caedf272f..448c20f55af 100644 --- a/src/test/regress/expected/with.out +++ b/src/test/regress/expected/with.out @@ -2872,6 +2872,52 @@ SELECT * FROM bug6051_3; --- (0 rows) +-- check that recursive CTE processing doesn't rewrite a CTE +-- more than once on an updatable view +-- (must not try to expand IDENTITY column more than once) +CREATE TABLE id_alw (i int GENERATED ALWAYS AS IDENTITY); +CREATE TABLE id_alw_base (i int); +CREATE 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 that recursive CTE processing rewrites CTES at every + --level for rule actions +CREATE TABLE t1 (a int); +CREATE TABLE t1a (a int); +CREATE TABLE t1b (a int); +CREATE rule t1r AS ON INSERT TO t1 do instead + WITH x AS (INSERT INTO t1a VALUES (1) RETURNING *) + INSERT INTO t1b SELECT a FROM x RETURNING *; +CREATE TABLE t2 (a int); +CREATE TABLE t2a (a int); +CREATE rule t2r AS ON INSERT TO t2 do instead + WITH xx AS (INSERT INTO t1 VALUES (1) RETURNING *) + INSERT INTO t2a SELECT a FROM xx RETURNING *; +INSERT INTO t2 VALUES (1); +SELECT * FROM t1a; + a +--- + 1 +(1 row) + +SELECT * FROM t1b; + a +--- + 1 +(1 row) + +SELECT * FROM t2a; + a +--- + 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 index cd25a5e7154..b7af05c5cec 100644 --- a/src/test/regress/sql/with.sql +++ b/src/test/regress/sql/with.sql @@ -1360,6 +1360,39 @@ COMMIT; SELECT * FROM bug6051_3; +-- check that recursive CTE processing doesn't rewrite a CTE +-- more than once on an updatable view +-- (must not try to expand IDENTITY column more than once) +CREATE TABLE id_alw (i int GENERATED ALWAYS AS IDENTITY); +CREATE TABLE id_alw_base (i int); +CREATE 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 that recursive CTE processing rewrites CTES at every + --level for rule actions +CREATE TABLE t1 (a int); +CREATE TABLE t1a (a int); +CREATE TABLE t1b (a int); +CREATE rule t1r AS ON INSERT TO t1 do instead + WITH x AS (INSERT INTO t1a VALUES (1) RETURNING *) + INSERT INTO t1b SELECT a FROM x RETURNING *; + +CREATE TABLE t2 (a int); +CREATE TABLE t2a (a int); +CREATE rule t2r AS ON INSERT TO t2 do instead + WITH xx AS (INSERT INTO t1 VALUES (1) RETURNING *) + INSERT INTO t2a SELECT a FROM xx RETURNING *; + +INSERT INTO t2 VALUES (1); + +SELECT * FROM t1a; +SELECT * FROM t1b; +SELECT * FROM t2a; + -- check case where CTE reference is removed due to optimization EXPLAIN (VERBOSE, COSTS OFF) SELECT q1 FROM -- 2.43.0
