Re: Allow batched insert during cross-partition updates

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Tuesday, March 16, 2021 9:59 AM, Amit Langote <amitlangote09(at)gmail(dot)com> wrote:

> Hi Georgios,
>
> On Tue, Mar 16, 2021 at 5:12 PM gkokolatos(at)pm(dot)me wrote:
>
> > On Tuesday, March 16, 2021 6:13 AM, Amit Langote amitlangote09(at)gmail(dot)com wrote:
> >
> > > On Fri, Mar 12, 2021 at 7:59 PM gkokolatos(at)pm(dot)me wrote:
> > >
> > > > On Friday, March 12, 2021 3:45 AM, Amit Langote amitlangote09(at)gmail(dot)com wrote:
> > > >
> > > > > By the way, the test case added by commit 927f453a94106 does exercise
> > > > > the code added by this patch, but as I said in the last paragraph, we
> > > > > can't verify that by inspecting EXPLAIN output.
> > > >
> > > > I never doubted that. However, there is a difference. The current patch
> > > > changes the query to be executed in the remote from:
> > > > INSERT INTO <snip> VALUES ($1);
> > > > to:
> > > > INSERT INTO <snip> VALUES ($1), ($2) ... ($n);
> > > > When this patch gets in, it would be very helpful to know that subsequent
> > > > code changes will not cause regressions. So I was wondering if there is
> > > > a way to craft a test case that would break for the code in 927f453a94106
> > > > yet succeed with the current patch.
> > >
> > > The test case "works" both before and after the patch, with the
> > > difference being in the form of the remote query. It seems to me
> > > though that you do get that.
> > >
> > > > I attach version 2 of my small reproduction. I am under the impression that
> > > > in this version, examining the value of cmin in the remote table should
> > > > give an indication of whether the remote received a multiple insert queries
> > > > with a single value, or a single insert query with multiple values.
> > > > Or is this a wrong assumption of mine?
> > >
> > > No, I think you have a good idea here.
> > > I've adjusted that test case to confirm that the batching indeed works
> > > by checking cmin of the moved rows, as you suggest. Please check the
> > > attached updated patch.
> >
> > Excellent. The patch in the current version with the added test seems
> > ready to me.
>
> Thanks for quickly checking that.

A pleasure.

>
> > I would still vote to have accessor functions instead of exposing the
> > whole PartitionTupleRouting struct, but I am not going to hold a too
> > strong stance about it.
>
> I as well, although I would wait for others to chime in before
> updating the patch that way.

Fair enough.

Status updated to RfC in the commitfest app.

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