From 3881561d7715647dbb4a5bc27f116504903daf1b Mon Sep 17 00:00:00 2001 From: Dean Rasheed Date: Sat, 29 Nov 2025 12:28:59 +0000 Subject: Avoid rewriting data-modifying CTEs more than once. Formerly, when updating an auto-updatable view, or a relation with rules, if the original query had any data-modifying CTEs, the rewriter would rewrite those CTEs multiple times as RewriteQuery() recursed into the product queries. In most cases that was harmless, because RewriteQuery() is mostly idempotent. However, if the CTE involved updating an always-generated column, it would trigger an error because any subsequent rewrite would appear to be attempting to assign a non-default value to the always-generated column. This could perhaps be fixed by attempting to make RewriteQuery() fully idempotent, but that looks quite tricky to achieve, and would probably be quite fragile, given that more generated-column-type features might be added in the future. Instead, fix by arranging for RewriteQuery() to rewrite each CTE exactly once (by tracking the number of CTEs already rewritten as it recurses). This has the advantage of being simpler and more efficient, but it does make RewriteQuery() dependent on the order in which rewriteRuleAction() joins the CTE lists from the original query and the rule action, so care must be taken if that is ever changed. Reported-by: Bernice Southey Author: Bernice Southey Author: Dean Rasheed Reviewed-by: Tom Lane Reviewed-by: Kirill Reshke Discussion: https://postgr.es/m/CAEDh4nyD6MSH9bROhsOsuTqGAv_QceU_GDvN9WcHLtZTCYM1kA@mail.gmail.com Backpatch-through: 14 --- src/backend/rewrite/rewriteHandler.c | 31 ++++++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-) (limited to 'src/backend/rewrite/rewriteHandler.c') diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c index adc9e7600e1..8675776fcc6 100644 --- a/src/backend/rewrite/rewriteHandler.c +++ b/src/backend/rewrite/rewriteHandler.c @@ -592,7 +592,10 @@ rewriteRuleAction(Query *parsetree, } } - /* OK, it's safe to combine the CTE lists */ + /* + * OK, it's safe to combine the CTE lists. Beware that RewriteQuery + * knows we concatenate the lists in this order. + */ sub_action->cteList = list_concat(sub_action->cteList, copyObject(parsetree->cteList)); /* ... and don't forget about the associated flags */ @@ -3872,9 +3875,13 @@ 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. + * + * num_ctes_processed is the number of CTEs at the end of the query's cteList + * that have already been rewritten, and must not be rewritten again. */ static List * -RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length) +RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length, + int num_ctes_processed) { CmdType event = parsetree->commandType; bool instead = false; @@ -3888,17 +3895,29 @@ RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length) * 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 a CTE query more than once (because expanding + * generated columns in the targetlist more than once would fail). Since + * new CTEs from product queries are added to the start of the list (see + * rewriteRuleAction), we just skip the last num_ctes_processed items. */ foreach(lc1, parsetree->cteList) { CommonTableExpr *cte = lfirst_node(CommonTableExpr, lc1); Query *ctequery = castNode(Query, cte->ctequery); + int i = foreach_current_index(lc1); List *newstuff; + /* Skip already-processed CTEs at the end of the list */ + if (i >= list_length(parsetree->cteList) - num_ctes_processed) + break; + if (ctequery->commandType == CMD_SELECT) continue; - newstuff = RewriteQuery(ctequery, rewrite_events, 0); + newstuff = RewriteQuery(ctequery, rewrite_events, 0, 0); /* * Currently we can only handle unconditional, single-statement DO @@ -3958,6 +3977,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"))); } } + num_ctes_processed = list_length(parsetree->cteList); /* * If the statement is an insert, update, delete, or merge, adjust its @@ -4289,7 +4309,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, + num_ctes_processed); rewritten = list_concat(rewritten, newstuff); } @@ -4564,7 +4585,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, 0); /* * Step 2 -- cgit v1.2.3