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

On Fri, Feb 5, 2021 at 2:55 PM Greg Nancarrow <gregn4422(at)gmail(dot)com> wrote:
> On Fri, Feb 5, 2021 at 4:25 PM Hou, Zhijie <houzj(dot)fnst(at)cn(dot)fujitsu(dot)com> wrote:
> > > > That is very close to what I was going to suggest, which is this:
> > > >
> > > > diff --git a/src/backend/rewrite/rewriteHandler.c
> > > > b/src/backend/rewrite/rewriteHandler.c
> > > > index 0672f497c6..3c4417af98 100644
> > > > --- a/src/backend/rewrite/rewriteHandler.c
> > > > +++ b/src/backend/rewrite/rewriteHandler.c
> > > > @@ -631,6 +631,8 @@ rewriteRuleAction(Query *parsetree,
> > > > checkExprHasSubLink((Node *)
> > > > rule_action->returningList);
> > > > }
> > > >
> > > > + rule_action->hasModifyingCTE |= parsetree->hasModifyingCTE;
> > > > +
> > > > return rule_action;
> > > > }
> > >
> > >
> > > if (parsetree->cteList != NIL && sub_action->commandType !=
> > > CMD_UTILITY)
> > > {
> > > ...
> > > sub_action->cteList = list_concat(sub_action->cteList,
> > > }
> > >
> > > Is is possible when sub_action is CMD_UTILITY ?
> > > In this case CTE will be copied to the newone, should we set the set the
> > > flag in this case ?
> >
> > Sorry , a typo in my word.
> > In this case CTE will not be copied to the newone, should we set the set the flag in this case ?
> >
>
> No, strictly speaking, we probably shouldn't, because the CTE wasn't
> copied in that case.

Right.

> Also, I know the bitwise OR "works" in this case, but I think some
> will frown on use of that for a bool.
> IMHO better to use:
>
> if (parsetree->hasModifyingCTE)
> rule_action->hasModifyingCTE = true;
>
> So patch might be something like:
>
> diff --git a/src/backend/rewrite/rewriteHandler.c
> b/src/backend/rewrite/rewriteHandler.c
> index 0672f497c6..a989e02925 100644
> --- a/src/backend/rewrite/rewriteHandler.c
> +++ b/src/backend/rewrite/rewriteHandler.c
> @@ -557,6 +557,8 @@ 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;
> }
>
> /*
> @@ -594,6 +596,9 @@ rewriteRuleAction(Query *parsetree,
> *sub_action_ptr = sub_action;
> else
> rule_action = sub_action;
> +
> + if (parsetree->hasModifyingCTE)
> + sub_action->hasModifyingCTE = true;
> }

That may be better.

BTW, the original query's cteList is copied into sub_action query but
not into rule_action for reasons I haven't looked very closely into,
even though we'd like to ultimately set the latter's hasModifyingCTE
to reflect the original query's, right? So we should do the following
at some point before returning:

if (sub_action->hasModifyingCTE)
rule_action->hasModifyingCTE = true;

> I'll do some further checks, because the rewriting is recursive and
> tricky, so don't want to miss any cases ...

Always a good idea.

--
Amit Langote
EDB: http://www.enterprisedb.com