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

(2018/04/03 13:59), Amit Langote wrote:
> 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 really think so too.

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

OK

> Noticed a couple of things in the patch:
>
> +<para>
> +      When this is called by a<command>COPY FROM</command>  command, the
> +      plan-related global data in<literal>mtstate</literal>  is not provided
> +      and the<literal>planSlot</literal>  parameter of
> +<function>ExecForeignInsert</function>  called for each inserted
> tuple is
>
> How about s/called/subsequently called/g?

Done.

> +<literal>NULL</literal>, wether the foreign table is the partition
> chosen
>
> Typo: s/wether/whether/g

Done.

Thanks again, Amit!

Best regards,
Etsuro Fujita