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(dot)etsuro(at)lab(dot)ntt(dot)co(dot)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