(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
