Skip to content

Commit 134a8ee

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 f19502f commit 134a8ee

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
@@ -585,7 +585,10 @@ rewriteRuleAction(Query *parsetree,
585585
}
586586
}
587587

588-
/* OK, it's safe to combine the CTE lists */
588+
/*
589+
* OK, it's safe to combine the CTE lists. Beware that RewriteQuery
590+
* knows we concatenate the lists in this order.
591+
*/
589592
sub_action->cteList = list_concat(sub_action->cteList,
590593
copyObject(parsetree->cteList));
591594
/* ... and don't forget about the associated flags */
@@ -3687,9 +3690,13 @@ rewriteTargetView(Query *parsetree, Relation view)
36873690
* orig_rt_length is the length of the originating query's rtable, for product
36883691
* queries created by fireRules(), and 0 otherwise. This is used to skip any
36893692
* already-processed VALUES RTEs from the original query.
3693+
*
3694+
* num_ctes_processed is the number of CTEs at the end of the query's cteList
3695+
* that have already been rewritten, and must not be rewritten again.
36903696
*/
36913697
static List *
3692-
RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length)
3698+
RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length,
3699+
int num_ctes_processed)
36933700
{
36943701
CmdType event = parsetree->commandType;
36953702
bool instead = false;
@@ -3703,17 +3710,29 @@ RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length)
37033710
* First, recursively process any insert/update/delete statements in WITH
37043711
* clauses. (We have to do this first because the WITH clauses may get
37053712
* copied into rule actions below.)
3713+
*
3714+
* Any new WITH clauses from rule actions are processed when we recurse
3715+
* into product queries below. However, when recursing, we must take care
3716+
* to avoid rewriting a CTE query more than once (because expanding
3717+
* generated columns in the targetlist more than once would fail). Since
3718+
* new CTEs from product queries are added to the start of the list (see
3719+
* rewriteRuleAction), we just skip the last num_ctes_processed items.
37063720
*/
37073721
foreach(lc1, parsetree->cteList)
37083722
{
37093723
CommonTableExpr *cte = lfirst_node(CommonTableExpr, lc1);
37103724
Query *ctequery = castNode(Query, cte->ctequery);
3725+
int i = foreach_current_index(lc1);
37113726
List *newstuff;
37123727

3728+
/* Skip already-processed CTEs at the end of the list */
3729+
if (i >= list_length(parsetree->cteList) - num_ctes_processed)
3730+
break;
3731+
37133732
if (ctequery->commandType == CMD_SELECT)
37143733
continue;
37153734

3716-
newstuff = RewriteQuery(ctequery, rewrite_events, 0);
3735+
newstuff = RewriteQuery(ctequery, rewrite_events, 0, 0);
37173736

37183737
/*
37193738
* Currently we can only handle unconditional, single-statement DO
@@ -3772,6 +3791,7 @@ RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length)
37723791
errmsg("multi-statement DO INSTEAD rules are not supported for data-modifying statements in WITH")));
37733792
}
37743793
}
3794+
num_ctes_processed = list_length(parsetree->cteList);
37753795

37763796
/*
37773797
* If the statement is an insert, update, delete, or merge, adjust its
@@ -4134,7 +4154,8 @@ RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length)
41344154
newstuff = RewriteQuery(pt, rewrite_events,
41354155
pt == parsetree ?
41364156
orig_rt_length :
4137-
product_orig_rt_length);
4157+
product_orig_rt_length,
4158+
num_ctes_processed);
41384159
rewritten = list_concat(rewritten, newstuff);
41394160
}
41404161

@@ -4286,7 +4307,7 @@ QueryRewrite(Query *parsetree)
42864307
*
42874308
* Apply all non-SELECT rules possibly getting 0 or many queries
42884309
*/
4289-
querylist = RewriteQuery(parsetree, NIL, 0);
4310+
querylist = RewriteQuery(parsetree, NIL, 0, 0);
42904311

42914312
/*
42924313
* Step 2

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

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2798,6 +2798,47 @@ SELECT * FROM bug6051_3;
27982798
---
27992799
(0 rows)
28002800

2801+
-- check that recursive CTE processing doesn't rewrite a CTE more than once
2802+
-- (must not try to expand GENERATED ALWAYS IDENTITY columns more than once)
2803+
CREATE TEMP TABLE id_alw1 (i int GENERATED ALWAYS AS IDENTITY);
2804+
CREATE TEMP TABLE id_alw2 (i int GENERATED ALWAYS AS IDENTITY);
2805+
CREATE TEMP VIEW id_alw2_view AS SELECT * FROM id_alw2;
2806+
CREATE TEMP TABLE id_alw3 (i int GENERATED ALWAYS AS IDENTITY);
2807+
CREATE RULE id_alw3_ins AS ON INSERT TO id_alw3 DO INSTEAD
2808+
WITH t1 AS (INSERT INTO id_alw1 DEFAULT VALUES RETURNING i)
2809+
INSERT INTO id_alw2_view DEFAULT VALUES RETURNING i;
2810+
CREATE TEMP VIEW id_alw3_view AS SELECT * FROM id_alw3;
2811+
CREATE TEMP TABLE id_alw4 (i int GENERATED ALWAYS AS IDENTITY);
2812+
WITH t4 AS (INSERT INTO id_alw4 DEFAULT VALUES RETURNING i)
2813+
INSERT INTO id_alw3_view DEFAULT VALUES RETURNING i;
2814+
i
2815+
---
2816+
1
2817+
(1 row)
2818+
2819+
SELECT * from id_alw1;
2820+
i
2821+
---
2822+
1
2823+
(1 row)
2824+
2825+
SELECT * from id_alw2;
2826+
i
2827+
---
2828+
1
2829+
(1 row)
2830+
2831+
SELECT * from id_alw3;
2832+
i
2833+
---
2834+
(0 rows)
2835+
2836+
SELECT * from id_alw4;
2837+
i
2838+
---
2839+
1
2840+
(1 row)
2841+
28012842
-- check case where CTE reference is removed due to optimization
28022843
EXPLAIN (VERBOSE, COSTS OFF)
28032844
SELECT q1 FROM

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

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

13281328
SELECT * FROM bug6051_3;
13291329

1330+
-- check that recursive CTE processing doesn't rewrite a CTE more than once
1331+
-- (must not try to expand GENERATED ALWAYS IDENTITY columns more than once)
1332+
CREATE TEMP TABLE id_alw1 (i int GENERATED ALWAYS AS IDENTITY);
1333+
1334+
CREATE TEMP TABLE id_alw2 (i int GENERATED ALWAYS AS IDENTITY);
1335+
CREATE TEMP VIEW id_alw2_view AS SELECT * FROM id_alw2;
1336+
1337+
CREATE TEMP TABLE id_alw3 (i int GENERATED ALWAYS AS IDENTITY);
1338+
CREATE RULE id_alw3_ins AS ON INSERT TO id_alw3 DO INSTEAD
1339+
WITH t1 AS (INSERT INTO id_alw1 DEFAULT VALUES RETURNING i)
1340+
INSERT INTO id_alw2_view DEFAULT VALUES RETURNING i;
1341+
CREATE TEMP VIEW id_alw3_view AS SELECT * FROM id_alw3;
1342+
1343+
CREATE TEMP TABLE id_alw4 (i int GENERATED ALWAYS AS IDENTITY);
1344+
1345+
WITH t4 AS (INSERT INTO id_alw4 DEFAULT VALUES RETURNING i)
1346+
INSERT INTO id_alw3_view DEFAULT VALUES RETURNING i;
1347+
1348+
SELECT * from id_alw1;
1349+
SELECT * from id_alw2;
1350+
SELECT * from id_alw3;
1351+
SELECT * from id_alw4;
1352+
13301353
-- check case where CTE reference is removed due to optimization
13311354
EXPLAIN (VERBOSE, COSTS OFF)
13321355
SELECT q1 FROM

0 commit comments

Comments
 (0)