Re: Parallel Inserts in CREATE TABLE AS

On Mon, Dec 14, 2020 at 4:06 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Thu, Dec 10, 2020 at 7:20 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> > On Thu, Dec 10, 2020 at 5:19 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > > > > > +               allow = ps && IsA(ps, GatherState) && !ps->ps_ProjInfo &&
> > > > > > +                               plannedstmt->parallelModeNeeded &&
> > > > > > +                               plannedstmt->planTree &&
> > > > > > +                               IsA(plannedstmt->planTree, Gather) &&
> > > > > > +                               plannedstmt->planTree->lefttree &&
> > > > > > +                               plannedstmt->planTree->lefttree->parallel_aware &&
> > > > > > +                               plannedstmt->planTree->lefttree->parallel_safe;
> > > > > >
> > > > > > I noticed it check both IsA(ps, GatherState) and IsA(plannedstmt->planTree, Gather).
> > > > > > Does it mean it is possible that IsA(ps, GatherState) is true but IsA(plannedstmt->planTree, Gather) is
false? 
> > > > > >
> > > > > > I did some test but did not find a case like that.
> > > > > >
> > > > > This seems like an extra check.  Apart from that if we combine 0001
> > > > > and 0002 there should be an additional protection so that it should
> > > > > not happen that in cost_gather we have ignored the parallel tuple cost
> > > > > and now we are rejecting the parallel insert. Probably we should add
> > > > > an assert.
> > > >
> > > > Yeah it's an extra check. I don't think we need that extra check IsA(plannedstmt->planTree, Gather).
GatherStatecheck is enough. I verified it as follows: the gatherstate will be allocated and initialized with the plan
treein ExecInitGather which are the ones we are checking here. So, there is no chance that the plan state is
GatherStateand the plan tree will not be Gather.  I will remove IsA(plannedstmt->planTree, Gather) check in the next
versionof the patch set. 
> > > >
> > > > Breakpoint 4, ExecInitGather (node=0x5647f98ae994 <ExecCheckRTEPerms+131>, estate=0x1ca8, eflags=730035099) at
nodeGather.c:61
> > > > (gdb) p gatherstate
> > > > $10 = (GatherState *) 0x5647fac83850
> > > > (gdb) p gatherstate->ps.plan
> > > > $11 = (Plan *) 0x5647fac918a0
> > > >
> > > > Breakpoint 1, IsParallelInsertInCTASAllowed (into=0x5647fac97580, queryDesc=0x5647fac835e0) at createas.c:663
> > > > 663     {
> > > > (gdb) p ps
> > > > $13 = (PlanState *) 0x5647fac83850
> > > > (gdb) p ps->plan
> > > > $14 = (Plan *) 0x5647fac918a0
> > > >
> > > Hope you did not miss the second part of my comment
> > > "
> > > > Apart from that if we combine 0001
> > > > and 0002 there should be additional protection so that it should
> > > > not happen that in cost_gather we have ignored the parallel tuple cost
> > > > and now we are rejecting the parallel insert. Probably we should add
> > > > an assert.
> > > "
> >
> > IIUC, we need to set a flag in cost_gather(in 0002 patch) whenever we
> > ignore the parallel tuple cost and while checking to allow or disallow
> > parallel inserts in IsParallelInsertInCTASAllowed(), we need to add an
> > assert something like Assert(cost_ignored_in_cost_gather && allow)
> > before return allow;
> >
> > This assertion fails 1) either if we have not ignored the cost but
> > allowing parallel inserts 2) or we ignored the cost but not allowing
> > parallel inserts.
> >
> > 1) seems to be fine, we can go ahead and perform parallel inserts. 2)
> > is the concern that the planner would have wrongly chosen the parallel
> > plan, but in this case also isn't it better to go ahead with the
> > parallel plan instead of failing the query?
> >
> > +        /*
> > +         * We allow parallel inserts by the workers only if the Gather node has
> > +         * no projections to perform and if the upper node is Gather. In case,
> > +         * the Gather node has projections, which is possible if there are any
> > +         * subplans in the query, the workers can not do those projections. And
> > +         * when the upper node is GatherMerge, then the leader has to perform
> > +         * the final phase i.e. merge the results by workers.
> > +         */
> > +        allow = ps && IsA(ps, GatherState) && !ps->ps_ProjInfo &&
> > +                plannedstmt->parallelModeNeeded &&
> > +                plannedstmt->planTree &&
> > +                plannedstmt->planTree->lefttree &&
> > +                plannedstmt->planTree->lefttree->parallel_aware &&
> > +                plannedstmt->planTree->lefttree->parallel_safe;
> > +
> > +        return allow;
> > +    }
>
> I added the assertion into the 0002 patch so that it fails when the
> planner ignores parallel tuple cost and may choose parallel plan but
> later we don't allow parallel inserts. make check and make check-world
> passeses without any assertion failures.
>
> Attaching v11 patch set. Please review it further.

I can see a lot of unrelated changes in 0002, or you have done a lot
of code refactoring especially in createas.c file.  If it is intended
refactoring then please move the refactoring to a separate patch so
that the patch is readable.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com