Re: Issue in ExecCleanupTupleRouting()

On 2019/04/11 22:28, David Rowley wrote:
> On Fri, 12 Apr 2019 at 01:06, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> wrote:
>> +       /*
>> +        * Check if this result rel is one belonging to the node's subplans,
>> +        * if so, let ExecEndPlan() clean it up.
>> +        */
>> +       if (htab)
>> +       {
>> +           Oid         partoid;
>> +           bool        found;
>> +
>> +           partoid = RelationGetRelid(resultRelInfo->ri_RelationDesc);
>> +
>> +           (void) hash_search(htab, &partoid, HASH_FIND, &found);
>> +           if (found)
>> +               continue;
>> +       }
>>
>>         /* Allow any FDWs to shut down if they've been exercised */
>> -       if (resultRelInfo->ri_PartitionReadyForRouting &&
>> -           resultRelInfo->ri_FdwRoutine != NULL &&
>> +       if (resultRelInfo->ri_FdwRoutine != NULL &&
>>             resultRelInfo->ri_FdwRoutine->EndForeignInsert != NULL)
>>
>> resultRelInfo->ri_FdwRoutine->EndForeignInsert(mtstate->ps.state,
>>                                                            resultRelInfo);
>>
>> This skips subplan resultrels before calling EndForeignInsert() if they
>> are foreign tables, which I think causes an issue: the FDWs would fail
>> to release resources for their foreign insert operations, because
>> ExecEndPlan() and ExecEndModifyTable() don't do anything to allow them
>> to do that.  So I think we should skip subplan resultrels after
>> EndForeignInsert().  Attached is a small patch for that.
> 
> Oops. I had for some reason been under the impression that it was
> nodeModifyTable.c, or whatever the calling code happened to be that
> handles these ones, but this is not the case as we call
> ExecInitRoutingInfo() from ExecFindPartition() which makes the call to
> BeginForeignInsert. If that part is handled by the tuple routing code,
> then the subsequent cleanup should be too, in which case your patch
> looks fine.

That sounds right.

Thanks,
Amit