From b501bde271cc7786b781e1ba46540a1343acc548 Mon Sep 17 00:00:00 2001 From: Greg Nancarrow Date: Fri, 27 Aug 2021 12:29:09 +1000 Subject: [PATCH] Propagate CTE property flags when copying a CTE list into a rule. rewriteRuleAction() neglected to copy the hasModifyingCTE flag, which meant it was possible for some rewritten queries to be erroneously executed in parallel mode. The hasRecursive flag was also not copied, though that is just cosmetic at the moment. rewriteRuleAction() also allowed a rewritten query to contain a data-modifying CTE at the subquery level (i.e., not at the top level), in the case of an INSERT...SELECT rule action applied to a command with a data-modifying CTE. As such queries are normally not allowed by Postgres (disallowed by the parser), this patch updates the code to disallow it in rewritten queries too, and updates one existing test case affected by this. New test cases are added for the new error case, and to confirm that the hasModifyingCTE flag is propagated by query rewriting. Discussion: https://postgr.es/m/CAJcOf-fAdj=nDKMsRhQzndm-O13NY4dL6xGcEvdX5Xvbbi0V7g@mail.gmail.com --- src/backend/rewrite/rewriteHandler.c | 18 ++++++++++ src/test/regress/expected/with.out | 53 ++++++++++++++++++++++++++-- src/test/regress/sql/with.sql | 31 ++++++++++++++-- 3 files changed, 98 insertions(+), 4 deletions(-) diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c index 54fd6d6fb2..9310905c85 100644 --- a/src/backend/rewrite/rewriteHandler.c +++ b/src/backend/rewrite/rewriteHandler.c @@ -535,6 +535,9 @@ rewriteRuleAction(Query *parsetree, * * This could possibly be fixed by using some sort of internally * generated ID, instead of names, to link CTE RTEs to their CTEs. + * However, decompiling the results would be quite confusing; note the + * merge of hasRecursive flags below, which could change the apparent + * semantics of such redundantly-named CTEs. */ foreach(lc, parsetree->cteList) { @@ -556,6 +559,21 @@ rewriteRuleAction(Query *parsetree, /* OK, it's safe to combine the CTE lists */ sub_action->cteList = list_concat(sub_action->cteList, copyObject(parsetree->cteList)); + /* ... and don't forget about the associated flags */ + sub_action->hasRecursive |= parsetree->hasRecursive; + sub_action->hasModifyingCTE |= parsetree->hasModifyingCTE; + + /* + * Queries resulting from INSERT...SELECT rule actions that are + * applied to commands with data-modifying CTEs end up having a + * data-modifying CTE at the subquery level - such querues are not + * normally allowed, so we shouldn't allow the query rewriter to + * produce them either. + */ + if (sub_action->hasModifyingCTE && rule_action != sub_action) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("INSERT...SELECT rule actions are not supported on events having data-modifying statements in WITH"))); } /* diff --git a/src/test/regress/expected/with.out b/src/test/regress/expected/with.out index 3523a7dcc1..723c906716 100644 --- a/src/test/regress/expected/with.out +++ b/src/test/regress/expected/with.out @@ -2380,10 +2380,23 @@ SELECT * FROM bug6051; CREATE TEMP TABLE bug6051_2 (i int); CREATE RULE bug6051_ins AS ON INSERT TO bug6051 DO INSTEAD - INSERT INTO bug6051_2 - SELECT NEW.i; +WITH t0 AS ( INSERT INTO bug6051_2 SELECT * FROM bug6051 RETURNING * ) + SELECT * FROM t0; WITH t1 AS ( DELETE FROM bug6051 RETURNING * ) INSERT INTO bug6051 SELECT * FROM t1; + i +--- + 1 + 2 + 3 + 1 + 2 + 3 + 1 + 2 + 3 +(9 rows) + SELECT * FROM bug6051; i --- @@ -2397,6 +2410,42 @@ SELECT * FROM bug6051_2; 3 (3 rows) +DROP RULE bug6051_ins ON bug6051; +-- check INSERT...SELECT rule actions are disallowed on commands +-- that have modifyingCTEs +CREATE RULE bug6051_ins_2 AS ON INSERT TO bug6051 DO INSTEAD + INSERT INTO bug6051_2 + SELECT NEW.i; +WITH t1 AS ( DELETE FROM bug6051 RETURNING * ) +INSERT INTO bug6051 SELECT * FROM t1; +ERROR: INSERT...SELECT rule actions are not supported on events having data-modifying statements in WITH +-- silly example to verify that hasModifyingCTE flag is propagated +CREATE TEMP TABLE bug6051_3 AS + select a from generate_series(11,13) as a; +CREATE RULE bug6051_3_ins AS ON INSERT TO bug6051_3 DO INSTEAD + SELECT i FROM bug6051_2; +BEGIN; SET LOCAL force_parallel_mode = on; +WITH t1 AS ( DELETE FROM bug6051_3 RETURNING * ) + INSERT INTO bug6051_3 SELECT * FROM t1; + i +--- + 1 + 2 + 3 + 1 + 2 + 3 + 1 + 2 + 3 +(9 rows) + +COMMIT; +SELECT * FROM bug6051_3; + a +--- +(0 rows) + -- a truly recursive CTE in the same list WITH RECURSIVE t(a) AS ( SELECT 0 diff --git a/src/test/regress/sql/with.sql b/src/test/regress/sql/with.sql index 8b213ee408..c172439ab7 100644 --- a/src/test/regress/sql/with.sql +++ b/src/test/regress/sql/with.sql @@ -1117,8 +1117,8 @@ SELECT * FROM bug6051; CREATE TEMP TABLE bug6051_2 (i int); CREATE RULE bug6051_ins AS ON INSERT TO bug6051 DO INSTEAD - INSERT INTO bug6051_2 - SELECT NEW.i; +WITH t0 AS ( INSERT INTO bug6051_2 SELECT * FROM bug6051 RETURNING * ) + SELECT * FROM t0; WITH t1 AS ( DELETE FROM bug6051 RETURNING * ) INSERT INTO bug6051 SELECT * FROM t1; @@ -1126,6 +1126,33 @@ INSERT INTO bug6051 SELECT * FROM t1; SELECT * FROM bug6051; SELECT * FROM bug6051_2; +DROP RULE bug6051_ins ON bug6051; + +-- check INSERT...SELECT rule actions are disallowed on commands +-- that have modifyingCTEs +CREATE RULE bug6051_ins_2 AS ON INSERT TO bug6051 DO INSTEAD + INSERT INTO bug6051_2 + SELECT NEW.i; + +WITH t1 AS ( DELETE FROM bug6051 RETURNING * ) +INSERT INTO bug6051 SELECT * FROM t1; + +-- silly example to verify that hasModifyingCTE flag is propagated +CREATE TEMP TABLE bug6051_3 AS + select a from generate_series(11,13) as a; + +CREATE RULE bug6051_3_ins AS ON INSERT TO bug6051_3 DO INSTEAD + SELECT i FROM bug6051_2; + +BEGIN; SET LOCAL force_parallel_mode = on; + +WITH t1 AS ( DELETE FROM bug6051_3 RETURNING * ) + INSERT INTO bug6051_3 SELECT * FROM t1; + +COMMIT; + +SELECT * FROM bug6051_3; + -- a truly recursive CTE in the same list WITH RECURSIVE t(a) AS ( SELECT 0 -- 2.27.0