Date: Wed, 19 Dec 2018 11:38:14 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
Cc: Luis Carril <luis.carril@swarm64.com>,
	Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>,
	"pgsql-bugs@lists.postgresql.org" <pgsql-bugs@lists.postgresql.org>,
	PG Bug reporting form <noreply@postgresql.org>,
	Michael Paquier <michael.paquier@gmail.com>
Subject: Re: BUG #15552: Unexpected error in COPY to a foreign table in a
 transaction
Message-ID: <20181219023814.GA6577@paquier.xyz>
References: <15552-c64aa14c5c22f63c@postgresql.org>
 <8e82d10d-cc16-eed4-7634-3654f59d8053@lab.ntt.co.jp>
 <LEJPR01MB00109D0BECB8A961074A7CAFE7BC0@LEJPR01MB0010.DEUPRD01.PROD.OUTLOOK.DE>
 <30718b48-8020-329a-c6f0-74e1222e5283@lab.ntt.co.jp>
 <5C18ECBB.2000507@lab.ntt.co.jp>
 <LEJPR01MB001074FD67CB65B539122E4DE7BD0@LEJPR01MB0010.DEUPRD01.PROD.OUTLOOK.DE>
 <3539e73a-313a-5406-27c7-850affb47921@lab.ntt.co.jp>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="ftEhullJWpWg/VHq"
Content-Disposition: inline
In-Reply-To: <3539e73a-313a-5406-27c7-850affb47921@lab.ntt.co.jp>
Precedence: bulk


--ftEhullJWpWg/VHq
Content-Type: multipart/mixed; boundary="KsGdsel6WgEHnImy"
Content-Disposition: inline


--KsGdsel6WgEHnImy
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Wed, Dec 19, 2018 at 10:19:58AM +0900, Amit Langote wrote:
> Not sure that's a good name to give to the test file, although since I'm
> not greatly enthusiastic about adding a TAP test for this, I don't have a
> good suggestion.  Maybe, choose a more generic name?  Michael, any
> suggestions?

I have looked at the proposal seriously myself, and adding a TAP test
for that edge case is overdoing it.  So why not just adding a test to
postgres_fdw.sql?  The failure can be triggered with installcheck and
wal_level =3D minimal.  Not everybody tests with non-default options, but
some of us do.

> @@ -2401,6 +2401,7 @@ CopyFrom(CopyState cstate)
>  ^I */
>  ^I/* createSubid is creation check, newRelfilenodeSubid is truncation
> check */
>  ^Iif (cstate->rel->rd_rel->relkind !=3D RELKIND_PARTITIONED_TABLE &&
> +^I    cstate->rel->rd_rel->relkind !=3D RELKIND_FOREIGN_TABLE &&
>  ^I^I(cstate->rel->rd_createSubid !=3D InvalidSubTransactionId ||
>  ^I^I cstate->rel->rd_newRelfilenodeSubid !=3D InvalidSubTransactionId))
>  ^I{
>=20
> It seems you've used spaces to indent the newly added line.  Please,
> change that to a tab like neighboring lines.

Indeed.

Attached is a more simple, updated, patch which adds a new test and a
comment.  I would rather not play with the semantics of heap_sync() on
the back branches as well as on a thread dealing with a bug about COPY
with foreign tables.  Such discussions deserve a larger audience on
-hackers.

it is a bit funny to see COPY FREEZE working for foreign tables
actually, but perhaps this has some use-cases for some FDWs, so I'd
rather not touch it.
--
Michael

--KsGdsel6WgEHnImy
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="no_heap_opt_fdw_copy-v4.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postg=
res_fdw/expected/postgres_fdw.out
index 666548ba69..bb92d9d37a 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -7981,6 +7981,22 @@ drop trigger rem2_trig_row_before on rem2;
 drop trigger rem2_trig_row_after on rem2;
 drop trigger loc2_trig_row_before_insert on loc2;
 delete from rem2;
+-- test COPY FROM with foreign table created in the same transaction
+create table loc3 (f1 int, f2 text);
+begin;
+create foreign table rem3 (f1 int, f2 text)
+	server loopback options(table_name 'loc3');
+copy rem3 from stdin;
+commit;
+select * from rem3;
+ f1 | f2 =20
+----+-----
+  1 | foo
+  2 | bar
+(2 rows)
+
+drop foreign table rem3;
+drop table loc3;
 -- =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
 -- test IMPORT FOREIGN SCHEMA
 -- =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_f=
dw/sql/postgres_fdw.sql
index f963e99ab6..f438165650 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -2124,6 +2124,20 @@ drop trigger loc2_trig_row_before_insert on loc2;
=20
 delete from rem2;
=20
+-- test COPY FROM with foreign table created in the same transaction
+create table loc3 (f1 int, f2 text);
+begin;
+create foreign table rem3 (f1 int, f2 text)
+	server loopback options(table_name 'loc3');
+copy rem3 from stdin;
+1	foo
+2	bar
+\.
+commit;
+select * from rem3;
+drop foreign table rem3;
+drop table loc3;
+
 -- =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
 -- test IMPORT FOREIGN SCHEMA
 -- =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 4311e16007..4d39509c61 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2397,10 +2397,14 @@ CopyFrom(CopyState cstate)
 	 * possible to improve on this, but it does mean maintaining heap insert
 	 * option flags per partition and setting them when we first open the
 	 * partition.
+	 *
+	 * This optimization is not supported for foreign tables as they have no
+	 * physical storage.
 	 *----------
 	 */
 	/* createSubid is creation check, newRelfilenodeSubid is truncation check=
 */
 	if (cstate->rel->rd_rel->relkind !=3D RELKIND_PARTITIONED_TABLE &&
+		cstate->rel->rd_rel->relkind !=3D RELKIND_FOREIGN_TABLE &&
 		(cstate->rel->rd_createSubid !=3D InvalidSubTransactionId ||
 		 cstate->rel->rd_newRelfilenodeSubid !=3D InvalidSubTransactionId))
 	{

--KsGdsel6WgEHnImy--

--ftEhullJWpWg/VHq
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAlwZrxYACgkQnvQgOdby
QH28Rg//e3Vl1BCBfwzIe06+lZuU73U8jdGr8fP6AjcT1Z+tSkjq2vogINZzRC1G
uDULj6L4KXRK93f28CsfJNPpoxB8HzFVhgI5vsbXg5WR+4XIHWP7ahKyECXwU9kU
oJY9INF+3hb1bK31z+lI2JR1IscEF32b1OxrUF8IJzzDjAECc5LZwCXZRac7iVI6
ie8LW5TZDPFigDoQdDAK+qFj2a5Pngh9DrUGZWdFN5oy0NyjIlWb5ITxfYQ8TvNL
nuZ8pg9A0aW86uHEMEvScJI1LMxNifU7dynfVEchzRUo4qGSilXpL3cl0ovYVF+F
kwiTWsmJwCN1UbuwSDmUGImBu2EHivfdo9gfdONqA0XUgHJ4QtHkMZi9/9lobSjN
xUHlPtdlLrHRTGhjFGJaygqi61ac7SwhB0QnokfTKvwwVRZullWKAZk6o/iiJSMB
nD3hM3VLDnYrTLiqVFmKQWHrfi2KFtIqiI0Ga8uOnqfvMj2HC3loCii0/DhZDxEA
AyuNBtyWkt7QJKf41C5NAO1JLnHGhzsL6Y2bBcNZqq9o/SGOOEDatk1A5Y3CJ7xT
DUibORoruHC7CFbw46XSrd1onjwnomMiodeJcligHAk8Fva8vXjyloXkn/yol1o5
j6kfjTO1cGJku91ISYrW7+Qg2vDnRvs3smX/vDra78u75qKad2g=
=4nqC
-----END PGP SIGNATURE-----

--ftEhullJWpWg/VHq--