Skip to content

Commit c090965

Browse files
deanrasheeddeanrasheed
committed
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 <bernice.southey@gmail.com> Author: Bernice Southey <bernice.southey@gmail.com> Author: Dean Rasheed <dean.a.rasheed@gmail.com> Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us> Reviewed-by: Kirill Reshke <reshkekirill@gmail.com> Discussion: https://postgr.es/m/CAEDh4nyD6MSH9bROhsOsuTqGAv_QceU_GDvN9WcHLtZTCYM1kA@mail.gmail.com Backpatch-through: 14
1 parent e79b276 commit c090965

File tree

3 files changed

+90
-5
lines changed

3 files changed

+90
-5
lines changed

‎src/backend/rewrite/rewriteHandler.c‎

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -590,7 +590,10 @@ rewriteRuleAction(Query *parsetree,
590590
}
591591
}
592592

593-
/* OK, it's safe to combine the CTE lists */
593+
/*
594+
* OK, it's safe to combine the CTE lists. Beware that RewriteQuery
595+
* knows we concatenate the lists in this order.
596+
*/
594597
sub_action->cteList = list_concat(sub_action->cteList,
595598
copyObject(parsetree->cteList));
596599
/* ... and don't forget about the associated flags */
@@ -3860,9 +3863,13 @@ rewriteTargetView(Query *parsetree, Relation view)
38603863
* orig_rt_length is the length of the originating query's rtable, for product
38613864
* queries created by fireRules(), and 0 otherwise. This is used to skip any
38623865
* already-processed VALUES RTEs from the original query.
3866+
*
3867+
* num_ctes_processed is the number of CTEs at the end of the query's cteList
3868+
* that have already been rewritten, and must not be rewritten again.
38633869
*/
38643870
static List *
3865-
RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length)
3871+
RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length,
3872+
int num_ctes_processed)
38663873
{
38673874
CmdType event = parsetree->commandType;
38683875
bool instead = false;
@@ -3876,17 +3883,29 @@ RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length)
38763883
* First, recursively process any insert/update/delete/merge statements in
38773884
* WITH clauses. (We have to do this first because the WITH clauses may
38783885
* get copied into rule actions below.)
3886+
*
3887+
* Any new WITH clauses from rule actions are processed when we recurse
3888+
* into product queries below. However, when recursing, we must take care
3889+
* to avoid rewriting a CTE query more than once (because expanding
3890+
* generated columns in the targetlist more than once would fail). Since
3891+
* new CTEs from product queries are added to the start of the list (see
3892+
* rewriteRuleAction), we just skip the last num_ctes_processed items.
38793893
*/
38803894
foreach(lc1, parsetree->cteList)
38813895
{
38823896
CommonTableExpr *cte = lfirst_node(CommonTableExpr, lc1);
38833897
Query *ctequery = castNode(Query, cte->ctequery);
3898+
int i = foreach_current_index(lc1);
38843899
List *newstuff;
38853900

3901+
/* Skip already-processed CTEs at the end of the list */
3902+
if (i >= list_length(parsetree->cteList) - num_ctes_processed)
3903+
break;
3904+
38863905
if (ctequery->commandType == CMD_SELECT)
38873906
continue;
38883907

3889-
newstuff = RewriteQuery(ctequery, rewrite_events, 0);
3908+
newstuff = RewriteQuery(ctequery, rewrite_events, 0, 0);
38903909

38913910
/*
38923911
* Currently we can only handle unconditional, single-statement DO
@@ -3946,6 +3965,7 @@ RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length)
39463965
errmsg("multi-statement DO INSTEAD rules are not supported for data-modifying statements in WITH")));
39473966
}
39483967
}
3968+
num_ctes_processed = list_length(parsetree->cteList);
39493969

39503970
/*
39513971
* If the statement is an insert, update, delete, or merge, adjust its
@@ -4277,7 +4297,8 @@ RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length)
42774297
newstuff = RewriteQuery(pt, rewrite_events,
42784298
pt == parsetree ?
42794299
orig_rt_length :
4280-
product_orig_rt_length);
4300+
product_orig_rt_length,
4301+
num_ctes_processed);
42814302
rewritten = list_concat(rewritten, newstuff);
42824303
}
42834304

@@ -4429,7 +4450,7 @@ QueryRewrite(Query *parsetree)
44294450
*
44304451
* Apply all non-SELECT rules possibly getting 0 or many queries
44314452
*/
4432-
querylist = RewriteQuery(parsetree, NIL, 0);
4453+
querylist = RewriteQuery(parsetree, NIL, 0, 0);
44334454

44344455
/*
44354456
* Step 2

‎src/test/regress/expected/with.out‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2851,6 +2851,47 @@ SELECT * FROM bug6051_3;
28512851
---
28522852
(0 rows)
28532853

2854+
-- check that recursive CTE processing doesn't rewrite a CTE more than once
2855+
-- (must not try to expand GENERATED ALWAYS IDENTITY columns more than once)
2856+
CREATE TEMP TABLE id_alw1 (i int GENERATED ALWAYS AS IDENTITY);
2857+
CREATE TEMP TABLE id_alw2 (i int GENERATED ALWAYS AS IDENTITY);
2858+
CREATE TEMP VIEW id_alw2_view AS SELECT * FROM id_alw2;
2859+
CREATE TEMP TABLE id_alw3 (i int GENERATED ALWAYS AS IDENTITY);
2860+
CREATE RULE id_alw3_ins AS ON INSERT TO id_alw3 DO INSTEAD
2861+
WITH t1 AS (INSERT INTO id_alw1 DEFAULT VALUES RETURNING i)
2862+
INSERT INTO id_alw2_view DEFAULT VALUES RETURNING i;
2863+
CREATE TEMP VIEW id_alw3_view AS SELECT * FROM id_alw3;
2864+
CREATE TEMP TABLE id_alw4 (i int GENERATED ALWAYS AS IDENTITY);
2865+
WITH t4 AS (INSERT INTO id_alw4 DEFAULT VALUES RETURNING i)
2866+
INSERT INTO id_alw3_view DEFAULT VALUES RETURNING i;
2867+
i
2868+
---
2869+
1
2870+
(1 row)
2871+
2872+
SELECT * from id_alw1;
2873+
i
2874+
---
2875+
1
2876+
(1 row)
2877+
2878+
SELECT * from id_alw2;
2879+
i
2880+
---
2881+
1
2882+
(1 row)
2883+
2884+
SELECT * from id_alw3;
2885+
i
2886+
---
2887+
(0 rows)
2888+
2889+
SELECT * from id_alw4;
2890+
i
2891+
---
2892+
1
2893+
(1 row)
2894+
28542895
-- check case where CTE reference is removed due to optimization
28552896
EXPLAIN (VERBOSE, COSTS OFF)
28562897
SELECT q1 FROM

‎src/test/regress/sql/with.sql‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1346,6 +1346,29 @@ COMMIT;
13461346

13471347
SELECT * FROM bug6051_3;
13481348

1349+
-- check that recursive CTE processing doesn't rewrite a CTE more than once
1350+
-- (must not try to expand GENERATED ALWAYS IDENTITY columns more than once)
1351+
CREATE TEMP TABLE id_alw1 (i int GENERATED ALWAYS AS IDENTITY);
1352+
1353+
CREATE TEMP TABLE id_alw2 (i int GENERATED ALWAYS AS IDENTITY);
1354+
CREATE TEMP VIEW id_alw2_view AS SELECT * FROM id_alw2;
1355+
1356+
CREATE TEMP TABLE id_alw3 (i int GENERATED ALWAYS AS IDENTITY);
1357+
CREATE RULE id_alw3_ins AS ON INSERT TO id_alw3 DO INSTEAD
1358+
WITH t1 AS (INSERT INTO id_alw1 DEFAULT VALUES RETURNING i)
1359+
INSERT INTO id_alw2_view DEFAULT VALUES RETURNING i;
1360+
CREATE TEMP VIEW id_alw3_view AS SELECT * FROM id_alw3;
1361+
1362+
CREATE TEMP TABLE id_alw4 (i int GENERATED ALWAYS AS IDENTITY);
1363+
1364+
WITH t4 AS (INSERT INTO id_alw4 DEFAULT VALUES RETURNING i)
1365+
INSERT INTO id_alw3_view DEFAULT VALUES RETURNING i;
1366+
1367+
SELECT * from id_alw1;
1368+
SELECT * from id_alw2;
1369+
SELECT * from id_alw3;
1370+
SELECT * from id_alw4;
1371+
13491372
-- check case where CTE reference is removed due to optimization
13501373
EXPLAIN (VERBOSE, COSTS OFF)
13511374
SELECT q1 FROM

0 commit comments

Comments
 (0)