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

(2018/04/02 18:49), Amit Langote wrote:
>>> 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.

That's exactly what I'm thinking.

>> 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 revisited this.  Please see the reply to Alvaro I sent right now.

> 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.

The biggest issue in performing PlanForeignModify() plus 
BeginForeignModify() instead of the new FDW API would be: can the core 
code create a valid-looking planner state passed to PlanForeignModify() 
such as the ModifyTable plan node or the query tree stored in PlannerInfo?

> 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).

I agree.

Thanks for the review!

Best regards,
Etsuro Fujita