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