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
