Re: POC: postgres_fdw insert batching

On Thu, Jan 21, 2021 at 10:42 AM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:
> On 1/21/21 2:24 AM, Amit Langote wrote:
> > On Thu, Jan 21, 2021 at 9:56 AM Tomas Vondra
> > <tomas.vondra@enterprisedb.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