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

On Mon, Mar 1, 2021 at 12:38 PM Greg Nancarrow <gregn4422(at)gmail(dot)com> wrote:
> On Fri, Feb 26, 2021 at 5:50 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> >
> > On Fri, Feb 26, 2021 at 3:35 PM Greg Nancarrow <gregn4422(at)gmail(dot)com> wrote:
> > > On Fri, Feb 26, 2021 at 4:07 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> > > > The attached patch fixes this, although I am starting to have second
> > > > thoughts about how we're tracking partitions in this patch. Wondering
> > > > if we should bite the bullet and add partitions into the main range
> > > > table instead of tracking them separately in partitionOids, which
> > > > might result in a cleaner patch overall.
> > >
> > > Thanks Amit,
> > >
> > > I was able to reproduce the problem using your instructions (though I
> > > found I had to run that explain an extra time, in order to hit the
> > > breakpoint).
> > > Also, I can confirm that the problem doesn't occur after application
> > > of your patch.
> > > I'll leave it to your better judgement as to what to do next - if you
> > > feel the current tracking method is not sufficient
> >
> > Just to be clear, I think the tracking method added by the patch is
> > sufficient AFAICS for the problems we were able to discover. The
> > concern I was trying to express is that we seem to be duct-taping
> > holes in our earlier chosen design to track partitions separately from
> > the range table. If we had decided to add partitions to the range
> > table as "extra" target relations from the get-go, both the issues I
> > mentioned with cached plans -- partitions not being counted as a
> > dependency and partitions not being locked before execution -- would
> > have been prevented. I haven't fully grasped how invasive that design
> > would be, but it sure sounds like it would be a bit more robust.
>
> Posting an updated set of patches that includes Amit Langote's patch
> to the partition tracking scheme...

Thanks Greg.

> (the alternative of adding partitions to the range table needs further
> investigation)

I looked at this today with an intention to write and post a PoC
patch, but was quickly faced with the task of integrating that design
with how ModifyTable works for inserts. Specifically if, in addition
to adding partitions to the range table, we also their RT indexes to
ModifyTable.resultRelations, then maybe we'll need to rethink our
executor tuple routing code. That code currently tracks the insert's
target partitions separately from the range table, exactly because
they are not there to begin with. So if we change the latter as in
this hypothetical PoC, maybe we should also revisit the former. Also,
it would not be great that the planner's arrays in PlannerInfo would
get longer as a result of Query.rtable getting longer by adding
partitions, thus making all the loops over those arrays slower for no
benefit.

So, let's do this in a follow-on patch, if at all, instead of
considering this topic a blocker for committing the current set of
patches.

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