Fix rewriter to set hasModifyingCTE correctly on rewritten queries.

If we copy data-modifying CTEs from the original query to a replacement
query (from a DO INSTEAD rule), we must set hasModifyingCTE properly
in the replacement query.  Failure to do this can cause various
unpleasantness, such as unsafe usage of parallel plans.  The code also
neglected to propagate hasRecursive, though that's only cosmetic at
the moment.

A difficulty arises if the rule action is an INSERT...SELECT.  We
attach the original query's RTEs and CTEs to the sub-SELECT Query, but
data-modifying CTEs are only allowed to appear in the topmost Query.
For the moment, throw an error in such cases.  It would probably be
possible to avoid this error by attaching the CTEs to the top INSERT
Query instead; but that would require a bunch of new code to adjust
ctelevelsup references.  Given the narrowness of the use-case, and
the need to back-patch this fix, it does not seem worth the trouble
for now.  We can revisit this if we get field complaints.

Per report from Greg Nancarrow.  Back-patch to all supported branches.
(The test case added here does not fail before v10, but there are
plenty of places checking top-level hasModifyingCTE in 9.6, so I have
no doubt that this code change is necessary there too.)

Greg Nancarrow and Tom Lane

Discussion: https://postgr.es/m/CAJcOf-f68DT=26YAMz_i0+Au3TcLO5oiHY5=fL6Sfuits6r+_w@mail.gmail.com
Discussion: https://postgr.es/m/CAJcOf-fAdj=nDKMsRhQzndm-O13NY4dL6xGcEvdX5Xvbbi0V7g@mail.gmail.com

diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 8f3470987abcce5620bdcab73c8903ab6432861d..45399aa5b8694128eed0dcfdc92a84df926accaa 100644 (file)
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -511,6 +511,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)
        {
@@ -532,6 +535,26 @@ 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;
+
+       /*
+        * If rule_action is different from sub_action (i.e., the rule action
+        * is an INSERT...SELECT), then we might have just added some
+        * data-modifying CTEs that are not at the top query level.  This is
+        * disallowed by the parser and we mustn't generate such trees here
+        * either, so throw an error.
+        *
+        * Conceivably such cases could be supported by attaching the original
+        * query's CTEs to rule_action not sub_action.  But to do that, we'd
+        * have to increment ctelevelsup in RTEs and SubLinks copied from the
+        * original query.  For now, it doesn't seem worth the trouble.
+        */
+       if (sub_action->hasModifyingCTE && rule_action != sub_action)
+           ereport(ERROR,
+                   (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                    errmsg("INSERT...SELECT rule actions are not supported for queries having data-modifying statements in WITH")));
    }
 
    /*

diff --git a/src/test/regress/expected/with.out b/src/test/regress/expected/with.out
index c9cac36089037d8c4364e5730f298dd6aa70bae5..956da9afbce077472a661325b0bca4b41741d27f 100644 (file)
--- a/src/test/regress/expected/with.out
+++ b/src/test/regress/expected/with.out
@@ -1644,7 +1644,7 @@ 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;
+ VALUES(NEW.i);
 WITH t1 AS ( DELETE FROM bug6051 RETURNING * )
 INSERT INTO bug6051 SELECT * FROM t1;
 SELECT * FROM bug6051;
@@ -1660,6 +1660,41 @@ SELECT * FROM bug6051_2;
  3
 (3 rows)
 
+-- check INSERT...SELECT rule actions are disallowed on commands
+-- that have modifyingCTEs
+CREATE OR REPLACE RULE bug6051_ins 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 for queries 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 8bb9817e7ce3107b7df7b3dc54ca0e8d9f707363..2fa867a25194f57f1cacb0404c92cbbcf8c30a95 100644 (file)
--- a/src/test/regress/sql/with.sql
+++ b/src/test/regress/sql/with.sql
@@ -756,7 +756,7 @@ 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;
+ VALUES(NEW.i);
 
 WITH t1 AS ( DELETE FROM bug6051 RETURNING * )
 INSERT INTO bug6051 SELECT * FROM t1;
@@ -764,6 +764,31 @@ INSERT INTO bug6051 SELECT * FROM t1;
 SELECT * FROM bug6051;
 SELECT * FROM bug6051_2;
 
+-- check INSERT...SELECT rule actions are disallowed on commands
+-- that have modifyingCTEs
+CREATE OR REPLACE RULE bug6051_ins 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