Re: Parallel INSERT (INTO ... SELECT ...)

On Fri, Feb 5, 2021 at 6:56 PM Greg Nancarrow <gregn4422@gmail.com> wrote:
> On Fri, Feb 5, 2021 at 8:07 PM Amit Langote <amitlangote09@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