Re: Issue in ExecCleanupTupleRouting()

(2019/04/12 10:48), Amit Langote wrote:
> 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.

Pushed.  Thanks for reviewing, David and Amit!

Best regards,
Etsuro Fujita