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.
Before PostgreSQL v11, a foreign data wrapper could be certain that
BeginForeignModify is always called before ExecForeignInsert.
This is no longer true.
> > 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.
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.
But, of course it is too late for that now.
Note that postgres_fdw would have been broken by that API change as well
if it hasn't been patched.
At the very least, this should have been mentioned in the list of
incompatible changes for v11.
Yours,
Laurenz Albe
