Date: Thu, 20 Dec 2018 09:31:40 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
Cc: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>,
	Luis Carril <luis.carril@swarm64.com>,
	"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: <20181220003140.GE27104@paquier.xyz>
References: 
 <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>
 <20181219023814.GA6577@paquier.xyz>
 <6daa6d4a-d4a9-bd2c-0b84-c4e31d2b9844@lab.ntt.co.jp>
 <5C19F7FF.9060904@lab.ntt.co.jp>
 <20181219083422.GA680@paquier.xyz>
 <5C1A0B6C.1040502@lab.ntt.co.jp>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="lIrNkN/7tmsD/ALM"
Content-Disposition: inline
In-Reply-To: <5C1A0B6C.1040502@lab.ntt.co.jp>
Precedence: bulk


--lIrNkN/7tmsD/ALM
Content-Type: multipart/mixed; boundary="BQPnanjtCNWHyqYD"
Content-Disposition: inline


--BQPnanjtCNWHyqYD
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Dec 19, 2018 at 06:12:12PM +0900, Etsuro Fujita wrote:
> I think so too.  I think it might be better to add regression tests for t=
he
> view case as well, though.

Agreed.

>> Would you like to
>> update the patch yourself?
>=20
> I'd be happy if you worked on that.

Attached is the patch with two new test cases blowing with wal_level =3D
minimal.  On HEAD, I suggest that we use RELKIND_CAN_HAVE_STORAGE to not
fall again in this trap in the future.  For back-branches, let's just
add the appropriate relkind checks as suggested upthread.  Please note
that the attached patch is for HEAD, and I'll adjust for other branches.
The view part needs to go down to v10, and the part for foreign tables
down to v11.

Thoughts?
--
Michael

--BQPnanjtCNWHyqYD
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="no_heap_opt_fdw_copy-v5.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..9f6d7ed85b 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 relation types which do not
+	 * have any physical storage, with foreign tables and views using
+	 * INSTEAD OF triggers entering in this category.
 	 *----------
 	 */
 	/* createSubid is creation check, newRelfilenodeSubid is truncation check=
 */
-	if (cstate->rel->rd_rel->relkind !=3D RELKIND_PARTITIONED_TABLE &&
+	if (RELKIND_CAN_HAVE_STORAGE(cstate->rel->rd_rel->relkind) &&
 		(cstate->rel->rd_createSubid !=3D InvalidSubTransactionId ||
 		 cstate->rel->rd_newRelfilenodeSubid !=3D InvalidSubTransactionId))
 	{
diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expecte=
d/copy2.out
index 19bb538411..1d5bed5cc5 100644
--- a/src/test/regress/expected/copy2.out
+++ b/src/test/regress/expected/copy2.out
@@ -545,6 +545,23 @@ SELECT * FROM instead_of_insert_tbl;
   1 | test1
 (1 row)
=20
+-- Test of COPY optimization with view using INSTEAD OF INSERT
+-- trigger when relation is created in the same transaction as
+-- when COPY is executed.
+BEGIN;
+CREATE VIEW instead_of_insert_tbl_view_2 as select ''::text as str;
+CREATE TRIGGER trig_instead_of_insert_tbl_view_2
+  INSTEAD OF INSERT ON instead_of_insert_tbl_view_2
+  FOR EACH ROW EXECUTE PROCEDURE fun_instead_of_insert_tbl();
+COPY instead_of_insert_tbl_view_2 FROM stdin;
+SELECT * FROM instead_of_insert_tbl;
+ id | name =20
+----+-------
+  1 | test1
+  2 | test1
+(2 rows)
+
+COMMIT;
 -- clean up
 DROP TABLE forcetest;
 DROP TABLE vistest;
@@ -557,4 +574,5 @@ DROP FUNCTION fn_x_before();
 DROP FUNCTION fn_x_after();
 DROP TABLE instead_of_insert_tbl;
 DROP VIEW instead_of_insert_tbl_view;
+DROP VIEW instead_of_insert_tbl_view_2;
 DROP FUNCTION fun_instead_of_insert_tbl();
diff --git a/src/test/regress/sql/copy2.sql b/src/test/regress/sql/copy2.sql
index e36df8858e..a35f455fb5 100644
--- a/src/test/regress/sql/copy2.sql
+++ b/src/test/regress/sql/copy2.sql
@@ -398,6 +398,21 @@ test1
=20
 SELECT * FROM instead_of_insert_tbl;
=20
+-- Test of COPY optimization with view using INSTEAD OF INSERT
+-- trigger when relation is created in the same transaction as
+-- when COPY is executed.
+BEGIN;
+CREATE VIEW instead_of_insert_tbl_view_2 as select ''::text as str;
+CREATE TRIGGER trig_instead_of_insert_tbl_view_2
+  INSTEAD OF INSERT ON instead_of_insert_tbl_view_2
+  FOR EACH ROW EXECUTE PROCEDURE fun_instead_of_insert_tbl();
+
+COPY instead_of_insert_tbl_view_2 FROM stdin;
+test1
+\.
+
+SELECT * FROM instead_of_insert_tbl;
+COMMIT;
=20
 -- clean up
 DROP TABLE forcetest;
@@ -411,4 +426,5 @@ DROP FUNCTION fn_x_before();
 DROP FUNCTION fn_x_after();
 DROP TABLE instead_of_insert_tbl;
 DROP VIEW instead_of_insert_tbl_view;
+DROP VIEW instead_of_insert_tbl_view_2;
 DROP FUNCTION fun_instead_of_insert_tbl();

--BQPnanjtCNWHyqYD--

--lIrNkN/7tmsD/ALM
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAlwa4uwACgkQnvQgOdby
QH3xdQ/+LXk4J1vSEmI+itZeiCXLd5OrO30aL0Hsd5CNKKxCnJdGKqNfe42xXL45
tjOGo2fvHTOebUr11Ox8TiCj6ICVXQ3UVol+oeSR9wNIRROuPuiTBnEgJjxljtRr
akcALo0CbvBnfTxzC6LLtrS9DeMVx1X3gia4LKzkf1sKJMm1qSFF8JAHjBrWM9zW
7kghFiXf6zu46BSCzq1LnWY5oyASX2uPP52PN2fB4Z+bEdnTuD+OrLXjJTB+fW2L
QANlvwIpWDV63vbVrP+OL9nXoEEhdnLgyKQIRyFEdZ5hV0kk5A0x/pTa7pHeXqCZ
7xb9Z780AR294OyfAjrGTF+UUE5G049A3KnrcP0u9xW4Zf+OJgsHl/Ppd8IxPj4S
3MG4nf6igPP2pAk68clluWpyU3KkoXp+1+LAq453UjHqdUItk32Nxb5RfcSXvERv
qcIdHeFJ/UbGTB7KNlPckE/oK6Dm977f3cz0MzMorwgkve5GlJwcQmUju9QgTL7z
mJR093UoLXGfM/8W87iGjsZExWTLgqetEyaEeAvDLutn/z43gW8xZcPZnCCypcPQ
JumiKfEJASgT61TsfsqH/70jrCJlw4RZfUs6HtQOKSg+Xat0EJk9jk9IICbLbmVk
rHYnaz8ubGhjavRx21NHPpOJFYTvPHQyR8684+zyNCLF5Dojs5Q=
=/A3l
-----END PGP SIGNATURE-----

--lIrNkN/7tmsD/ALM--