Re: POC: postgres_fdw insert batching

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.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company