(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
diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml
index 7b758bdf09..c2d26ba3c1 100644
--- a/doc/src/sgml/fdwhandler.sgml
+++ b/doc/src/sgml/fdwhandler.sgml
@@ -587,6 +587,14 @@ ExecForeignInsert(EState *estate,
with an error message.
</para>
+ <para>
+ Note that this function is also called when inserting routed tuples into
+ a foreign-table partition or executing <command>COPY FROM</command> on
+ a foreign table, in which case it is called in a different way than it
+ is in the <command>INSERT</command> case. See the callback functions
+ described below to support that.
+ </para>
+
<para>
<programlisting>
TupleTableSlot *
@@ -743,6 +751,13 @@ BeginForeignInsert(ModifyTableState *mtstate,
<literal>NULL</literal>, no action is taken for the initialization.
</para>
+ <para>
+ Note that if the FDW does not support routable foreign-table partitions
+ and/or executing <command>COPY FROM</command> on foreign tables, this
+ function or <function>ExecForeignInsert</function> subsequently called
+ must throw error as needed.
+ </para>
+
<para>
<programlisting>
void
diff --git a/doc/src/sgml/release-11.sgml b/doc/src/sgml/release-11.sgml
index 14e2726f0c..7994e5d269 100644
--- a/doc/src/sgml/release-11.sgml
+++ b/doc/src/sgml/release-11.sgml
@@ -2594,6 +2594,9 @@ Branch: REL9_3_STABLE [84261eb10] 2018-10-19 17:02:26 -0400
<para>
This is supported by <filename>postgres_fdw</filename>
foreign tables.
+ Since the <function>ExecForeignInsert</function> callback function
+ is called for this in a different way than it used to be, foreign
+ data wrappers must be modified to cope with the change.
</para>
</listitem>
