Message-ID: <b459d1b1b4eb8c0d538a2c5b70e131af3c391954.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: Mon, 22 Apr 2019 21:37:25 +0200
In-Reply-To: <5CBDB770.2060602@lab.ntt.co.jp>
References: <E1f4aif-0004qo-A6@gemulon.postgresql.org>
	 <ed0b7471dce91169d871347cf003d8094ab96895.camel@cybertec.at>
	 <5CBDB770.2060602@lab.ntt.co.jp>
Organization: Cybertec
Content-Type: multipart/mixed; boundary="=-CWs/4/1ZhK07yf/GNKxB"
MIME-Version: 1.0
Precedence: bulk


--=-CWs/4/1ZhK07yf/GNKxB
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit

On Mon, 2019-04-22 at 21:45 +0900, Etsuro Fujita wrote:

Thanks for looking into this!

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

> > maybe there are FDWs that support INSERT but don't want to support COPY
> > for some reason.
> 
> I agree on that point.
> 
> > I propose that PostgreSQL only allows COPY FROM on a foreign table if the FDW
> > implements BeginForeignInsert.  The attached patch implements that.
> 
> I don't think that is a good idea, because there might be some FDWs that 
> want to support COPY FROM on foreign tables without providing 
> BeginForeignInsert.  (As for INSERT into foreign tables, we actually 
> allow FDWs to support it without providing PlanForeignModify, 
> BeginForeignModify, or EndForeignModify.)
> 
> 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.

My point is that this should not be necessary.

If a FDW worked well with v10, it should continue to work with v11
unless there is a necessity for a compatibility-breaking change.

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.

I realized that my previous patch forgot to check for tuple routing,
updated patch attached.

Yours,
Laurenz Albe

--=-CWs/4/1ZhK07yf/GNKxB
Content-Disposition: attachment;
	filename*0=0001-Foreign-table-COPY-FROM-and-tuple-routing-requires-B.pat;
	filename*1=ch
Content-Type: text/x-patch;
	name="0001-Foreign-table-COPY-FROM-and-tuple-routing-requires-B.patch";
	charset="UTF-8"
Content-Transfer-Encoding: base64

RnJvbSA1NDVhYzlkNTY3ZWE2Y2E2MTBjZTA4MzU1NDUxOTIzY2M3ODdlMTNkIE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBMYXVyZW56IEFsYmUgPGxhdXJlbnouYWxiZUBjeWJlcnRlYy5h
dD4KRGF0ZTogTW9uLCAyMiBBcHIgMjAxOSAyMTozNTowNiArMDIwMApTdWJqZWN0OiBbUEFUQ0hd
IEZvcmVpZ24gdGFibGUgQ09QWSBGUk9NIGFuZCB0dXBsZSByb3V0aW5nIHJlcXVpcmVzCiBCZWdp
bkZvcmVpZ25JbnNlcnQKCkNvbW1pdCAzZDk1NmQ5NTZhIGludHJvZHVjZWQgc3VwcG9ydCBmb3Ig
Zm9yZWlnbiB0YWJsZXMgYXMgcGFydGl0aW9ucwphbmQgQ09QWSBGUk9NIG9uIGZvcmVpZ24gdGFi
bGVzLgoKSWYgYSBmb3JlaWduIGRhdGEgd3JhcHBlciBzdXBwb3J0cyBkYXRhIG1vZGlmaWNhdGlv
bnMsIGJ1dCBlaXRoZXIgaGFzCm5vdCBhZGFwdGVkIHRvIHRoaXMgY2hhbmdlIHlldCBvciBkb2Vz
bid0IHdhbnQgdG8gc3VwcG9ydCBpdApmb3Igb3RoZXIgcmVhc29ucywgaXQgcHJvYmFibHkgZ290
IGJyb2tlbiBieSB0aGUgYWJvdmUgY29tbWl0LApiZWNhdXNlIENPUFkgd2lsbCBqdXN0IGNhbGwg
RXhlY0ZvcmVpZ25JbnNlcnQgYW55d2F5LCB3aGljaCBtaWdodCBub3QKd29yayBiZWNhdXNlIG5l
aXRoZXIgUGxhbkZvcmVpZ25Nb2RpZnkgbm9yIEJlZ2luRm9yZWlnbk1vZGlmeSBoYXZlCmJlZW4g
Y2FsbGVkLgoKVG8gYXZvaWQgYnJlYWtpbmcgdGhpcmQtcGFydHkgZm9yZWlnbiBkYXRhIHdyYXBw
ZXJzIGluIHRoYXQgd2F5LCBhbGxvdwpDT1BZIEZST00gYW5kIHR1cGxlIHJvdXRpbmcgZm9yIGZv
cmVpZ24gdGFibGVzIG9ubHkgaWYgdGhlIGZvcmVpZ24gZGF0YQp3cmFwcGVyIGltcGxlbWVudHMg
QmVnaW5Gb3JlaWduSW5zZXJ0LgotLS0KIGRvYy9zcmMvc2dtbC9mZHdoYW5kbGVyLnNnbWwgICAg
ICAgICB8ICAyICsrCiBzcmMvYmFja2VuZC9jb21tYW5kcy9jb3B5LmMgICAgICAgICAgfCAgNSAr
KysrKwogc3JjL2JhY2tlbmQvZXhlY3V0b3IvZXhlY1BhcnRpdGlvbi5jIHwgMTcgKysrKysrKysr
KysrLS0tLS0KIDMgZmlsZXMgY2hhbmdlZCwgMTkgaW5zZXJ0aW9ucygrKSwgNSBkZWxldGlvbnMo
LSkKCmRpZmYgLS1naXQgYS9kb2Mvc3JjL3NnbWwvZmR3aGFuZGxlci5zZ21sIGIvZG9jL3NyYy9z
Z21sL2Zkd2hhbmRsZXIuc2dtbAppbmRleCAyYzA3YTg2NjM3Li44NGYwNmQ3ZjgxIDEwMDY0NAot
LS0gYS9kb2Mvc3JjL3NnbWwvZmR3aGFuZGxlci5zZ21sCisrKyBiL2RvYy9zcmMvc2dtbC9mZHdo
YW5kbGVyLnNnbWwKQEAgLTcyNCw2ICs3MjQsOCBAQCBCZWdpbkZvcmVpZ25JbnNlcnQoTW9kaWZ5
VGFibGVTdGF0ZSAqbXRzdGF0ZSwKICAgICAgcGVyZm9ybSBhbnkgaW5pdGlhbGl6YXRpb24gbmVl
ZGVkIHByaW9yIHRvIHRoZSBhY3R1YWwgaW5zZXJ0aW9uLgogICAgICBTdWJzZXF1ZW50bHksIDxm
dW5jdGlvbj5FeGVjRm9yZWlnbkluc2VydDwvZnVuY3Rpb24+IHdpbGwgYmUgY2FsbGVkIGZvcgog
ICAgICBlYWNoIHR1cGxlIHRvIGJlIGluc2VydGVkIGludG8gdGhlIGZvcmVpZ24gdGFibGUuCisg
ICAgIEFsbCBmb3JlaWduIGRhdGEgd3JhcHBlcnMgdGhhdCBzdXBwb3J0IDxjb21tYW5kPkNPUFkg
RlJPTTwvY29tbWFuZD4gb3IKKyAgICAgcGFydGl0aW9ucyB0aGF0IGFyZSBmb3JlaWduIHRhYmxl
cyBoYXZlIHRvIHByb3ZpZGUgdGhpcyBjYWxsYmFjayBmdW5jdGlvbi4KICAgICA8L3BhcmE+CiAK
ICAgICA8cGFyYT4KZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL2NvbW1hbmRzL2NvcHkuYyBiL3Ny
Yy9iYWNrZW5kL2NvbW1hbmRzL2NvcHkuYwppbmRleCBjMzkyMThmOGRiLi4yZTA5YWYwMDcwIDEw
MDY0NAotLS0gYS9zcmMvYmFja2VuZC9jb21tYW5kcy9jb3B5LmMKKysrIGIvc3JjL2JhY2tlbmQv
Y29tbWFuZHMvY29weS5jCkBAIC0yODU3LDYgKzI4NTcsMTEgQEAgQ29weUZyb20oQ29weVN0YXRl
IGNzdGF0ZSkKIAkJcmVzdWx0UmVsSW5mby0+cmlfRmR3Um91dGluZS0+QmVnaW5Gb3JlaWduSW5z
ZXJ0ICE9IE5VTEwpCiAJCXJlc3VsdFJlbEluZm8tPnJpX0Zkd1JvdXRpbmUtPkJlZ2luRm9yZWln
bkluc2VydChtdHN0YXRlLAogCQkJCQkJCQkJCQkJCQkgcmVzdWx0UmVsSW5mbyk7CisJZWxzZQor
CQllcmVwb3J0KEVSUk9SLAorCQkJCShlcnJjb2RlKEVSUkNPREVfRkVBVFVSRV9OT1RfU1VQUE9S
VEVEKSwKKwkJCQkgZXJybXNnKCJjYW5ub3QgY29weSB0byBmb3JlaWduIHRhYmxlIFwiJXNcIiIs
CisJCQkJCQlSZWxhdGlvbkdldFJlbGF0aW9uTmFtZShjc3RhdGUtPnJlbCkpKSk7CiAKIAkvKiBQ
cmVwYXJlIHRvIGNhdGNoIEFGVEVSIHRyaWdnZXJzLiAqLwogCUFmdGVyVHJpZ2dlckJlZ2luUXVl
cnkoKTsKZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL2V4ZWN1dG9yL2V4ZWNQYXJ0aXRpb24uYyBi
L3NyYy9iYWNrZW5kL2V4ZWN1dG9yL2V4ZWNQYXJ0aXRpb24uYwppbmRleCA1MDgwMGFhNWNhLi4w
MzE2ZDRjYmQ0IDEwMDY0NAotLS0gYS9zcmMvYmFja2VuZC9leGVjdXRvci9leGVjUGFydGl0aW9u
LmMKKysrIGIvc3JjL2JhY2tlbmQvZXhlY3V0b3IvZXhlY1BhcnRpdGlvbi5jCkBAIC05MzksMTIg
KzkzOSwxOSBAQCBFeGVjSW5pdFJvdXRpbmdJbmZvKE1vZGlmeVRhYmxlU3RhdGUgKm10c3RhdGUs
CiAJCXBhcnRyb3V0ZWluZm8tPnBpX1BhcnRpdGlvblRvUm9vdE1hcCA9IE5VTEw7CiAKIAkvKgot
CSAqIElmIHRoZSBwYXJ0aXRpb24gaXMgYSBmb3JlaWduIHRhYmxlLCBsZXQgdGhlIEZEVyBpbml0
IGl0c2VsZiBmb3IKLQkgKiByb3V0aW5nIHR1cGxlcyB0byB0aGUgcGFydGl0aW9uLgorCSAqIElm
IHRoZSBwYXJ0aXRpb24gaXMgYSBmb3JlaWduIHRhYmxlIGFuZCB0aGUgRkRXIHN1cHBvcnRzIGl0
LAorCSAqIGxldCBpdCBpbml0IGl0c2VsZiBmb3Igcm91dGluZyB0dXBsZXMgdG8gdGhlIHBhcnRp
dGlvbi4KIAkgKi8KLQlpZiAocGFydFJlbEluZm8tPnJpX0Zkd1JvdXRpbmUgIT0gTlVMTCAmJgot
CQlwYXJ0UmVsSW5mby0+cmlfRmR3Um91dGluZS0+QmVnaW5Gb3JlaWduSW5zZXJ0ICE9IE5VTEwp
Ci0JCXBhcnRSZWxJbmZvLT5yaV9GZHdSb3V0aW5lLT5CZWdpbkZvcmVpZ25JbnNlcnQobXRzdGF0
ZSwgcGFydFJlbEluZm8pOworCWlmIChwYXJ0UmVsSW5mby0+cmlfRmR3Um91dGluZSAhPSBOVUxM
KQorCXsKKwkJaWYgKHBhcnRSZWxJbmZvLT5yaV9GZHdSb3V0aW5lLT5CZWdpbkZvcmVpZ25JbnNl
cnQgIT0gTlVMTCkKKwkJCXBhcnRSZWxJbmZvLT5yaV9GZHdSb3V0aW5lLT5CZWdpbkZvcmVpZ25J
bnNlcnQobXRzdGF0ZSwgcGFydFJlbEluZm8pOworCQllbHNlCisJCQllcmVwb3J0KEVSUk9SLAor
CQkJCQkoZXJyY29kZShFUlJDT0RFX0ZFQVRVUkVfTk9UX1NVUFBPUlRFRCksCisJCQkJCSBlcnJt
c2coImNhbm5vdCByb3V0ZSBpbnNlcnRlZCB0dXBsZXMgdG8gZm9yZWlnbiB0YWJsZSBcIiVzXCIi
LAorCQkJCQkJUmVsYXRpb25HZXRSZWxhdGlvbk5hbWUocGFydFJlbEluZm8tPnJpX1JlbGF0aW9u
RGVzYykpKSk7CisJfQogCiAJcGFydFJlbEluZm8tPnJpX1BhcnRpdGlvbkluZm8gPSBwYXJ0cm91
dGVpbmZvOwogCXBhcnRSZWxJbmZvLT5yaV9Db3B5TXVsdGlJbnNlcnRCdWZmZXIgPSBOVUxMOwot
LSAKMi4yMC4xCgo=


--=-CWs/4/1ZhK07yf/GNKxB--