Re: Allow batched insert during cross-partition updates

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.

> 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.

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