(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