Re: POC: postgres_fdw insert batching

On Thu, Jan 21, 2021 at 10:42 AM Tomas Vondra
<tomas(dot)vondra(at)enterprisedb(dot)com> wrote:
> On 1/21/21 2:24 AM, Amit Langote wrote:
> > On Thu, Jan 21, 2021 at 9:56 AM Tomas Vondra
> > <tomas(dot)vondra(at)enterprisedb(dot)com> wrote:
> >> On 1/21/21 1:17 AM, Zhihong Yu wrote:
> >>> Hi,
> >>> The assignment to resultRelInfo is done when junk_filter_needed is true:
> >>>
> >>> if (junk_filter_needed)
> >>> {
> >>> resultRelInfo = mtstate->resultRelInfo;
> >>>
> >>> Should the code for determining batch size access mtstate->resultRelInfo
> >>> directly ?
> >>>
> >>
> >> IMO the issue is that code iterates over all plans and moves to the next
> >> for each one:
> >>
> >> resultRelInfo++;
> >>
> >> so it ends up pointing past the last element, hence the failures. So
> >> yeah, either the code needs to move before the loop (per my patch), or
> >> we need to access mtstate->resultRelInfo directly.
> >
> > Accessing mtstate->resultRelInfo directly would do. The only
> > constraint on where this block should be placed is that
> > ri_projectReturning must be valid as of calling
> > GetForeignModifyBatchSize(), as Tsunakawa-san pointed out upthread.
> > So, after this block in ExecInitModifyTable:
> >
> > /*
> > * Initialize RETURNING projections if needed.
> > */
> > if (node->returningLists)
> > {
> > ....
> > /*
> > * Build a projection for each result rel.
> > */
> > resultRelInfo = mtstate->resultRelInfo;
> > foreach(l, node->returningLists)
> > {
> > List *rlist = (List *) lfirst(l);
> >
> > resultRelInfo->ri_returningList = rlist;
> > resultRelInfo->ri_projectReturning =
> > ExecBuildProjectionInfo(rlist, econtext, slot, &mtstate->ps,
> > resultRelInfo->ri_RelationDesc->rd_att);
> > resultRelInfo++;
> > }
> > }
> >
>
> Right. But I think Tom is right this should initialize ri_BatchSize for
> all the resultRelInfo elements, not just the first one. Per the attached
> patch, which resolves the issue both on x86_64 and armv7l for me.

+1 in general. To avoid looping uselessly in the case of
UPDATE/DELETE where batching can't be used today, I'd suggest putting
if (operation == CMD_INSERT) around the loop.

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