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
