Re: Allow batched insert during cross-partition updates

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Friday, March 12, 2021 3:45 AM, Amit Langote <amitlangote09(at)gmail(dot)com> wrote:

> On Thu, Mar 11, 2021 at 8:36 PM gkokolatos(at)pm(dot)me wrote:
>
> > On Thursday, March 11, 2021 9:42 AM, Amit Langote amitlangote09(at)gmail(dot)com wrote:
> >
> > > On Wed, Mar 10, 2021 at 9:30 PM Georgios gkokolatos(at)protonmail(dot)com wrote:
> > >
> > > > I continued looking a bit at the patch, yet I am either failing to see fix or I am
> > > > looking at the wrong thing. Please find attached a small repro of what my expectetions
> > > > were.
> > > > As you can see in the repro, I would expect the
> > > > UPDATE local_root_remote_partitions SET a = 2;
> > > > to move the tuples to remote_partition_2 on the same transaction.
> > > > However this is not the case, with or without the patch.
> > > > Is my expectation of this patch wrong?
> > >
> > > I think yes. We currently don't have the feature you are looking for
> > > -- moving tuples from one remote partition to another remote
> > > partition. This patch is not for adding that feature.
> >
> > Thank you for correcting me.
> >
> > > What we do support however is moving rows from a local partition to a
> > > remote partition and that involves performing an INSERT on the latter.
> > > This patch is for teaching those INSERTs to use batched mode if
> > > allowed, which is currently prohibited. So with this patch, if an
> > > UPDATE moves 10 rows from a local partition to a remote partition,
> > > then they will be inserted with a single INSERT command containing all
> > > 10 rows, instead of 10 separate INSERT commands.
> >
> > So, if I understand correctly then in my previously attached repro I
> > should have written instead:
> >
> > CREATE TABLE local_root_remote_partitions (a int) PARTITION BY LIST ( a );
> > CREATE TABLE
> > local_root_local_partition_1
> > PARTITION OF
> > local_root_remote_partitions FOR VALUES IN (1);
> >
> > CREATE FOREIGN TABLE
> > local_root_remote_partition_2
> > PARTITION OF
> > local_root_remote_partitions FOR VALUES IN (2)
> > SERVER
> > remote_server
> > OPTIONS (
> > table_name 'remote_partition_2',
> > batch_size '10'
> > );
> >
> > INSERT INTO local_root_remote_partitions VALUES (1), (1);
> > -- Everything should be on local_root_local_partition_1 and on the same transaction
> > SELECT ctid, xmin, xmax, cmax, tableoid::regclass, a FROM local_root_remote_partitions;
> >
> > UPDATE local_root_remote_partitions SET a = 2;
> > -- Everything should be on remote_partition_2 and on the same transaction
> > SELECT ctid, xmin, xmax, cmax, tableoid::regclass, a FROM local_root_remote_partitions;
> >
> >
> > I am guessing that I am still wrong because the UPDATE operation above will
> > fail due to the restrictions imposed in postgresBeginForeignInsert regarding
> > UPDATES.
>
> Yeah, for the move to work without hitting the restriction you
> mention, you will need to write the UPDATE query such that
> local_root_remote_partition_2 is not updated. For example, as
> follows:
>
> UPDATE local_root_remote_partitions SET a = 2 WHERE a <> 2;
>
> With this query, the remote partition is not one of the result
> relations to be updated, so is able to escape that restriction.

Excellent. Thank you for the explanation and patience.

>
> > Would it be too much to ask for the addition of a test case that will
> > demonstrate the change of behaviour found in patch.
>
> Hmm, I don't think there's a way to display whether the INSERT done on
> the remote partition as a part of an (tuple-moving) UPDATE used
> batching or not. That's because that INSERT's state is hidden from
> EXPLAIN. Maybe we should change EXPLAIN to make it show such hidden
> INSERT's state (especially its batch size) under the original UPDATE
> node, but I am not sure.

Yeah, there does not seem to be a way for explain to do show that information
with the current code.

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

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?

Cheers,
//Georgios

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