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(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.
>

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