On Fri, Feb 5, 2021 at 11:01 PM Greg Nancarrow <gregn4422@gmail.com> wrote:
> On Fri, Feb 5, 2021 at 11:12 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > That seems good enough as far as I am concerned. Although either an
> > Assert as follows or a comment why the if (sub_action_ptr) is needed
> > seems warranted.
> >
> > if (sub_action_ptr)
> > rule_action->hasModifyingCTE = true;
> > else
> > Assert(sub_action == rule_action);
> >
> > Does the Assert seem overly confident?
>
> No, the Assert is exactly right, and I'll add a comment too.
> See below.
> I'll post the patch separately, if you can't see any further issues.
>
> diff --git a/src/backend/rewrite/rewriteHandler.c
> b/src/backend/rewrite/rewriteHandler.c
> index 0672f497c6..05b80bd347 100644
> --- a/src/backend/rewrite/rewriteHandler.c
> +++ b/src/backend/rewrite/rewriteHandler.c
> @@ -557,6 +557,21 @@ rewriteRuleAction(Query *parsetree,
> /* OK, it's safe to combine the CTE lists */
> sub_action->cteList = list_concat(sub_action->cteList,
> copyObject(parsetree->cteList));
> +
> + /*
> + * If the hasModifyingCTE flag is set in the source parsetree from
> + * which the CTE list is copied, the flag needs to be set in the
> + * sub_action and, if applicable, in the rule_action (INSERT...SELECT
> + * case).
> + */
> + if (parsetree->hasModifyingCTE)
> + {
> + sub_action->hasModifyingCTE = true;
> + if (sub_action_ptr)
> + rule_action->hasModifyingCTE = true;
> + else
> + Assert(sub_action == rule_action);
> + }
> }
LGTM, thank you.
--
Amit Langote
EDB: http://www.enterprisedb.com
