On Fri, Feb 5, 2021 at 6:56 PM Greg Nancarrow <gregn4...@gmail.com> wrote:
> On Fri, Feb 5, 2021 at 8:07 PM Amit Langote <amitlangot...@gmail.com> wrote:
> > > This is one reason for my original approach (though I admit, it was
> > > not optimal) because at least it was reliable and detected the
> > > modifyingCTE after all the rewriting and kludgy code had finished.
> >
> > Yeah it's hard to go through all of this highly recursive legacy code
> > to be sure that hasModifyingCTE is consistent with reality in *all*
> > cases, but let's try to do it. No other has* flags are set
> > after-the-fact, so I wouldn't bet on a committer letting this one
> > through.
>
> I have debugged the code a bit more now, and the following patch seems
> to correctly fix the issue, at least for the known test cases.
> (i.e. SELECT case, shared by houzj, and the INSERT...SELECT case, as
> in the "with" regression tests, for which I originally detected the
> issue)
>
> diff --git a/src/backend/rewrite/rewriteHandler.c
> b/src/backend/rewrite/rewriteHandler.c
> index 0672f497c6..8f695b32ec 100644
> --- a/src/backend/rewrite/rewriteHandler.c
> +++ b/src/backend/rewrite/rewriteHandler.c
> @@ -557,6 +557,12 @@ rewriteRuleAction(Query *parsetree,
> /* OK, it's safe to combine the CTE lists */
> sub_action->cteList = list_concat(sub_action->cteList,
> copyObject(parsetree->cteList));
> + if (parsetree->hasModifyingCTE)
> + {
> + sub_action->hasModifyingCTE = true;
> + if (sub_action_ptr)
> + rule_action->hasModifyingCTE = true;
> + }
> }
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?
--
Amit Langote
EDB: http://www.enterprisedb.com
