Received: from malur.postgresql.org ([217.196.149.56])
	by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256)
	(Exim 4.89)
	(envelope-from <pgsql-hackers-owner+archive@lists.postgresql.org>)
	id 1hJGUS-0002Bm-6Z
	for pgsql-hackers@arkaria.postgresql.org; Wed, 24 Apr 2019 11:55:04 +0000
Received: from localhost ([127.0.0.1] helo=malur.postgresql.org)
	by malur.postgresql.org with esmtp (Exim 4.89)
	(envelope-from <pgsql-hackers-owner+archive@lists.postgresql.org>)
	id 1hJGUQ-00048e-Vw
	for pgsql-hackers@arkaria.postgresql.org; Wed, 24 Apr 2019 11:55:02 +0000
Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29])
	by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256)
	(Exim 4.89)
	(envelope-from <fujita.etsuro@lab.ntt.co.jp>)
	id 1hJGUQ-00048S-N2; Wed, 24 Apr 2019 11:55:02 +0000
Received: from tama500.ecl.ntt.co.jp ([129.60.39.148])
	by magus.postgresql.org with esmtp (Exim 4.89)
	(envelope-from <fujita.etsuro@lab.ntt.co.jp>)
	id 1hJGUM-0005gx-No; Wed, 24 Apr 2019 11:55:02 +0000
Received: from vc1.ecl.ntt.co.jp (vc1.ecl.ntt.co.jp [129.60.86.153])
	by tama500.ecl.ntt.co.jp (8.13.8/8.13.8) with ESMTP id x3OBscSZ013541;
	Wed, 24 Apr 2019 20:54:38 +0900
Received: from vc1.ecl.ntt.co.jp (localhost [127.0.0.1])
	by vc1.ecl.ntt.co.jp (Postfix) with ESMTP id AA3F2EA8351;
	Wed, 24 Apr 2019 20:54:38 +0900 (JST)
Received: from jcms-pop21.ecl.ntt.co.jp (jcms-pop21.ecl.ntt.co.jp
 [129.60.87.134])
	by vc1.ecl.ntt.co.jp (Postfix) with ESMTP id 95D6FEA834E;
	Wed, 24 Apr 2019 20:54:38 +0900 (JST)
Received: from [IPv6:::1] (unknown [129.60.241.75])
	by jcms-pop21.ecl.ntt.co.jp (Postfix) with ESMTPSA id 8C5624003CA;
	Wed, 24 Apr 2019 20:54:38 +0900 (JST)
Message-ID: <5CC04E72.90405@lab.ntt.co.jp>
Date: Wed, 24 Apr 2019 20:54:26 +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>
In-Reply-To: <b459d1b1b4eb8c0d538a2c5b70e131af3c391954.camel@cybertec.at>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
X-CC-Mail-RelayStamp: 1
To: Laurenz Albe <laurenz.albe@cybertec.at>
Cc: Robert Haas <rhaas@postgresql.org>, pgsql-committers@lists.postgresql.org,
        pgsql-hackers@lists.postgresql.org
X-TM-AS-MML: disable
List-Id: <pgsql-hackers.lists.postgresql.org>
List-Help: <https://lists.postgresql.org/manage/>
List-Subscribe: <https://lists.postgresql.org/manage/>
List-Post: <mailto:pgsql-hackers@lists.postgresql.org>
List-Owner: <mailto:pgsql-hackers-owner@lists.postgresql.org>
List-Archive: <https://www.postgresql.org/list/pgsql-hackers>
Precedence: bulk

(2019/04/23 4:37), Laurenz Albe wrote:
> On Mon, 2019-04-22 at 21:45 +0900, Etsuro Fujita wrote:
>> (2019/04/20 20:53), Laurenz Albe wrote:
>>> On Fri, 2018-04-06 at 23:24 +0000, Robert Haas wrote:
>>>> Allow insert and update tuple routing and COPY for foreign tables.
>>>>
>>>> Also enable this for postgres_fdw.
>>>>
>>>> Etsuro Fujita, based on an earlier patch by Amit Langote. The larger
>>>> patch series of which this is a part has been reviewed by Amit
>>>> Langote, David Fetter, Maksim Milyutin, Álvaro Herrera, Stephen Frost,
>>>> and me.  Minor documentation changes to the final version by me.
>>>>
>>>> Discussion: http://postgr.es/m/29906a26-da12-8c86-4fb9-d8f88442f2b9@lab.ntt.co.jp

>>> If a FDW implements ExecForeignInsert, this commit automatically assumes
>>> that it also supports COPY FROM.  It will call ExecForeignInsert without
>>> calling PlanForeignModify and BeginForeignModify, and a FDW that does not
>>> expect that will probably fail.
>>
>> This is not 100% correct; the FDW documentation says:
>>
>>       <para>
>>        Tuples inserted into a partitioned table by
>> <command>INSERT</command>  or
>>        <command>COPY FROM</command>  are routed to partitions.  If an FDW
>>        supports routable foreign-table partitions, it should also provide the
>>        following callback functions.  These functions are also called when
>>        <command>COPY FROM</command>  is executed on a foreign table.
>>       </para>
>
> I don't see the difference between the documentation and what I wrote above.
>
> Before v11, a FDW could expect that ExecForeignInsert is only called if
> BeginForeignModify was called earlier.
> That has silently changed with v11.

I have to admit that the documentation is not sufficient.

>> It's permissible to throw an error in BeginForeignInsert, so what I was
>> thinking for FDWs that don't want to support COPY FROM and
>> INSERT/UPDATE/COPY FROM tuple routing was to provide BeginForeignInsert
>> implementing something like this:
>>
>> static void
>> fooBeginForeignInsert(ModifyTableState *mtstate,
>>                         ResultRelInfo *resultRelInfo)
>> {
>>       Relation    rel = resultRelInfo->ri_RelationDesc;
>>
>>       if (mtstate->ps.plan == NULL)
>>           ereport(ERROR,
>>                   (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>>                    errmsg("cannot copy to foreign table \"%s\"",
>>                           RelationGetRelationName(rel))));
>>       else
>>           ereport(ERROR,
>>                   (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>>                    errmsg("cannot route tuples into foreign table \"%s\"",
>>                           RelationGetRelationName(rel))));
>> }
>
> Sure, it is not hard to modify a FDW to continue working with v11.

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.

> My point is that this should not be necessary.

In my opinion, I think this is necessary...

> 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.

Best regards,
Etsuro Fujita