Subject: Re: Allow batched insert during cross-partition updates

From: Amit Langote <[email-masked]>
Date: Tue, 22 Mar 2022 10:17:08 +0900

On Tue, Mar 22, 2022 at 9:30 AM Andres Freund <[email-masked]> 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

---

From: Etsuro Fujita <[email-masked]>
Date: Tue, 6 Dec 2022 18:48:54 +0900

Amit-san,

> 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

---

From: Amit Langote <[email-masked]>
Date: Thu, 8 Dec 2022 16:59:46 +0900

Hi Fujita-san,

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

No problem and thanks for taking a look.

>     execPartition.c, is exposed via execPartition.h.

Ah, I see.  Removed those hunks.

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

> This is correct, but I would vote for leaving that as-is, to make

Removed this hunk.

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

Updated patch attached.

--
Thanks, Amit Langote

---

From: Etsuro Fujita <[email-masked]>
Date: Thu, 8 Dec 2022 20:01:02 +0900

Amit-san,

> that are not also being updated by the original UPDATE.

Yeah, I think so too.

> updated the comment a bit to clarify the restriction a bit.

Looks good to me.

> > This is correct, but I would vote for leaving that as-is, to make

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

---

From: Etsuro Fujita <[email-masked]>
Date: Wed, 14 Dec 2022 18:44:48 +0900

On Thu, Dec 8, 2022 at 8:01 PM Etsuro Fujita <[email-masked]> wrote:
> 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

---

From: Amit Langote <[email-masked]>
Date: Wed, 14 Dec 2022 22:28:46 +0900

> Attached is an updated patch.  If there are no objections, I will

Thanks for the changes.  LGTM.

---

From: Etsuro Fujita <[email-masked]>
Date: Tue, 20 Dec 2022 19:19:51 +0900

Hi Amit-san,

On Wed, Dec 14, 2022 at 10:29 PM Amit Langote <[email-masked]> wrote:
> LGTM.

Cool!  Pushed.

Best regards,
Etsuro Fujita

---

From: Amit Langote <[email-masked]>
Date: Tue, 20 Dec 2022 20:19:21 +0900

> > > Attached is an updated patch.  If there are no objections, I will

Thank you, Fujita-san.