Fix erroneous parallel execution when modifying CTE is present in rewritten query
Hi Hackers,
There is a known bug in the query rewriter: if a query that has a modifying
CTE is re-written, the hasModifyingCTE flag is not getting set in the
re-written query. This bug can result in the query being allowed to execute
in parallel-mode, which results in an error.
For more details from a previous discussion about this, and a test case
that illustrates the issue, refer to:
/messages/by-id/CAJcOf-fAdj=nDKMsRhQzndm-O13NY4dL6xGcEvdX5Xvbbi0V7g@mail.gmail.com
As a proposal to fix this problem, I've attached a patch which:
1) Copies the associated hasModifyingCTE and hasRecursive flags when the
rewriter combines CTE lists (using Tom Lane's initial patch code seen in
[1]: ). This flag copying is missing from the current Postgres code. 2) Adds an error case to specifically disallow the case of applying an INSERT...SELECT rule action to a command with a data-modifying CTE. This is because in this case, the rewritten query will actually end up having a data-modifying CTE that is not at the top level (as it is associated with the SELECT subquery part), which is not actually allowed by Postgres if that query is entered normally (as it's the parser that contains the error-check to ensure that the modifying CTE is at the top level, so this case avoids detection in the rewriter). 3) Modifies the existing test case in with.sql that tests the merging of an outer CTE with a CTE in a rule action (as currently that rule action is using INSERT...SELECT).
2) Adds an error case to specifically disallow the case of applying an
INSERT...SELECT rule action to a command with a data-modifying CTE. This is
because in this case, the rewritten query will actually end up having a
data-modifying CTE that is not at the top level (as it is associated with
the SELECT subquery part), which is not actually allowed by Postgres if
that query is entered normally (as it's the parser that contains the
error-check to ensure that the modifying CTE is at the top level, so this
case avoids detection in the rewriter).
3) Modifies the existing test case in with.sql that tests the merging of an
outer CTE with a CTE in a rule action (as currently that rule action is
using INSERT...SELECT).
For the record, a workaround for this issue (at least addressing how
hasModifyingCTE is meant to exclude the query from parallel execution) has
been suggested in the past, but was not well received. It is the following
addition to the max_parallel_hazard_walker() function:
+ /*
+ * ModifyingCTE expressions are treated as parallel-unsafe.
+ *
+ * XXX Normally, if the Query has a modifying CTE, the
hasModifyingCTE
+ * flag is set in the Query tree, and the query will be
regarded as
+ * parallel-usafe. However, in some cases, a re-written query
with a
+ * modifying CTE does not have that flag set, due to a bug in
the query
+ * rewriter. The following else-if is a workaround for this
bug, to detect
+ * a modifying CTE in the query and regard it as
parallel-unsafe. This
+ * comment, and the else-if block immediately below, may be
removed once
+ * the bug in the query rewriter is fixed.
+ */
+ else if (IsA(node, CommonTableExpr))
+ {
+ CommonTableExpr *cte = (CommonTableExpr *)
node;
+ Query *ctequery = castNode(Query,
cte->ctequery);
+
+ if (ctequery->commandType != CMD_SELECT)
+ {
+ context->max_hazard =
PROPARALLEL_UNSAFE;
+ return true;
+ }
+ }
+Regards,
Greg Nancarrow
Fujitsu Australia
Attachments:
v1-0001-Propagate-CTE-property-flags-when-copying-a-CTE-list.patchapplication/octet-stream; name=v1-0001-Propagate-CTE-property-flags-when-copying-a-CTE-list.patchDownload
From b501bde271cc7786b781e1ba46540a1343acc548 Mon Sep 17 00:00:00 2001
From: Greg Nancarrow <gregn4422@gmail.com>
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
Greg Nancarrow <gregn4422@gmail.com> writes:
[ v1-0001-Propagate-CTE-property-flags-when-copying-a-CTE-list.patch ]
Pushed with a couple of adjustments:
* I rewrote the comment, mostly so as to include an explanation of how
the error could be removed, in case anyone ever wants to go to the
trouble.
* The existing test case can be fixed up without fundamentally changing
what it's testing, by replacing INSERT...SELECT with INSERT...VALUES.
(That should likely also be our first suggestion to any complainers.)
Thanks for the patch!
regards, tom lane