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>
From: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
Message-ID: <d76c786b-8d10-f776-527e-2997ccbe0bee@lab.ntt.co.jp>
Date: Mon, 22 Apr 2019 14:51:16 +0900
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.6.1
MIME-Version: 1.0
In-Reply-To: <ed0b7471dce91169d871347cf003d8094ab96895.camel@cybertec.at>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
To: Laurenz Albe <laurenz.albe@cybertec.at>,
        Robert Haas <rhaas@postgresql.org>,
        pgsql-committers@lists.postgresql.org,
        pgsql-hackers@lists.postgresql.org
Precedence: bulk

Hi,

On 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
> 
> This commit makes life hard for foreign data wrappers that support
> data modifications.
> 
> 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.
> 
> So this commit silently turns a functioning FDW into a broken FDW.
> That is not nice.  Probably not every FDW is aware of this change, and
> maybe there are FDWs that support INSERT but don't want to support COPY
> for some reason.

That seems like an oversight to me.  I agree that we had better checked
that a table's FDW provides BeginForeignInsert() before proceeding with
copying into the table, as your patch teaches CopyFrom() to do.

> I propose that PostgreSQL only allows COPY FROM on a foreign table if the FDW
> implements BeginForeignInsert.  The attached patch implements that.

Looks good to me, including the documentation change.

> I think this should be backpatched to v11.

Agreed.

Thanks,
Amit