Message-ID: <bf36a0288e8f31b4f2f40952e225bf892dc1ffc5.camel@cybertec.at>
Subject: Re: pgsql: Allow insert and update tuple routing and COPY for
 foreign table
From: Laurenz Albe <laurenz.albe@cybertec.at>
To: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
Cc: Robert Haas <rhaas@postgresql.org>,
 pgsql-committers@lists.postgresql.org,  pgsql-hackers@lists.postgresql.org
Date: Wed, 24 Apr 2019 15:04:47 +0200
In-Reply-To: <5CC04E72.90405@lab.ntt.co.jp>
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>
Organization: Cybertec
Content-Type: text/plain; charset="UTF-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

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