Re: pgsql: Allow insert and update tuple routing and COPY for foreign table

(2019/04/23 4:37), Laurenz Albe wrote:
> On Mon, 2019-04-22 at 21:45 +0900, Etsuro Fujita wrote:
>> (2019/04/20 20:53), Laurenz Albe wrote:
>>> On Fri, 2018-04-06 at 23:24 +0000, Robert Haas wrote:
>>>> Allow insert and update tuple routing and COPY for foreign tables.
>>>>
>>>> Also enable this for postgres_fdw.
>>>>
>>>> Etsuro Fujita, based on an earlier patch by Amit Langote. The larger
>>>> patch series of which this is a part has been reviewed by Amit
>>>> Langote, David Fetter, Maksim Milyutin, Álvaro Herrera, Stephen Frost,
>>>> and me.  Minor documentation changes to the final version by me.
>>>>
>>>> Discussion: http://postgr.es/m/29906a26-da12-8c86-4fb9-d8f88442f2b9@lab.ntt.co.jp

>>> If a FDW implements ExecForeignInsert, this commit automatically assumes
>>> that it also supports COPY FROM.  It will call ExecForeignInsert without
>>> calling PlanForeignModify and BeginForeignModify, and a FDW that does not
>>> expect that will probably fail.
>>
>> This is not 100% correct; the FDW documentation says:
>>
>>       <para>
>>        Tuples inserted into a partitioned table by
>> <command>INSERT</command>  or
>>        <command>COPY FROM</command>  are routed to partitions.  If an FDW
>>        supports routable foreign-table partitions, it should also provide the
>>        following callback functions.  These functions are also called when
>>        <command>COPY FROM</command>  is executed on a foreign table.
>>       </para>
>
> I don't see the difference between the documentation and what I wrote above.
>
> Before v11, a FDW could expect that ExecForeignInsert is only called if
> BeginForeignModify was called earlier.
> That has silently changed with v11.

I have to admit that the documentation is not sufficient.

>> It's permissible to throw an error in BeginForeignInsert, so what I was
>> thinking for FDWs that don't want to support COPY FROM and
>> INSERT/UPDATE/COPY FROM tuple routing was to provide BeginForeignInsert
>> implementing something like this:
>>
>> static void
>> fooBeginForeignInsert(ModifyTableState *mtstate,
>>                         ResultRelInfo *resultRelInfo)
>> {
>>       Relation    rel = resultRelInfo->ri_RelationDesc;
>>
>>       if (mtstate->ps.plan == NULL)
>>           ereport(ERROR,
>>                   (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>>                    errmsg("cannot copy to foreign table \"%s\"",
>>                           RelationGetRelationName(rel))));
>>       else
>>           ereport(ERROR,
>>                   (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>>                    errmsg("cannot route tuples into foreign table \"%s\"",
>>                           RelationGetRelationName(rel))));
>> }
>
> Sure, it is not hard to modify a FDW to continue working with v11.

How about adding to the documentation for BeginForeignInsert a mention 
that if an FDW doesn't support COPY FROM and/or routable foreign tables, 
it must throw an error in BeginForeignInsert accordingly.

> My point is that this should not be necessary.

In my opinion, I think this is necessary...

> On the other hand, if a FDW wants to support COPY in v11 and has no
> need for BeginForeignInsert to support that, it is a simple exercise
> for it to provide an empty BeginForeignInsert just to signal that it
> wants to support COPY.

That seems to me inconsistent with the concept of the existing APIs for 
updating foreign tables, because for an FDW that wants to support 
INSERT/UPDATE/DELETE and has no need for 
PlanForeignModify/BeginForeignModify, those APIs don't require the FDW 
to provide empty PlanForeignModify/BeginForeignModify to tell the core 
that it wants to support INSERT/UPDATE/DELETE.

Best regards,
Etsuro Fujita