Re: POC: postgres_fdw insert batching

On 1/21/21 2:53 AM, Amit Langote wrote:
> 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.
> 

Right, that's pretty much what I ended up doing (without the CMD_INSERT 
check it'd add batching info to explain for updates too, for example). 
I'll do a bit more testing on the attached patch, but I think that's the 
right fix to push.

regards

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