Re: Allow batched insert during cross-partition updates

Amit-san,

On Thu, Dec 8, 2022 at 5:00 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Tue, Dec 6, 2022 at 6:47 PM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)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