Horton

Thread

Commits

  1. Allow batching of inserts during cross-partition updates.

  2. Fix handling of pending inserts in nodeModifyTable.c.

  3. Postpone some stuff out of ExecInitModifyTable.

  4. Fix tuple routing to initialize batching only for inserts

  1. Allow batched insert during cross-partition updates

    Amit Langote <amitlangote09@gmail.com> — 2021-02-18T09:52:36Z

    Based on the discussion at:

https://www.postgresql.org/message-id/6929d485-2d2a-da46-3681-4a400a3d794f%40enterprisedb.com

I'm posting the patch for $subject here in this new thread and I'll
add it to the next CF per Tomas' advice.

With 927f453a94106 committed earlier today, we limit insert batching
only to the cases where the query's main command is also insert,
because allowing it to be used in other cases can hit some limitations
of the current code.

One such case is cross-partition updates of a partitioned table which
internally uses insert.  postgres_fdw supports some cases where a row
is moved from a local partition to a foreign partition.  When doing
so, the moved row is inserted into the latter, but those inserts can't
use batching due to the aforementioned commit.

As described in the thread linked above, to make batching possible for
those internal inserts, we'd need to make some changes to both the
core code and postgres_fdw, which the attached patch implements.
Details are in the commit message.

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

  2. Re: Allow batched insert during cross-partition updates

    Amit Langote <amitlangote09@gmail.com> — 2021-02-18T09:54:44Z

    On Thu, Feb 18, 2021 at 6:52 PM Amit Langote <amitlangote09@gmail.com> wrote:
>
> Based on the discussion at:
>
> https://www.postgresql.org/message-id/6929d485-2d2a-da46-3681-4a400a3d794f%40enterprisedb.com
>
> I'm posting the patch for $subject here in this new thread and I'll
> add it to the next CF per Tomas' advice.

Done: https://commitfest.postgresql.org/32/2992/

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

  3. Re: Allow batched insert during cross-partition updates

    Georgios <gkokolatos@protonmail.com> — 2021-03-09T15:53:09Z

    Hi,

thanks for the patch. I had a first look and played around with the code.

The code seems clean, complete, and does what it says on the tin. I will
need a bit more time to acclimatise with all the use cases for a more
thorough review.

I small question though is why expose PartitionTupleRouting and not add
a couple of functions to get the necessary info? If I have read the code
correctly the only members actually needed to be exposed are num_partitions
and partitions. Not a critique, I am just curious.

Cheers,
//Georgios

  4. Re: Allow batched insert during cross-partition updates

    Georgios <gkokolatos@protonmail.com> — 2021-03-10T12:23:30Z

    Hi,

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Thursday, February 18, 2021 10:54 AM, Amit Langote <amitlangote09@gmail.com> wrote:

> On Thu, Feb 18, 2021 at 6:52 PM Amit Langote amitlangote09@gmail.com wrote:
>
> > Based on the discussion at:
> > https://www.postgresql.org/message-id/6929d485-2d2a-da46-3681-4a400a3d794f%40enterprisedb.com
> > I'm posting the patch for $subject here in this new thread and I'll
> > add it to the next CF per Tomas' advice.
>
> Done:https://commitfest.postgresql.org/32/2992/
>
> --------------------------------------------------

apparently I did not receive the review comment I sent via the commitfest app.
Apologies for the chatter. Find the message-id here:



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

  5. Re: Allow batched insert during cross-partition updates

    Georgios <gkokolatos@protonmail.com> — 2021-03-10T12:30:08Z

    
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Wednesday, March 10, 2021 1:23 PM, Georgios <gkokolatos@protonmail.com> wrote:

>
>
> Hi,
>
> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> On Thursday, February 18, 2021 10:54 AM, Amit Langote amitlangote09@gmail.com wrote:
>
> > On Thu, Feb 18, 2021 at 6:52 PM Amit Langote amitlangote09@gmail.com wrote:
> >
> > > Based on the discussion at:
> > > https://www.postgresql.org/message-id/6929d485-2d2a-da46-3681-4a400a3d794f%40enterprisedb.com
> > > I'm posting the patch for $subject here in this new thread and I'll
> > > add it to the next CF per Tomas' advice.
> >
> > Done:https://commitfest.postgresql.org/32/2992/
>
> apparently I did not receive the review comment I sent via the commitfest app.
> Apologies for the chatter. Find the message-id here:
https://www.postgresql.org/message-id/161530518971.29967.9368488207318158252.pgcf%40coridan.postgresql.org

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?

Cheers,
//Georgios

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

  6. Re: Allow batched insert during cross-partition updates

    Amit Langote <amitlangote09@gmail.com> — 2021-03-11T08:09:06Z

    Hi Georgios,

On Wed, Mar 10, 2021 at 12:54 AM Georgios Kokolatos
<gkokolatos@protonmail.com> wrote:
>
> Hi,
>
> thanks for the patch. I had a first look and played around with the code.
>
> The code seems clean, complete, and does what it says on the tin. I will
> need a bit more time to acclimatise with all the use cases for a more
> thorough review.

Thanks for checking.

> I small question though is why expose PartitionTupleRouting and not add
> a couple of functions to get the necessary info? If I have read the code
> correctly the only members actually needed to be exposed are num_partitions
> and partitions. Not a critique, I am just curious.

I had implemented accessor functions in an earlier unposted version of
the patch, but just exposing PartitionTupleRouting does not sound so
harmful, so I switched to that approach.  Maybe if others agree with
you that accessor functions would be better, I will change the patch
that way.

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

  7. Re: Allow batched insert during cross-partition updates

    Amit Langote <amitlangote09@gmail.com> — 2021-03-11T08:42:43Z

    Hi Georgios,

On Wed, Mar 10, 2021 at 9:30 PM Georgios <gkokolatos@protonmail.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.

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.

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

  8. Re: Allow batched insert during cross-partition updates

    gkokolatos@pm.me — 2021-03-11T11:36:28Z

    



‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Thursday, March 11, 2021 9:42 AM, Amit Langote <amitlangote09@gmail.com> wrote:

> Hi Georgios,
>
> On Wed, Mar 10, 2021 at 9:30 PM Georgios gkokolatos@protonmail.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.

Would it be too much to ask for the addition of a test case that will
demonstrate the change of behaviour found in patch?

Cheers,
//Georgios

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

  9. Re: Allow batched insert during cross-partition updates

    Amit Langote <amitlangote09@gmail.com> — 2021-03-12T02:45:50Z

    On Thu, Mar 11, 2021 at 8:36 PM <gkokolatos@pm.me> wrote:
> On Thursday, March 11, 2021 9:42 AM, Amit Langote <amitlangote09@gmail.com> wrote:
> > On Wed, Mar 10, 2021 at 9:30 PM Georgios gkokolatos@protonmail.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.

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

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.


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

  10. Re: Allow batched insert during cross-partition updates

    gkokolatos@pm.me — 2021-03-12T10:59:19Z

    



‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Friday, March 12, 2021 3:45 AM, Amit Langote <amitlangote09@gmail.com> wrote:

> On Thu, Mar 11, 2021 at 8:36 PM gkokolatos@pm.me wrote:
>
> > On Thursday, March 11, 2021 9:42 AM, Amit Langote amitlangote09@gmail.com wrote:
> >
> > > On Wed, Mar 10, 2021 at 9:30 PM Georgios gkokolatos@protonmail.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

  11. Re: Allow batched insert during cross-partition updates

    Amit Langote <amitlangote09@gmail.com> — 2021-03-16T05:13:50Z

    Hi Georgios,

On Fri, Mar 12, 2021 at 7:59 PM <gkokolatos@pm.me> wrote:
> On Friday, March 12, 2021 3:45 AM, Amit Langote <amitlangote09@gmail.com> wrote:
> > On Thu, Mar 11, 2021 at 8:36 PM gkokolatos@pm.me wrote:
> > > On Thursday, March 11, 2021 9:42 AM, Amit Langote amitlangote09@gmail.com wrote:
> > > > 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.

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.
--
Amit Langote
EDB: http://www.enterprisedb.com

  12. Re: Allow batched insert during cross-partition updates

    gkokolatos@pm.me — 2021-03-16T08:11:57Z

    



‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Tuesday, March 16, 2021 6:13 AM, Amit Langote <amitlangote09@gmail.com> wrote:

> Hi Georgios,
>
> On Fri, Mar 12, 2021 at 7:59 PM gkokolatos@pm.me wrote:
>
> > On Friday, March 12, 2021 3:45 AM, Amit Langote amitlangote09@gmail.com wrote:
> >
> > > On Thu, Mar 11, 2021 at 8:36 PM gkokolatos@pm.me wrote:
> > >
> > > > On Thursday, March 11, 2021 9:42 AM, Amit Langote amitlangote09@gmail.com wrote:
> > > >
> > > > > 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.
>
> 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.

Thank you.

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

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.

If you agree with me, please provide an updated version of the patch.
Otherwise let it be known and I will flag the patch as RfC in the
commitfest app.

Cheers,
//Georgios

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

  13. Re: Allow batched insert during cross-partition updates

    Amit Langote <amitlangote09@gmail.com> — 2021-03-16T08:59:39Z

    Hi Georgios,

On Tue, Mar 16, 2021 at 5:12 PM <gkokolatos@pm.me> wrote:
> On Tuesday, March 16, 2021 6:13 AM, Amit Langote <amitlangote09@gmail.com> wrote:
> > On Fri, Mar 12, 2021 at 7:59 PM gkokolatos@pm.me wrote:
> > > On Friday, March 12, 2021 3:45 AM, Amit Langote amitlangote09@gmail.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

  14. Re: Allow batched insert during cross-partition updates

    gkokolatos@pm.me — 2021-03-16T09:13:51Z

    



‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Tuesday, March 16, 2021 9:59 AM, Amit Langote <amitlangote09@gmail.com> wrote:

> Hi Georgios,
>
> On Tue, Mar 16, 2021 at 5:12 PM gkokolatos@pm.me wrote:
>
> > On Tuesday, March 16, 2021 6:13 AM, Amit Langote amitlangote09@gmail.com wrote:
> >
> > > On Fri, Mar 12, 2021 at 7:59 PM gkokolatos@pm.me wrote:
> > >
> > > > On Friday, March 12, 2021 3:45 AM, Amit Langote amitlangote09@gmail.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

  15. Re: Allow batched insert during cross-partition updates

    Amit Langote <amitlangote09@gmail.com> — 2021-04-04T15:05:43Z

    On Tue, Mar 16, 2021 at 6:13 PM <gkokolatos@pm.me> wrote:
> Status updated to RfC in the commitfest app.

Patch fails to apply per cfbot, so rebased.

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

  16. Re: Allow batched insert during cross-partition updates

    Zhihong Yu <zyu@yugabyte.com> — 2021-04-04T16:19:27Z

    Hi,
In the description:

cross-partition update of partitioned tables can't use batching
because ExecInitRoutingInfo() which initializes the insert target

'which' should be dropped since 'because' should start a sentence.

+-- Check that batched inserts also works for inserts made during

inserts also works -> inserts also work

+       Assert(node->rootResultRelInfo->ri_RelationDesc->rd_rel->relkind ==
+              RELKIND_PARTITIONED_TABLE);

The level of nested field accesses is quite deep. If the assertion fails,
it would be hard to know which field is null.
Maybe use several assertions:
       Assert(node->rootResultRelInfo)
       Assert(node->rootResultRelInfo->ri_RelationDesc)
       Assert(node->rootResultRelInfo->ri_RelationDesc->rd_rel->relkind ==
...

Cheers

On Sun, Apr 4, 2021 at 8:06 AM Amit Langote <amitlangote09@gmail.com> wrote:

> On Tue, Mar 16, 2021 at 6:13 PM <gkokolatos@pm.me> wrote:
> > Status updated to RfC in the commitfest app.
>
> Patch fails to apply per cfbot, so rebased.
>
> --
> Amit Langote
> EDB: http://www.enterprisedb.com
>

  17. Re: Allow batched insert during cross-partition updates

    Amit Langote <amitlangote09@gmail.com> — 2021-04-06T09:37:48Z

    On Mon, Apr 5, 2021 at 1:16 AM Zhihong Yu <zyu@yugabyte.com> wrote:
>
> Hi,
> In the description:
>
> cross-partition update of partitioned tables can't use batching
> because ExecInitRoutingInfo() which initializes the insert target
>
> 'which' should be dropped since 'because' should start a sentence.
>
> +-- Check that batched inserts also works for inserts made during
>
> inserts also works -> inserts also work
>
> +       Assert(node->rootResultRelInfo->ri_RelationDesc->rd_rel->relkind ==
> +              RELKIND_PARTITIONED_TABLE);
>
> The level of nested field accesses is quite deep. If the assertion fails, it would be hard to know which field is null.
> Maybe use several assertions:
>        Assert(node->rootResultRelInfo)
>        Assert(node->rootResultRelInfo->ri_RelationDesc)
>        Assert(node->rootResultRelInfo->ri_RelationDesc->rd_rel->relkind == ...

Thanks for taking a look at this.

While I agree about having the 1st Assert you suggest, I don't think
this code needs the 2nd one, because its failure would very likely be
due to a problem in some totally unrelated code.

Updated patch attached.  I had to adjust the test case a little bit to
account for the changes of 86dc90056d, something I failed to notice
yesterday.  Also, I expanded the test case added in postgres_fdw.sql a
bit to show the batching in action more explicitly.

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

  18. Re: Allow batched insert during cross-partition updates

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-04-06T09:48:57Z

    On Tue, Apr 6, 2021 at 3:08 PM Amit Langote <amitlangote09@gmail.com> wrote:
>
> On Mon, Apr 5, 2021 at 1:16 AM Zhihong Yu <zyu@yugabyte.com> wrote:
> >
> > Hi,
> > In the description:
> >
> > cross-partition update of partitioned tables can't use batching
> > because ExecInitRoutingInfo() which initializes the insert target
> >
> > 'which' should be dropped since 'because' should start a sentence.
> >
> > +-- Check that batched inserts also works for inserts made during
> >
> > inserts also works -> inserts also work
> >
> > +       Assert(node->rootResultRelInfo->ri_RelationDesc->rd_rel->relkind ==
> > +              RELKIND_PARTITIONED_TABLE);
> >
> > The level of nested field accesses is quite deep. If the assertion fails, it would be hard to know which field is null.
> > Maybe use several assertions:
> >        Assert(node->rootResultRelInfo)
> >        Assert(node->rootResultRelInfo->ri_RelationDesc)
> >        Assert(node->rootResultRelInfo->ri_RelationDesc->rd_rel->relkind == ...
>
> Thanks for taking a look at this.
>
> While I agree about having the 1st Assert you suggest, I don't think
> this code needs the 2nd one, because its failure would very likely be
> due to a problem in some totally unrelated code.
>
> Updated patch attached.  I had to adjust the test case a little bit to
> account for the changes of 86dc90056d, something I failed to notice
> yesterday.  Also, I expanded the test case added in postgres_fdw.sql a
> bit to show the batching in action more explicitly.

Some minor comments:
1) don't we need order by clause for the selects in the tests added?
+SELECT tableoid::regclass, * FROM batch_cp_upd_test;
+SELECT cmin, * FROM batch_cp_upd_test1;

2) typo - it is "should" not "shoud"
+-- cmin shoud be different across rows, because each one would be inserted

3) will the cmin in the output always be the same?
+SELECT cmin, * FROM batch_cp_upd_test3;

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

  19. Re: Allow batched insert during cross-partition updates

    Amit Langote <amitlangote09@gmail.com> — 2021-04-06T13:07:41Z

    On Tue, Apr 6, 2021 at 6:49 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
> On Tue, Apr 6, 2021 at 3:08 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > Updated patch attached.  I had to adjust the test case a little bit to
> > account for the changes of 86dc90056d, something I failed to notice
> > yesterday.  Also, I expanded the test case added in postgres_fdw.sql a
> > bit to show the batching in action more explicitly.
>
> Some minor comments:

Thanks for the review.

> 1) don't we need order by clause for the selects in the tests added?
> +SELECT tableoid::regclass, * FROM batch_cp_upd_test;

Good point.  It wasn't necessary before, but it is after the test
expansion, so added.

> 3) will the cmin in the output always be the same?
> +SELECT cmin, * FROM batch_cp_upd_test3;

TBH, I am not so sure.  Maybe it's not a good idea to rely on cmin
after all.  I've rewritten the tests to use a different method of
determining if a single or multiple insert commands were used in
moving rows into foreign partitions.

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

  20. Re: Allow batched insert during cross-partition updates

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-04-06T13:52:40Z

    On Tue, Apr 6, 2021 at 6:37 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > 3) will the cmin in the output always be the same?
> > +SELECT cmin, * FROM batch_cp_upd_test3;
>
> TBH, I am not so sure.  Maybe it's not a good idea to rely on cmin
> after all.  I've rewritten the tests to use a different method of
> determining if a single or multiple insert commands were used in
> moving rows into foreign partitions.

Thanks! It looks good!

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

  21. Re: Allow batched insert during cross-partition updates

    Amit Langote <amitlangote09@gmail.com> — 2021-04-07T01:49:35Z

    On Tue, Apr 6, 2021 at 10:52 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Tue, Apr 6, 2021 at 6:37 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > > 3) will the cmin in the output always be the same?
> > > +SELECT cmin, * FROM batch_cp_upd_test3;
> >
> > TBH, I am not so sure.  Maybe it's not a good idea to rely on cmin
> > after all.  I've rewritten the tests to use a different method of
> > determining if a single or multiple insert commands were used in
> > moving rows into foreign partitions.
>
> Thanks! It looks good!

Thanks for checking.  I'll mark this as RfC.

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

  22. RE: Allow batched insert during cross-partition updates

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2021-05-07T09:39:37Z

    > > Thanks! It looks good!
> 
> Thanks for checking.  I'll mark this as RfC.

Hi,

The patch cannot be applied to the latest head branch, it will be nice if you can rebase it.
And when looking into the patch, I have some comments on it.

1)
IIRC, After the commit c5b7ba4, the initialization of mt_partition_tuple_routing was postponed out of ExecInitModifyTable.
So, the following if-test use "proute" which is initialized at the beginning of the ExecModifyTable() could be out of date.
And the regression test of postgres_fdw failed with the patch after the commit c5b7ba4.

+	 * If the query's main target relation is a partitioned table, any inserts
+	 * would have been performed using tuple routing, so use the appropriate
+	 * set of target relations.  Note that this also covers any inserts
+	 * performed during cross-partition UPDATEs that would have occurred
+	 * through tuple routing.
 	 */
 	if (proute)
...

It seems we should get the mt_partition_tuple_routing just before the if-test.

2)
+		foreach(lc, estate->es_opened_result_relations)
+		{
+			resultRelInfo = lfirst(lc);
+			if (resultRelInfo &&

I am not sure do we need to check if resultRelInfo == NULL because the the existing code did not check it.
And if we need to check it, it might be better use "if (resultRelInfo == NULL &&..."

3)
+	if (fmstate && fmstate->aux_fmstate != NULL)
+		fmstate = fmstate->aux_fmstate;

It might be better to write like " if (fmstate != NULL && fmstate->aux_fmstate != NULL)".

Best regards,
houzj

  23. Re: Allow batched insert during cross-partition updates

    Amit Langote <amitlangote09@gmail.com> — 2021-05-10T06:58:56Z

    On Fri, May 7, 2021 at 6:39 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> > > Thanks! It looks good!
> >
> > Thanks for checking.  I'll mark this as RfC.
>
> Hi,
>
> The patch cannot be applied to the latest head branch, it will be nice if you can rebase it.

Thanks, done.

> And when looking into the patch, I have some comments on it.
>
> 1)
> IIRC, After the commit c5b7ba4, the initialization of mt_partition_tuple_routing was postponed out of ExecInitModifyTable.
> So, the following if-test use "proute" which is initialized at the beginning of the ExecModifyTable() could be out of date.
> And the regression test of postgres_fdw failed with the patch after the commit c5b7ba4.
>
> +        * If the query's main target relation is a partitioned table, any inserts
> +        * would have been performed using tuple routing, so use the appropriate
> +        * set of target relations.  Note that this also covers any inserts
> +        * performed during cross-partition UPDATEs that would have occurred
> +        * through tuple routing.
>          */
>         if (proute)
> ...
>
> It seems we should get the mt_partition_tuple_routing just before the if-test.

That's a good observation.  Fixed.

> 2)
> +               foreach(lc, estate->es_opened_result_relations)
> +               {
> +                       resultRelInfo = lfirst(lc);
> +                       if (resultRelInfo &&
>
> I am not sure do we need to check if resultRelInfo == NULL because the the existing code did not check it.
> And if we need to check it, it might be better use "if (resultRelInfo == NULL &&..."

I don't quite remember why I added that test, because nowhere do we
add a NULL value to es_opened_result_relations.  Actually, we can even
Assert(resultRelInfo != NULL) here.

> 3)
> +       if (fmstate && fmstate->aux_fmstate != NULL)
> +               fmstate = fmstate->aux_fmstate;
>
> It might be better to write like " if (fmstate != NULL && fmstate->aux_fmstate != NULL)".

Sure, done.  Actually, there's a if (fmstate) statement right below
the code being added, which I fixed to match the style used by the new
code.

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

  24. Re: Allow batched insert during cross-partition updates

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-07-01T16:39:39Z

    Amit Langote <amitlangote09@gmail.com> writes:
> [ v6-0001-Allow-batching-of-inserts-during-cross-partition-.patch ]

Per the cfbot, this isn't applying anymore, so I'm setting it back
to Waiting on Author.

			regards, tom lane

  25. Re: Allow batched insert during cross-partition updates

    Amit Langote <amitlangote09@gmail.com> — 2021-07-02T02:05:08Z

    On Fri, Jul 2, 2021 at 1:39 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Langote <amitlangote09@gmail.com> writes:
> > [ v6-0001-Allow-batching-of-inserts-during-cross-partition-.patch ]
>
> Per the cfbot, this isn't applying anymore, so I'm setting it back
> to Waiting on Author.

Rebased patch attached.  Thanks for the reminder.


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

  26. Re: Allow batched insert during cross-partition updates

    vignesh C <vignesh21@gmail.com> — 2021-07-22T05:18:09Z

    On Fri, Jul 2, 2021 at 7:35 AM Amit Langote <amitlangote09@gmail.com> wrote:
>
> On Fri, Jul 2, 2021 at 1:39 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > Amit Langote <amitlangote09@gmail.com> writes:
> > > [ v6-0001-Allow-batching-of-inserts-during-cross-partition-.patch ]
> >
> > Per the cfbot, this isn't applying anymore, so I'm setting it back
> > to Waiting on Author.
>
> Rebased patch attached.  Thanks for the reminder.

One of the test postgres_fdw has failed, can you please post an
updated patch for the fix:
test postgres_fdw                 ... FAILED (test process exited with
exit code 2)     7264 ms

Regards,
Vignesh

  27. Re: Allow batched insert during cross-partition updates

    Amit Langote <amitlangote09@gmail.com> — 2021-07-27T02:32:36Z

    On Thu, Jul 22, 2021 at 2:18 PM vignesh C <vignesh21@gmail.com> wrote:
> On Fri, Jul 2, 2021 at 7:35 AM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Fri, Jul 2, 2021 at 1:39 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > >
> > > Amit Langote <amitlangote09@gmail.com> writes:
> > > > [ v6-0001-Allow-batching-of-inserts-during-cross-partition-.patch ]
> > >
> > > Per the cfbot, this isn't applying anymore, so I'm setting it back
> > > to Waiting on Author.
> >
> > Rebased patch attached.  Thanks for the reminder.
>
> One of the test postgres_fdw has failed, can you please post an
> updated patch for the fix:
> test postgres_fdw                 ... FAILED (test process exited with
> exit code 2)     7264 ms

Thanks Vignesh.

I found a problem with the underlying batching code that caused this
failure and have just reported it here:

https://www.postgresql.org/message-id/CA%2BHiwqEWd5B0-e-RvixGGUrNvGkjH2s4m95%3DJcwUnyV%3Df0rAKQ%40mail.gmail.com

Here's v8, including the patch for the above problem.

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

  28. Re: Allow batched insert during cross-partition updates

    Amit Langote <amitlangote09@gmail.com> — 2021-08-24T03:03:59Z

    On Tue, Jul 27, 2021 at 11:32 AM Amit Langote <amitlangote09@gmail.com> wrote:
> On Thu, Jul 22, 2021 at 2:18 PM vignesh C <vignesh21@gmail.com> wrote:
> > One of the test postgres_fdw has failed, can you please post an
> > updated patch for the fix:
> > test postgres_fdw                 ... FAILED (test process exited with
> > exit code 2)     7264 ms
>
> Thanks Vignesh.
>
> I found a problem with the underlying batching code that caused this
> failure and have just reported it here:
>
> https://www.postgresql.org/message-id/CA%2BHiwqEWd5B0-e-RvixGGUrNvGkjH2s4m95%3DJcwUnyV%3Df0rAKQ%40mail.gmail.com
>
> Here's v8, including the patch for the above problem.

Tomas committed the bug-fix, so attaching a rebased version of the
patch for $subject.

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

  29. Re: Allow batched insert during cross-partition updates

    Andres Freund <andres@anarazel.de> — 2022-03-22T00:30:28Z

    Hi,

On 2021-08-24 12:03:59 +0900, Amit Langote wrote:
> Tomas committed the bug-fix, so attaching a rebased version of the
> patch for $subject.

This patch is in the current CF, but doesn't apply: http://cfbot.cputube.org/patch_37_2992.log
marked the entry as waiting-on-author.

Greetings,

Andres Freund

  30. Re: Allow batched insert during cross-partition updates

    Amit Langote <amitlangote09@gmail.com> — 2022-03-22T01:17:08Z

    On Tue, Mar 22, 2022 at 9:30 AM Andres Freund <andres@anarazel.de> wrote:
> On 2021-08-24 12:03:59 +0900, Amit Langote wrote:
> > Tomas committed the bug-fix, so attaching a rebased version of the
> > patch for $subject.
>
> This patch is in the current CF, but doesn't apply: http://cfbot.cputube.org/patch_37_2992.log
> marked the entry as waiting-on-author.

Thanks for the heads up.

Rebased to fix a minor conflict with some recently committed
nodeModifyTable.c changes.

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

  31. Re: Allow batched insert during cross-partition updates

    Etsuro Fujita <etsuro.fujita@gmail.com> — 2022-12-06T09:48:54Z

    Amit-san,

On Tue, Mar 22, 2022 at 10:17 AM Amit Langote <amitlangote09@gmail.com> wrote:
> Rebased to fix a minor conflict with some recently committed
> nodeModifyTable.c changes.

Apologies for not having reviewed the patch.  Here are some review comments:

* The patch conflicts with commit ffbb7e65a, so please update the
patch.  (That commit would cause an API break, so I am planning to
apply a fix to HEAD as well [1].)  That commit fixed the handling of
pending inserts, which I think would eliminate the need to do this:

    * ExecModifyTable(), when inserting any remaining batched tuples,
    must look at the correct set of ResultRelInfos that would've been
    used by such inserts, because failing to do so would result in those
    tuples not actually getting inserted.  To fix, ExecModifyTable() is
    now made to get the ResultRelInfos from the PartitionTupleRouting
    data structure which contains the ResultRelInfo that would be used by
    those internal inserts. To allow nodeModifyTable.c look inside
    PartitionTupleRouting, its definition, which was previously local to
    execPartition.c, is exposed via execPartition.h.

* In postgresGetForeignModifyBatchSize():

    /*
-    * Should never get called when the insert is being performed as part of a
-    * row movement operation.
+    * Use the auxiliary state if any; see postgresBeginForeignInsert() for
+    * details on what it represents.
     */
-   Assert(fmstate == NULL || fmstate->aux_fmstate == NULL);
+   if (fmstate != NULL && fmstate->aux_fmstate != NULL)
+       fmstate = fmstate->aux_fmstate;

I might be missing something, but I think we should leave the Assert
as-is, because we still disallow to move rows to another foreign-table
partition that is also an UPDATE subplan result relation, which means
that any fmstate should have fmstate->aux_fmstate=NULL.

* Also in that function:

-   if (fmstate)
+   if (fmstate != NULL)

This is correct, but I would vote for leaving that as-is, to make
back-patching easy.

That is all I have for now.  I will mark this as Waiting on Author.

Best regards,
Etsuro Fujita

[1] https://www.postgresql.org/message-id/CAPmGK17rmXEY3BL%3DAq71L8UZv5f-mz%3DuxJkz1kMnfSSY%2BpFe-A%40mail.gmail.com

  32. Re: Allow batched insert during cross-partition updates

    Amit Langote <amitlangote09@gmail.com> — 2022-12-08T07:59:46Z

    Hi Fujita-san,

On Tue, Dec 6, 2022 at 6:47 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> On Tue, Mar 22, 2022 at 10:17 AM Amit Langote <amitlangote09@gmail.com> wrote:
> > Rebased to fix a minor conflict with some recently committed
> > nodeModifyTable.c changes.
>
> Apologies for not having reviewed the patch.  Here are some review comments:

No problem and thanks for taking a look.

> * The patch conflicts with commit ffbb7e65a, so please update the
> patch.  (That commit would cause an API break, so I am planning to
> apply a fix to HEAD as well [1].)  That commit fixed the handling of
> pending inserts, which I think would eliminate the need to do this:
>
>     * ExecModifyTable(), when inserting any remaining batched tuples,
>     must look at the correct set of ResultRelInfos that would've been
>     used by such inserts, because failing to do so would result in those
>     tuples not actually getting inserted.  To fix, ExecModifyTable() is
>     now made to get the ResultRelInfos from the PartitionTupleRouting
>     data structure which contains the ResultRelInfo that would be used by
>     those internal inserts. To allow nodeModifyTable.c look inside
>     PartitionTupleRouting, its definition, which was previously local to
>     execPartition.c, is exposed via execPartition.h.

Ah, I see.  Removed those hunks.

> * In postgresGetForeignModifyBatchSize():
>
>     /*
> -    * Should never get called when the insert is being performed as part of a
> -    * row movement operation.
> +    * Use the auxiliary state if any; see postgresBeginForeignInsert() for
> +    * details on what it represents.
>      */
> -   Assert(fmstate == NULL || fmstate->aux_fmstate == NULL);
> +   if (fmstate != NULL && fmstate->aux_fmstate != NULL)
> +       fmstate = fmstate->aux_fmstate;
>
> I might be missing something, but I think we should leave the Assert
> as-is, because we still disallow to move rows to another foreign-table
> partition that is also an UPDATE subplan result relation, which means
> that any fmstate should have fmstate->aux_fmstate=NULL.

Hmm, yes.  I forgot that 86dc90056df effectively disabled *all*
attempts of inserting into foreign partitions that are also UPDATE
target relations, so you are correct that fmstate->aux_fmstate would
never be set when entering this function.

That means this functionality is only useful for foreign partitions
that are not also being updated by the original UPDATE.

I've reinstated the Assert, removed the if block as it's useless, and
updated the comment a bit to clarify the restriction a bit.

> * Also in that function:
>
> -   if (fmstate)
> +   if (fmstate != NULL)
>
> This is correct, but I would vote for leaving that as-is, to make
> back-patching easy.

Removed this hunk.

> That is all I have for now.  I will mark this as Waiting on Author.

Updated patch attached.


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

  33. Re: Allow batched insert during cross-partition updates

    Etsuro Fujita <etsuro.fujita@gmail.com> — 2022-12-08T11:01:02Z

    Amit-san,

On Thu, Dec 8, 2022 at 5:00 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Tue, Dec 6, 2022 at 6:47 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> > * In postgresGetForeignModifyBatchSize():
> >
> >     /*
> > -    * Should never get called when the insert is being performed as part of a
> > -    * row movement operation.
> > +    * Use the auxiliary state if any; see postgresBeginForeignInsert() for
> > +    * details on what it represents.
> >      */
> > -   Assert(fmstate == NULL || fmstate->aux_fmstate == NULL);
> > +   if (fmstate != NULL && fmstate->aux_fmstate != NULL)
> > +       fmstate = fmstate->aux_fmstate;
> >
> > I might be missing something, but I think we should leave the Assert
> > as-is, because we still disallow to move rows to another foreign-table
> > partition that is also an UPDATE subplan result relation, which means
> > that any fmstate should have fmstate->aux_fmstate=NULL.
>
> Hmm, yes.  I forgot that 86dc90056df effectively disabled *all*
> attempts of inserting into foreign partitions that are also UPDATE
> target relations, so you are correct that fmstate->aux_fmstate would
> never be set when entering this function.
>
> That means this functionality is only useful for foreign partitions
> that are not also being updated by the original UPDATE.

Yeah, I think so too.

> I've reinstated the Assert, removed the if block as it's useless, and
> updated the comment a bit to clarify the restriction a bit.

Looks good to me.

> > * Also in that function:
> >
> > -   if (fmstate)
> > +   if (fmstate != NULL)
> >
> > This is correct, but I would vote for leaving that as-is, to make
> > back-patching easy.
>
> Removed this hunk.

Thanks!

> Updated patch attached.

Thanks for the patch!  I will review the patch a bit more, but I think
it would be committable.

Best regards,
Etsuro Fujita

  34. Re: Allow batched insert during cross-partition updates

    Etsuro Fujita <etsuro.fujita@gmail.com> — 2022-12-14T09:44:48Z

    On Thu, Dec 8, 2022 at 8:01 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> On Thu, Dec 8, 2022 at 5:00 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > Updated patch attached.
>
> I will review the patch a bit more, but I think
> it would be committable.

One thing I noticed is this bit:

 -- Clean up
-DROP TABLE batch_table, batch_cp_upd_test, batch_table_p0,
batch_table_p1 CASCADE;
+DROP TABLE batch_table, batch_table_p0, batch_table_p1,
batch_cp_upd_test, cmdlog CASCADE;

This would be nitpicking, but this as-proposed will not remove remote
tables created for foreign-table partitions of the partitioned table
‘batch_cp_upd_test’.  So I modified this a bit further to remove them
as well.  Also, I split this into two, for readability.  Another thing
is a typo in a test-case comment: s/a single INSERTs/a single INSERT/.
I fixed it as well.  Other than that, the patch looks good to me.
Attached is an updated patch.  If there are no objections, I will
commit the patch.

Best regards,
Etsuro Fujita

  35. Re: Allow batched insert during cross-partition updates

    Amit Langote <amitlangote09@gmail.com> — 2022-12-14T13:28:46Z

    On Wed, Dec 14, 2022 at 6:45 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> On Thu, Dec 8, 2022 at 8:01 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> > On Thu, Dec 8, 2022 at 5:00 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > > Updated patch attached.
> >
> > I will review the patch a bit more, but I think
> > it would be committable.
>
> One thing I noticed is this bit:
>
>  -- Clean up
> -DROP TABLE batch_table, batch_cp_upd_test, batch_table_p0,
> batch_table_p1 CASCADE;
> +DROP TABLE batch_table, batch_table_p0, batch_table_p1,
> batch_cp_upd_test, cmdlog CASCADE;
>
> This would be nitpicking, but this as-proposed will not remove remote
> tables created for foreign-table partitions of the partitioned table
> ‘batch_cp_upd_test’.  So I modified this a bit further to remove them
> as well.  Also, I split this into two, for readability.  Another thing
> is a typo in a test-case comment: s/a single INSERTs/a single INSERT/.
> I fixed it as well.  Other than that, the patch looks good to me.
> Attached is an updated patch.  If there are no objections, I will
> commit the patch.

Thanks for the changes.  LGTM.

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

  36. Re: Allow batched insert during cross-partition updates

    Etsuro Fujita <etsuro.fujita@gmail.com> — 2022-12-20T10:19:51Z

    Hi Amit-san,

On Wed, Dec 14, 2022 at 10:29 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Wed, Dec 14, 2022 at 6:45 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> > One thing I noticed is this bit:
> >
> >  -- Clean up
> > -DROP TABLE batch_table, batch_cp_upd_test, batch_table_p0,
> > batch_table_p1 CASCADE;
> > +DROP TABLE batch_table, batch_table_p0, batch_table_p1,
> > batch_cp_upd_test, cmdlog CASCADE;
> >
> > This would be nitpicking, but this as-proposed will not remove remote
> > tables created for foreign-table partitions of the partitioned table
> > ‘batch_cp_upd_test’.  So I modified this a bit further to remove them
> > as well.  Also, I split this into two, for readability.  Another thing
> > is a typo in a test-case comment: s/a single INSERTs/a single INSERT/.
> > I fixed it as well.  Other than that, the patch looks good to me.
> > Attached is an updated patch.  If there are no objections, I will
> > commit the patch.
>
> LGTM.

Cool!  Pushed.

Best regards,
Etsuro Fujita

  37. Re: Allow batched insert during cross-partition updates

    Amit Langote <amitlangote09@gmail.com> — 2022-12-20T11:19:21Z

    On Tue, Dec 20, 2022 at 7:18 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> On Wed, Dec 14, 2022 at 10:29 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Wed, Dec 14, 2022 at 6:45 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> > > One thing I noticed is this bit:
> > >
> > >  -- Clean up
> > > -DROP TABLE batch_table, batch_cp_upd_test, batch_table_p0,
> > > batch_table_p1 CASCADE;
> > > +DROP TABLE batch_table, batch_table_p0, batch_table_p1,
> > > batch_cp_upd_test, cmdlog CASCADE;
> > >
> > > This would be nitpicking, but this as-proposed will not remove remote
> > > tables created for foreign-table partitions of the partitioned table
> > > ‘batch_cp_upd_test’.  So I modified this a bit further to remove them
> > > as well.  Also, I split this into two, for readability.  Another thing
> > > is a typo in a test-case comment: s/a single INSERTs/a single INSERT/.
> > > I fixed it as well.  Other than that, the patch looks good to me.
> > > Attached is an updated patch.  If there are no objections, I will
> > > commit the patch.
> >
> > LGTM.
>
> Cool!  Pushed.

Thank you, Fujita-san.

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