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

(2019/04/24 22:04), Laurenz Albe wrote:
> On Wed, 2019-04-24 at 20:54 +0900, Etsuro Fujita wrote:
>> 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.
>
> Sure, some more documentation would be good.
>
> The documentation of ExecForeignInsert should mention something like:
>
> ExecForeignInsert is called for INSERT statements as well
> as for COPY FROM and tuples that are inserted into a foreign table
> because it is a partition of a partitioned table.
>
> In the case of a normal INSERT, BeginForeignModify will be called
> before ExecForeignInsert to perform any necessary setup.
> In the other cases, this setup has to happen in BeginForeignInsert.

These seem a bit redundant to me because the documentation already says:

<programlisting>
void
BeginForeignModify(ModifyTableState *mtstate,
ResultRelInfo *rinfo,
List *fdw_private,
int subplan_index,
int eflags);
</programlisting>

Begin executing a foreign table modification operation. This
routine is
called during executor startup. It should perform any initialization
needed prior to the actual table modifications. Subsequently,
<function>*ExecForeignInsert*</function>,
<function>ExecForeignUpdate</funct\
ion> or
<function>ExecForeignDelete</function> will be called for each
tuple to be
inserted, updated, or deleted.

And

<programlisting>
void
BeginForeignInsert(ModifyTableState *mtstate,
ResultRelInfo *rinfo);
</programlisting>

Begin executing an insert operation on a foreign table. This
routine is
called right before the first tuple is inserted into the foreign table
in both cases when it is the partition chosen for tuple routing
and the
target specified in a <command>COPY FROM</command> command. It should
perform any initialization needed prior to the actual insertion.
Subsequently, <function>*ExecForeignInsert*</function> will be
called for
each tuple to be inserted into the foreign table.

> Before PostgreSQL v11, a foreign data wrapper could be certain that
> BeginForeignModify is always called before ExecForeignInsert.
> This is no longer true.

OK, how about something like the attached? I reworded this a bit, though.

>>> 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.
>
> That is true, but so far there hasn't been a change to the FDW API that
> caused a callback to be invoked in a different fashion than it used to be.

I agree on that point.

> Perhaps it would have been better to create a new callback like
> ExecForeignCopy with the same signature as ExecForeignInsert so that
> you can use the same callback function for both if you want.
> That would also have avoided the breakage.

If so, we would actually need another new callback
ExecForeignTupleRouting since ExecForeignInsert is also called for
INSERT/UPDATE tuple routing (or another two new callbacks
ExecForeignInsertTupleRouting and ExecForeignUpdateTupleRouting in case
an FDW wants to support either of the tuple routing). My concern about
that is: introducing such a concept might lead to an increase in the
number of callbacks as FDW evolves, increasing the maintenance cost of
the core. So I think it would be better to just have ExecForeignInsert
as a foreign-table alternative for heap_insert, as that would keep the
core much simple.

> At the very least, this should have been mentioned in the list of
> incompatible changes for v11.

Agreed. In the attached, I added a mention to the release notes for PG11.

Best regards,
Etsuro Fujita