Message-ID: <5CC1B358.5090205@lab.ntt.co.jp>
Date: Thu, 25 Apr 2019 22:17:12 +0900
From: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64;
 rv:11.0) Gecko/20120327 Thunderbird/11.0.1
MIME-Version: 1.0
Subject: Re: pgsql: Allow insert and update tuple routing and COPY for foreign
 table
References: <E1f4aif-0004qo-A6@gemulon.postgresql.org>
   <ed0b7471dce91169d871347cf003d8094ab96895.camel@cybertec.at>
   <5CBDB770.2060602@lab.ntt.co.jp>
  <b459d1b1b4eb8c0d538a2c5b70e131af3c391954.camel@cybertec.at>
  <5CC04E72.90405@lab.ntt.co.jp>
 <bf36a0288e8f31b4f2f40952e225bf892dc1ffc5.camel@cybertec.at>
In-Reply-To: <bf36a0288e8f31b4f2f40952e225bf892dc1ffc5.camel@cybertec.at>
Content-Type: multipart/mixed;
 boundary="------------050108090608010006080706"
To: Laurenz Albe <laurenz.albe@cybertec.at>
Cc: Robert Haas <rhaas@postgresql.org>, pgsql-committers@lists.postgresql.org,
        pgsql-hackers@lists.postgresql.org
Precedence: bulk

This is a multi-part message in MIME format.
--------------050108090608010006080706
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

(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

--------------050108090608010006080706
Content-Type: text/x-patch;
 name="FDW-documentation.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="FDW-documentation.patch"

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>
 

--------------050108090608010006080706--