Re: [HACKERS] Add support for tuple routing to foreign partitions

Fujita-san,

Thanks for updating the patch.

On 2018/03/30 21:56, Etsuro Fujita wrote:
> I modified the patch to use the existing API ExecForeignInsert instead of
> that API and removed that API including this doc.

OK.

>> 2. If I understand the description you provided in [1] correctly, the
>> point of having ri_PartitionIsValid and ExecInitExtraPartitionInfo() is to
>> avoid possibly-redundantly performing following two steps in
>> ExecSetupPartitionTupleRouting() for *reused* partition ResultRelInfos
>> that may not be used for tuple routing after all:
>>
>>   - create the parent_child_tupconv_maps[] entry for it
>>   - perform FDW tuple routing initialization.
>
> Sorry, my explanation was not enough, but that was just one of the reasons
> why I introduced those; another is to do CheckValidResultRel against a
> partition after the partition was chosen for tuple routing rather than in
> ExecSetupPartitionTupleRouting, to avoid aborting an UPDATE of a partition
> key unnecessarily due to non-routable foreign-partitions that may not be
> chosen for tuple routing after all.

I see. So, currently in ExecSetupPartitionTupleRouting, we call
CheckValidResultRel() to check if resultRelInfos reused from those
initialized for UPDATE are valid for insertions (or rather for routing
inserted tuples into it). But you're proposing to delay that check until
ExecPrepareTupleRouting is called for a given resultRelInfo, at which
point it's clear that we actually want to insert using that resultRelInfo.
That seems reasonable.

> Now we have ON CONFLICT for partitioned tables, which requires the
> conversion map to be computed in ExecInitPartitionInfo, so I updated the
> patch so that we keep the former step in ExecInitPartitionInfo and
> ExecSetupPartitionTupleRouting and so that we just init the FDW in
> ExecInitExtraPartitionInfo (changed to ExecInitForeignRouting).  And I
> added comments to ExecInitForeignRouting and ExecPrepareTupleRouting.

That looks good.

I looked at the new patch. It looks good overall, although I have one
question -- IIUC, BeginForeignInsert() performs actions that are
equivalent of performing PlanForeignModify() + BeginForeignModify() for an
INSERT as if it was directly executed on a given foreign table/partition.
Did you face any problems in doing the latter itself in the core code?
Doing it that way would mean no changes to a FDW itself will be required
(and hence no need for additional APIs), but I may be missing something.

One thing that seems good about your approach is that newly added support
for COPY FROM on foreign tables/partitions takes minimal changes as
implemented by using the new API, whereas with the alternative approach it
would require duplicated code (same code would have to be present in both
copy.c and execPartition.c, but it could perhaps be avoided).

Thanks,
Amit