On Fri, Jan 8, 2021 at 8:16 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
>
> - if (pcxt->nworkers_launched > 0)
> + if (pcxt->nworkers_launched > 0 && !(isParallelModifyLeader &&
> !isParallelModifyWithReturning))
> {
>
> I think this check could be simplified to if (pcxt->nworkers_launched
> > 0 && isParallelModifyWithReturning) or something like that.
>
Not quite. The existing check is correct, because it needs to account
for existing Parallel SELECT functionality (not just new Parallel
INSERT).
But I will re-write the test as an equivalent expression, so it's
hopefully more readable (taking into account Antonin's suggested
variable-name changes):
if (pcxt->nworkers_launched > 0 && (!isModify || isModifyWithReturning))
> >
> > @@ -252,6 +252,7 @@ set_plan_references(PlannerInfo *root, Plan *plan)
> > PlannerGlobal *glob = root->glob;
> > int rtoffset = list_length(glob->finalrtable);
> > ListCell *lc;
> > + Plan *finalPlan;
> >
> > /*
> > * Add all the query's RTEs to the flattened rangetable. The live ones
> > @@ -302,7 +303,17 @@ set_plan_references(PlannerInfo *root, Plan *plan)
> > }
> >
> > /* Now fix the Plan tree */
> > - return set_plan_refs(root, plan, rtoffset);
> > + finalPlan = set_plan_refs(root, plan, rtoffset);
> > + if (finalPlan != NULL && IsA(finalPlan, Gather))
> > + {
> > + Plan *subplan = outerPlan(finalPlan);
> > +
> > + if (IsA(subplan, ModifyTable) && castNode(ModifyTable, subplan)->returningLists != NULL)
> > + {
> > + finalPlan->targetlist = copyObject(subplan->targetlist);
> > + }
> > + }
> > + return finalPlan;
> > }
> >
> > I'm not sure if the problem of missing targetlist should be handled here (BTW,
> > NIL is the constant for an empty list, not NULL). Obviously this is a
> > consequence of the fact that the ModifyTable node has no regular targetlist.
> >
>
> I think it is better to add comments along with this change. In this
> form, this looks quite hacky to me.
>
The targetlist on the ModifyTable node has been setup correctly, but
it hasn't been propagated to the Gather node.
Of course, I have previously tried to elegantly fix this, but struck
various problems, using different approaches.
Perhaps this code could just be moved into set_plan_refs().
For the moment, I'll just keep the current code, but I'll add a FIXME
comment for this.
I'll investigate Antonin's suggestions as a lower-priority side-task.
Regards,
Greg Nancarrow
Fujitsu Australia