Skip to content

Commit 2d5b97b

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 9e77323 commit 2d5b97b

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 */
@@ -3677,9 +3680,13 @@ rewriteTargetView(Query *parsetree, Relation view)
36773680
* orig_rt_length is the length of the originating query's rtable, for product
36783681
* queries created by fireRules(), and 0 otherwise. This is used to skip any
36793682
* already-processed VALUES RTEs from the original query.
3683+
*
3684+
* num_ctes_processed is the number of CTEs at the end of the query's cteList
3685+
* that have already been rewritten, and must not be rewritten again.
36803686
*/
36813687
static List *
3682-
RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length)
3688+
RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length,
3689+
int num_ctes_processed)
36833690
{
36843691
CmdType event = parsetree->commandType;
36853692
bool instead = false;
@@ -3693,17 +3700,29 @@ RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length)
36933700
* First, recursively process any insert/update/delete statements in WITH
36943701
* clauses. (We have to do this first because the WITH clauses may get
36953702
* copied into rule actions below.)
3703+
*
3704+
* Any new WITH clauses from rule actions are processed when we recurse
3705+
* into product queries below. However, when recursing, we must take care
3706+
* to avoid rewriting a CTE query more than once (because expanding
3707+
* generated columns in the targetlist more than once would fail). Since
3708+
* new CTEs from product queries are added to the start of the list (see
3709+
* rewriteRuleAction), we just skip the last num_ctes_processed items.
36963710
*/
36973711
foreach(lc1, parsetree->cteList)
36983712
{
36993713
CommonTableExpr *cte = lfirst_node(CommonTableExpr, lc1);
37003714
Query *ctequery = castNode(Query, cte->ctequery);
3715+
int i = foreach_current_index(lc1);
37013716
List *newstuff;
37023717

3718+
/* Skip already-processed CTEs at the end of the list */
3719+
if (i >= list_length(parsetree->cteList) - num_ctes_processed)
3720+
break;
3721+
37033722
if (ctequery->commandType == CMD_SELECT)
37043723
continue;
37053724

3706-
newstuff = RewriteQuery(ctequery, rewrite_events, 0);
3725+
newstuff = RewriteQuery(ctequery, rewrite_events, 0, 0);
37073726

37083727
/*
37093728
* Currently we can only handle unconditional, single-statement DO
@@ -3762,6 +3781,7 @@ RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length)
37623781
errmsg("multi-statement DO INSTEAD rules are not supported for data-modifying statements in WITH")));
37633782
}
37643783
}
3784+
num_ctes_processed = list_length(parsetree->cteList);
37653785

37663786
/*
37673787
* If the statement is an insert, update, or delete, adjust its targetlist
@@ -4087,7 +4107,8 @@ RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length)
40874107
newstuff = RewriteQuery(pt, rewrite_events,
40884108
pt == parsetree ?
40894109
orig_rt_length :
4090-
product_orig_rt_length);
4110+
product_orig_rt_length,
4111+
num_ctes_processed);
40914112
rewritten = list_concat(rewritten, newstuff);
40924113
}
40934114

@@ -4239,7 +4260,7 @@ QueryRewrite(Query *parsetree)
42394260
*
42404261
* Apply all non-SELECT rules possibly getting 0 or many queries
42414262
*/
4242-
querylist = RewriteQuery(parsetree, NIL, 0);
4263+
querylist = RewriteQuery(parsetree, NIL, 0, 0);
42434264

42444265
/*
42454266
* Step 2

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

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2767,6 +2767,47 @@ SELECT * FROM bug6051_3;
27672767
---
27682768
(0 rows)
27692769

2770+
-- check that recursive CTE processing doesn't rewrite a CTE more than once
2771+
-- (must not try to expand GENERATED ALWAYS IDENTITY columns more than once)
2772+
CREATE TEMP TABLE id_alw1 (i int GENERATED ALWAYS AS IDENTITY);
2773+
CREATE TEMP TABLE id_alw2 (i int GENERATED ALWAYS AS IDENTITY);
2774+
CREATE TEMP VIEW id_alw2_view AS SELECT * FROM id_alw2;
2775+
CREATE TEMP TABLE id_alw3 (i int GENERATED ALWAYS AS IDENTITY);
2776+
CREATE RULE id_alw3_ins AS ON INSERT TO id_alw3 DO INSTEAD
2777+
WITH t1 AS (INSERT INTO id_alw1 DEFAULT VALUES RETURNING i)
2778+
INSERT INTO id_alw2_view DEFAULT VALUES RETURNING i;
2779+
CREATE TEMP VIEW id_alw3_view AS SELECT * FROM id_alw3;
2780+
CREATE TEMP TABLE id_alw4 (i int GENERATED ALWAYS AS IDENTITY);
2781+
WITH t4 AS (INSERT INTO id_alw4 DEFAULT VALUES RETURNING i)
2782+
INSERT INTO id_alw3_view DEFAULT VALUES RETURNING i;
2783+
i
2784+
---
2785+
1
2786+
(1 row)
2787+
2788+
SELECT * from id_alw1;
2789+
i
2790+
---
2791+
1
2792+
(1 row)
2793+
2794+
SELECT * from id_alw2;
2795+
i
2796+
---
2797+
1
2798+
(1 row)
2799+
2800+
SELECT * from id_alw3;
2801+
i
2802+
---
2803+
(0 rows)
2804+
2805+
SELECT * from id_alw4;
2806+
i
2807+
---
2808+
1
2809+
(1 row)
2810+
27702811
-- check case where CTE reference is removed due to optimization
27712812
EXPLAIN (VERBOSE, COSTS OFF)
27722813
SELECT q1 FROM

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

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

13201320
SELECT * FROM bug6051_3;
13211321

1322+
-- check that recursive CTE processing doesn't rewrite a CTE more than once
1323+
-- (must not try to expand GENERATED ALWAYS IDENTITY columns more than once)
1324+
CREATE TEMP TABLE id_alw1 (i int GENERATED ALWAYS AS IDENTITY);
1325+
1326+
CREATE TEMP TABLE id_alw2 (i int GENERATED ALWAYS AS IDENTITY);
1327+
CREATE TEMP VIEW id_alw2_view AS SELECT * FROM id_alw2;
1328+
1329+
CREATE TEMP TABLE id_alw3 (i int GENERATED ALWAYS AS IDENTITY);
1330+
CREATE RULE id_alw3_ins AS ON INSERT TO id_alw3 DO INSTEAD
1331+
WITH t1 AS (INSERT INTO id_alw1 DEFAULT VALUES RETURNING i)
1332+
INSERT INTO id_alw2_view DEFAULT VALUES RETURNING i;
1333+
CREATE TEMP VIEW id_alw3_view AS SELECT * FROM id_alw3;
1334+
1335+
CREATE TEMP TABLE id_alw4 (i int GENERATED ALWAYS AS IDENTITY);
1336+
1337+
WITH t4 AS (INSERT INTO id_alw4 DEFAULT VALUES RETURNING i)
1338+
INSERT INTO id_alw3_view DEFAULT VALUES RETURNING i;
1339+
1340+
SELECT * from id_alw1;
1341+
SELECT * from id_alw2;
1342+
SELECT * from id_alw3;
1343+
SELECT * from id_alw4;
1344+
13221345
-- check case where CTE reference is removed due to optimization
13231346
EXPLAIN (VERBOSE, COSTS OFF)
13241347
SELECT q1 FROM

0 commit comments

Comments
 (0)