On 2017/08/07 13:11, Amit Langote wrote:> The patch looks good too.
Although, looking at the following hunk:
+ Assert(partrel->rd_rel->relkind == RELKIND_RELATION ||
+ partrel->rd_rel->relkind == RELKIND_FOREIGN_TABLE);
+
/*
* Verify result relation is a valid target for the current
operation.
*/
! if (partrel->rd_rel->relkind == RELKIND_RELATION)
! CheckValidResultRel(partrel, CMD_INSERT);
makes me now wonder if we need the CheckValidResultRel check at all. The
only check currently in place for RELKIND_RELATION is
CheckCmdReplicaIdentity(), which is a noop (returns true) for inserts anyway.
Good point! I left the verification for a plain table because that is
harmless but as you mentioned, that is nothing but an overhead. So,
here is a new version which removes the verification at all from
ExecSetupPartitionTupleRouting.
Best regards,
Etsuro Fujita
*** /dev/null
--- b/contrib/file_fdw/data/list1.csv
***************
*** 0 ****
--- 1,2 ----
+ 1,foo
+ 1,bar
*** /dev/null
--- b/contrib/file_fdw/data/list2.bad
***************
*** 0 ****
--- 1,2 ----
+ 2,baz
+ 1,qux
*** /dev/null
--- b/contrib/file_fdw/data/list2.csv
***************
*** 0 ****
--- 1,2 ----
+ 2,baz
+ 2,qux
*** a/contrib/file_fdw/input/file_fdw.source
--- b/contrib/file_fdw/input/file_fdw.source
***************
*** 162,167 **** SELECT tableoid::regclass, * FROM agg FOR UPDATE;
--- 162,188 ----
ALTER FOREIGN TABLE agg_csv NO INHERIT agg;
DROP TABLE agg;
+ -- declarative partitioning tests
+ SET ROLE regress_file_fdw_superuser;
+ CREATE TABLE pt (a int, b text) partition by list (a);
+ CREATE FOREIGN TABLE p1 partition of pt for values in (1) SERVER file_server
+ OPTIONS (format 'csv', filename '@abs_srcdir@/data/list1.csv', delimiter ',');
+ CREATE TABLE p2 partition of pt for values in (2);
+ SELECT tableoid::regclass, * FROM pt;
+ SELECT tableoid::regclass, * FROM p1;
+ SELECT tableoid::regclass, * FROM p2;
+ COPY pt FROM '@abs_srcdir@/data/list2.bad' with (format 'csv', delimiter
','); -- ERROR
+ COPY pt FROM '@abs_srcdir@/data/list2.csv' with (format 'csv', delimiter ',');
+ SELECT tableoid::regclass, * FROM pt;
+ SELECT tableoid::regclass, * FROM p1;
+ SELECT tableoid::regclass, * FROM p2;
+ INSERT INTO pt VALUES (1, 'xyzzy'); -- ERROR
+ INSERT INTO pt VALUES (2, 'xyzzy');
+ SELECT tableoid::regclass, * FROM pt;
+ SELECT tableoid::regclass, * FROM p1;
+ SELECT tableoid::regclass, * FROM p2;
+ DROP TABLE pt;
+
-- privilege tests
SET ROLE regress_file_fdw_superuser;
SELECT * FROM agg_text ORDER BY a;
*** a/contrib/file_fdw/output/file_fdw.source
--- b/contrib/file_fdw/output/file_fdw.source
***************
*** 289,294 **** SELECT tableoid::regclass, * FROM agg FOR UPDATE;
--- 289,375 ----
ALTER FOREIGN TABLE agg_csv NO INHERIT agg;
DROP TABLE agg;
+ -- declarative partitioning tests
+ SET ROLE regress_file_fdw_superuser;
+ CREATE TABLE pt (a int, b text) partition by list (a);
+ CREATE FOREIGN TABLE p1 partition of pt for values in (1) SERVER file_server
+ OPTIONS (format 'csv', filename '@abs_srcdir@/data/list1.csv', delimiter ',');
+ CREATE TABLE p2 partition of pt for values in (2);
+ SELECT tableoid::regclass, * FROM pt;
+ tableoid | a | b
+ ----------+---+-----
+ p1 | 1 | foo
+ p1 | 1 | bar
+ (2 rows)
+
+ SELECT tableoid::regclass, * FROM p1;
+ tableoid | a | b
+ ----------+---+-----
+ p1 | 1 | foo
+ p1 | 1 | bar
+ (2 rows)
+
+ SELECT tableoid::regclass, * FROM p2;
+ tableoid | a | b
+ ----------+---+---
+ (0 rows)
+
+ COPY pt FROM '@abs_srcdir@/data/list2.bad' with (format 'csv', delimiter
','); -- ERROR
+ ERROR: cannot route inserted tuples to a foreign table
+ CONTEXT: COPY pt, line 2: "1,qux"
+ COPY pt FROM '@abs_srcdir@/data/list2.csv' with (format 'csv', delimiter ',');
+ SELECT tableoid::regclass, * FROM pt;
+ tableoid | a | b
+ ----------+---+-----
+ p1 | 1 | foo
+ p1 | 1 | bar
+ p2 | 2 | baz
+ p2 | 2 | qux
+ (4 rows)
+
+ SELECT tableoid::regclass, * FROM p1;
+ tableoid | a | b
+ ----------+---+-----
+ p1 | 1 | foo
+ p1 | 1 | bar
+ (2 rows)
+
+ SELECT tableoid::regclass, * FROM p2;
+ tableoid | a | b
+ ----------+---+-----
+ p2 | 2 | baz
+ p2 | 2 | qux
+ (2 rows)
+
+ INSERT INTO pt VALUES (1, 'xyzzy'); -- ERROR
+ ERROR: cannot route inserted tuples to a foreign table
+ INSERT INTO pt VALUES (2, 'xyzzy');
+ SELECT tableoid::regclass, * FROM pt;
+ tableoid | a | b
+ ----------+---+-------
+ p1 | 1 | foo
+ p1 | 1 | bar
+ p2 | 2 | baz
+ p2 | 2 | qux
+ p2 | 2 | xyzzy
+ (5 rows)
+
+ SELECT tableoid::regclass, * FROM p1;
+ tableoid | a | b
+ ----------+---+-----
+ p1 | 1 | foo
+ p1 | 1 | bar
+ (2 rows)
+
+ SELECT tableoid::regclass, * FROM p2;
+ tableoid | a | b
+ ----------+---+-------
+ p2 | 2 | baz
+ p2 | 2 | qux
+ p2 | 2 | xyzzy
+ (3 rows)
+
+ DROP TABLE pt;
-- privilege tests
SET ROLE regress_file_fdw_superuser;
SELECT * FROM agg_text ORDER BY a;
*** a/src/backend/executor/execMain.c
--- b/src/backend/executor/execMain.c
***************
*** 3279,3290 **** ExecSetupPartitionTupleRouting(Relation rel,
* closed by the caller.
*/
partrel = heap_open(lfirst_oid(cell), NoLock);
- part_tupdesc = RelationGetDescr(partrel);
! /*
! * Verify result relation is a valid target for the current
operation.
! */
! CheckValidResultRel(partrel, CMD_INSERT);
/*
* Save a tuple conversion map to convert a tuple routed to this
--- 3279,3290 ----
* closed by the caller.
*/
partrel = heap_open(lfirst_oid(cell), NoLock);
! /* Should be a plain table or foreign table */
! Assert(partrel->rd_rel->relkind == RELKIND_RELATION ||
! partrel->rd_rel->relkind == RELKIND_FOREIGN_TABLE);
!
! part_tupdesc = RelationGetDescr(partrel);
/*
* Save a tuple conversion map to convert a tuple routed to this
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
