Re: Issue in ExecCleanupTupleRouting()

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.

Thanks,
Amit