Hi,

I noticed that tuple-routing for partitioned tables that contain non-writable foreign partitions doesn't work as expected. Here is an example:


postgres=# create extension file_fdw;
postgres=# create server file_server foreign data wrapper file_fdw;
postgres=# create user mapping for CURRENT_USER server file_server;
postgres=# create table p (a int) partition by list (a);
postgres=# create foreign table t1 partition of p for values in (1) server file_server options (format 'csv', filename '/path/to/file', delimiter ',');
postgres=# create table t2 partition of p for values in (2);
postgres=# insert into p values (1);
ERROR:  cannot insert into foreign table "t1"

Looks good, but:

postgres=# insert into p values (2);
ERROR:  cannot insert into foreign table "t1"

The insert should work but doesn't. (It also seems odd to me that the error message points to t1, not t2.) The reason for that is CheckValidResultRel in ExecSetupPartitionTupleRouting aborts any insert into a partitioned table in the case where the partitioned table contains at least one foreign partition into which the FDW can't insert. I don't think that that is intentional behavior, so I'd like to propose to fix that by skipping CheckValidResultRel for foreign partitions because we can abort an insert into a foreign partition after ExecFindPartition in ExecInsert, by checking to see if resultRelInfo->ri_FdwRoutine is not NULL. Attached is a proposed patch for that. Since COPY FROM has the same issue, I added regression tests for COPY FROM as well as INSERT to file_fdw.

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
***************
*** 3281,3290 **** ExecSetupPartitionTupleRouting(Relation rel,
                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
--- 3281,3294 ----
                partrel = heap_open(lfirst_oid(cell), NoLock);
                part_tupdesc = RelationGetDescr(partrel);
  
+               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);
  
                /*
                 * 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

Reply via email to