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

Fujita-san,

On 2018/04/02 21:29, Etsuro Fujita wrote:
> (2018/04/02 18:49), Amit Langote wrote:
>> 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?

Hmm, I can see the point.  Passing mostly-dummy (garbage) PlannerInfo and
query tree from the core code to FDW seems bad.  By defining the new API
with a clean interface (receiving fully valid ModifyTableState), we're not
required to do that across the interface, but only inside the FDW's
implementation.  I was just slightly concerned that the new FDW function
would have to implement what would normally be carried out across multiple
special purpose callbacks, but maybe that's OK as long as it's clearly
documented what its job is.

Noticed a couple of things in the patch:

+     
+      When this is called by a COPY FROM command, the
+      plan-related global data in mtstate is not provided
+      and the planSlot parameter of
+      ExecForeignInsert called for each inserted
tuple is

How about s/called/subsequently called/g?

+      NULL, wether the foreign table is the partition
chosen

Typo: s/wether/whether/g

Thanks,
Amit