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>
Cc: Luis Carril <luis.carril@swarm64.com>
Cc: pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
Cc: PG Bug reporting form <noreply@postgresql.org>
Cc: Michael Paquier <michael.paquier@gmail.com>
Subject: Re: BUG #15552: Unexpected error in COPY to a foreign table in a transaction
Date: Thu, 20 Dec 2018 09:31:40 +0900
Message-ID: <20181220003140.GE27104@paquier.xyz> (raw)
In-Reply-To: <5C1A0B6C.1040502@lab.ntt.co.jp>
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>
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 the
> view case as well, though.
Agreed.
>> Would you like to
>> update the patch yourself?
>
> I'd be happy if you worked on that.
Attached is the patch with two new test cases blowing with wal_level =
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
Attachments:
[text/x-diff] no_heap_opt_fdw_copy-v5.patch (4.8K, ../20181220003140.GE27104@paquier.xyz/2-no_heap_opt_fdw_copy-v5.patch)
download | inline diff:diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.outindex 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 +----+-----+ 1 | foo+ 2 | bar+(2 rows)++drop foreign table rem3;+drop table loc3;
-- ===================================================================
-- test IMPORT FOREIGN SCHEMA
-- ===================================================================
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sqlindex 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;
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;+1 foo+2 bar+\.+commit;+select * from rem3;+drop foreign table rem3;+drop table loc3;+
-- ===================================================================
-- test IMPORT FOREIGN SCHEMA
-- ===================================================================
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.cindex 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 != RELKIND_PARTITIONED_TABLE &&+ if (RELKIND_CAN_HAVE_STORAGE(cstate->rel->rd_rel->relkind) &&
(cstate->rel->rd_createSubid != InvalidSubTransactionId ||
cstate->rel->rd_newRelfilenodeSubid != InvalidSubTransactionId))
{
diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expected/copy2.outindex 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)
+-- 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 +----+-------+ 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.sqlindex e36df8858e..a35f455fb5 100644--- a/src/test/regress/sql/copy2.sql+++ b/src/test/regress/sql/copy2.sql@@ -398,6 +398,21 @@ test1
SELECT * FROM instead_of_insert_tbl;
+-- 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;
-- 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();
[application/pgp-signature] signature.asc (832B, ../20181220003140.GE27104@paquier.xyz/3-signature.asc)
download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: michael@paquier.xyz, fujita.etsuro@lab.ntt.co.jp, Langote_Amit_f8@lab.ntt.co.jp, luis.carril@swarm64.com, pgsql-bugs@lists.postgresql.org, noreply@postgresql.org, michael.paquier@gmail.com
Subject: Re: BUG #15552: Unexpected error in COPY to a foreign table in a transaction
In-Reply-To: <20181220003140.GE27104@paquier.xyz>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox