Propagate CTE property flags when copying a CTE list into a rule.

tglsfdc · tglsfdc · commit 4384eccb1549 · 2021-02-06T19:28:39.000-05:00
rewriteRuleAction() neglected this step, although it was careful to propagate other similar flags such as hasSubLinks or hasRowSecurity. Omitting to transfer hasRecursive is just cosmetic at the moment, but omitting hasModifyingCTE is a live bug, since the executor certainly looks at that. The proposed test case only fails back to v10, but since the executor examines hasModifyingCTE in 9.x as well, I suspect that a test case could be devised that fails in older branches. Given the nearness of the release deadline, though, I'm not going to spend time looking for a better test. Report and patch by Greg Nancarrow, cosmetic changes by me 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
@@ -504,6 +504,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)
 		{
@@ -525,6 +528,9 @@ 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;
 	}
 
 	/*

Original file line number	Diff line number	Diff line change
`@@ -504,6 +504,9 @@ rewriteRuleAction(Query *parsetree,`
`504`	`504`	`*`
`505`	`505`	`* This could possibly be fixed by using some sort of internally`
`506`	`506`	`* generated ID, instead of names, to link CTE RTEs to their CTEs.`
	`507`	`+ * However, decompiling the results would be quite confusing; note the`
	`508`	`+ * merge of hasRecursive flags below, which could change the apparent`
	`509`	`+ * semantics of such redundantly-named CTEs.`
`507`	`510`	`*/`
`508`	`511`	`foreach(lc, parsetree->cteList)`
`509`	`512`	`{`
`@@ -525,6 +528,9 @@ rewriteRuleAction(Query *parsetree,`
`525`	`528`	`/* OK, it's safe to combine the CTE lists */`
`526`	`529`	`sub_action->cteList = list_concat(sub_action->cteList,`
`527`	`530`	`copyObject(parsetree->cteList));`
	`531`	`+ /* ... and don't forget about the associated flags */`
	`532`	`+ sub_action->hasRecursive \|= parsetree->hasRecursive;`
	`533`	`+ sub_action->hasModifyingCTE \|= parsetree->hasModifyingCTE;`
`528`	`534`	`}`
`529`	`535`
`530`	`536`	`/*`