Tuple-routing for certain partitioned tables not working as expected
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
Attachments:
ExecSetupPartitionTupleRouting.patchtext/plain; charset=UTF-8; name=ExecSetupPartitionTupleRouting.patchDownload
*** /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
Fujita-san,
On 2017/08/07 12:45, Etsuro Fujita wrote:
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.
Thanks for reporting this. All the issues you mention here seem valid to me.
So, once we add support in general for foreign partition tuple-routing,
we'd still get the same error ("cannot route to foreign partitions") in
the file_fdw's case, but only because
resultRelInfo->ri_FdwRoutine->ExecForeignInsert will be NULL for file_fdw.
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.
Thanks,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
ExecSetupPartitionTupleRouting-v2.patchtext/plain; charset=UTF-8; name=ExecSetupPartitionTupleRouting-v2.patchDownload
*** /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
On 2017/08/07 15:22, Etsuro Fujita wrote:
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.
The updated patch looks good to me, thanks.
Regards,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2017/08/07 15:33, Amit Langote wrote:
On 2017/08/07 15:22, Etsuro Fujita wrote:
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.The updated patch looks good to me, thanks.
Thanks for the review!
Best regards,
Etsuro Fujita
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2017/08/07 15:45, Etsuro Fujita wrote:
On 2017/08/07 15:33, Amit Langote wrote:
On 2017/08/07 15:22, Etsuro Fujita wrote:
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.The updated patch looks good to me, thanks.
Thanks for the review!
If there are no objections, I'll add this to the open item list for v10.
Best regards,
Etsuro Fujita
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Aug 21, 2017 at 7:45 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
If there are no objections, I'll add this to the open item list for v10.
This seems fairly ad-hoc to me. I mean, now you have
CheckValidResultRel not being called in just this one case -- but that
bypasses all the checks that function might do, not just this one. It
so happens that's OK at the moment because CheckCmdReplicaIdentity()
doesn't do anything in the insert case.
I'm somewhat inclined to just view this as a limitation of v10 and fix
it in v11. If you want to fix it in v10, I think we need a different
approach -- just ripping the CheckValidResultRel checks out entirely
doesn't seem like a good idea to me.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2017/08/22 1:08, Robert Haas wrote:
On Mon, Aug 21, 2017 at 7:45 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:If there are no objections, I'll add this to the open item list for v10.
This seems fairly ad-hoc to me. I mean, now you have
CheckValidResultRel not being called in just this one case -- but that
bypasses all the checks that function might do, not just this one. It
so happens that's OK at the moment because CheckCmdReplicaIdentity()
doesn't do anything in the insert case.I'm somewhat inclined to just view this as a limitation of v10 and fix
it in v11. If you want to fix it in v10, I think we need a different
approach -- just ripping the CheckValidResultRel checks out entirely
doesn't seem like a good idea to me.
Before 389af951552f, the relkind check that is now performed by
CheckValidResultRel(), used to be done in InitResultRelInfo(). ISTM, it
was separated out so that certain ResultRelInfos could be initialized
without the explicit relkind check, either because it's taken care of
elsewhere or the table in question is *known* to be a valid result
relation. Maybe, mostly just the former of the two reasons when that
commit went in.
IMO, the latter case applies when initializing ResultRelInfos for
partitions during tuple-routing, because the table types we allow to
become partitions are fairly restricted.
Also, it seems okay to show the error messages that CheckValidResultRel()
shows when the concerned table is *directly* addressed in a query, but the
same error does not seem so user-friendly when emitted for one of the
partitions while tuple-routing is being set up. IMHO, there should be
"tuple routing" somewhere in the error message shown in that case, even if
it's for the total lack of support for inserts by a FDW.
Thanks,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2017/08/22 9:55, Amit Langote wrote:
On 2017/08/22 1:08, Robert Haas wrote:
On Mon, Aug 21, 2017 at 7:45 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:If there are no objections, I'll add this to the open item list for v10.
This seems fairly ad-hoc to me. I mean, now you have
CheckValidResultRel not being called in just this one case -- but that
bypasses all the checks that function might do, not just this one. It
so happens that's OK at the moment because CheckCmdReplicaIdentity()
doesn't do anything in the insert case.I'm somewhat inclined to just view this as a limitation of v10 and fix
it in v11.
That limitation seems too restrictive to me.
If you want to fix it in v10, I think we need a different
approach -- just ripping the CheckValidResultRel checks out entirely
doesn't seem like a good idea to me.Before 389af951552f, the relkind check that is now performed by
CheckValidResultRel(), used to be done in InitResultRelInfo(). ISTM, it
was separated out so that certain ResultRelInfos could be initialized
without the explicit relkind check, either because it's taken care of
elsewhere or the table in question is *known* to be a valid result
relation. Maybe, mostly just the former of the two reasons when that
commit went in.IMO, the latter case applies when initializing ResultRelInfos for
partitions during tuple-routing, because the table types we allow to
become partitions are fairly restricted.
Agreed.
Also, it seems okay to show the error messages that CheckValidResultRel()
shows when the concerned table is *directly* addressed in a query, but the
same error does not seem so user-friendly when emitted for one of the
partitions while tuple-routing is being set up. IMHO, there should be
"tuple routing" somewhere in the error message shown in that case, even if
it's for the total lack of support for inserts by a FDW.
Agreed, but I'd vote for fixing this in v10 as proposed; I agree that
just ripping the CheckValidResultRel checks out entirely is not a good
idea, but that seems OK to me at least as a fix just for v10.
Best regards,
Etsuro Fujita
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Aug 23, 2017 at 4:55 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
Agreed, but I'd vote for fixing this in v10 as proposed; I agree that just
ripping the CheckValidResultRel checks out entirely is not a good idea, but
that seems OK to me at least as a fix just for v10.
I'm still not on-board with having this be the one case where we don't
do CheckValidResultRel. If we want to still call it but pass down
some additional information that can selectively skip certain checks,
I could probably live with that.
At some point we've got to stop developing v10 and just let it be what it is.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2017/08/25 22:26, Robert Haas wrote:
On Wed, Aug 23, 2017 at 4:55 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:Agreed, but I'd vote for fixing this in v10 as proposed; I agree that just
ripping the CheckValidResultRel checks out entirely is not a good idea, but
that seems OK to me at least as a fix just for v10.I'm still not on-board with having this be the one case where we don't
do CheckValidResultRel. If we want to still call it but pass down
some additional information that can selectively skip certain checks,
I could probably live with that.
Another idea would be to not do CheckValidResultRel for partitions in
ExecSetupPartitionTupleRouting; instead, do that the first time the
partition is chosen by ExecFindPartition, and if successfully checked,
initialize the partition's ResultRelInfo and other stuff. (We could
skip this after the first time by checking whether we already have a
valid ResultRelInfo for the chosen partition.) That could allow us to
not only call CheckValidResultRel the way it is, but avoid initializing
useless partitions for which tuples aren't routed at all.
At some point we've got to stop developing v10 and just let it be what it is.
I agree on that point, but ISTM that this is more like a bug.
Best regards,
Etsuro Fujita
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2017/08/29 20:18, Etsuro Fujita wrote:
On 2017/08/25 22:26, Robert Haas wrote:
On Wed, Aug 23, 2017 at 4:55 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:Agreed, but I'd vote for fixing this in v10 as proposed; I agree that just
ripping the CheckValidResultRel checks out entirely is not a good idea,
but
that seems OK to me at least as a fix just for v10.I'm still not on-board with having this be the one case where we don't
do CheckValidResultRel. If we want to still call it but pass down
some additional information that can selectively skip certain checks,
I could probably live with that.Another idea would be to not do CheckValidResultRel for partitions in
ExecSetupPartitionTupleRouting; instead, do that the first time the
partition is chosen by ExecFindPartition, and if successfully checked,
initialize the partition's ResultRelInfo and other stuff. (We could skip
this after the first time by checking whether we already have a valid
ResultRelInfo for the chosen partition.) That could allow us to not only
call CheckValidResultRel the way it is, but avoid initializing useless
partitions for which tuples aren't routed at all.
I too have thought about the idea of lazy initialization of the partition
ResultRelInfos. I think it would be a good idea, but definitely something
for PG 11.
Thanks,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2017/08/30 9:13, Amit Langote wrote:
On 2017/08/29 20:18, Etsuro Fujita wrote:
On 2017/08/25 22:26, Robert Haas wrote:
On Wed, Aug 23, 2017 at 4:55 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:Agreed, but I'd vote for fixing this in v10 as proposed; I agree that just
ripping the CheckValidResultRel checks out entirely is not a good idea,
but
that seems OK to me at least as a fix just for v10.I'm still not on-board with having this be the one case where we don't
do CheckValidResultRel. If we want to still call it but pass down
some additional information that can selectively skip certain checks,
I could probably live with that.Another idea would be to not do CheckValidResultRel for partitions in
ExecSetupPartitionTupleRouting; instead, do that the first time the
partition is chosen by ExecFindPartition, and if successfully checked,
initialize the partition's ResultRelInfo and other stuff. (We could skip
this after the first time by checking whether we already have a valid
ResultRelInfo for the chosen partition.) That could allow us to not only
call CheckValidResultRel the way it is, but avoid initializing useless
partitions for which tuples aren't routed at all.I too have thought about the idea of lazy initialization of the partition
ResultRelInfos. I think it would be a good idea, but definitely something
for PG 11.
Agreed. Here is a patch to skip the CheckValidResultRel checks for a
target table if it's a foreign partition to perform tuple-routing for,
as proposed by Robert.
Best regards,
Etsuro Fujita
Attachments:
ExecSetupPartitionTupleRouting-v3.patchtext/plain; charset=UTF-8; name=ExecSetupPartitionTupleRouting-v3.patchDownload
*** /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
***************
*** 1097,1103 **** InitPlan(QueryDesc *queryDesc, int eflags)
* CheckValidRowMarkRel.
*/
void
! CheckValidResultRel(Relation resultRel, CmdType operation)
{
TriggerDesc *trigDesc = resultRel->trigdesc;
FdwRoutine *fdwroutine;
--- 1097,1103 ----
* CheckValidRowMarkRel.
*/
void
! CheckValidResultRel(Relation resultRel, bool is_partition, CmdType operation)
{
TriggerDesc *trigDesc = resultRel->trigdesc;
FdwRoutine *fdwroutine;
***************
*** 1168,1173 **** CheckValidResultRel(Relation resultRel, CmdType operation)
--- 1168,1181 ----
RelationGetRelationName(resultRel))));
break;
case RELKIND_FOREIGN_TABLE:
+
+ /*
+ * If foreign partition to do tuple-routing for, nothing to do;
+ * it's disallowed elsewhere.
+ */
+ if (is_partition && operation == CMD_INSERT)
+ break;
+
/* Okay only if the FDW supports it */
fdwroutine = GetFdwRoutineForRelation(resultRel, false);
switch (operation)
***************
*** 3310,3316 **** ExecSetupPartitionTupleRouting(Relation rel,
/*
* 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
--- 3318,3324 ----
/*
* Verify result relation is a valid target for the current operation.
*/
! CheckValidResultRel(partrel, true, CMD_INSERT);
/*
* Save a tuple conversion map to convert a tuple routed to this
*** a/src/backend/executor/nodeModifyTable.c
--- b/src/backend/executor/nodeModifyTable.c
***************
*** 1854,1860 **** ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
/*
* Verify result relation is a valid target for the current operation
*/
! CheckValidResultRel(resultRelInfo->ri_RelationDesc, operation);
/*
* If there are indices on the result relation, open them and save
--- 1854,1860 ----
/*
* Verify result relation is a valid target for the current operation
*/
! CheckValidResultRel(resultRelInfo->ri_RelationDesc, false, operation);
/*
* If there are indices on the result relation, open them and save
*** a/src/include/executor/executor.h
--- b/src/include/executor/executor.h
***************
*** 177,183 **** extern void ExecutorEnd(QueryDesc *queryDesc);
extern void standard_ExecutorEnd(QueryDesc *queryDesc);
extern void ExecutorRewind(QueryDesc *queryDesc);
extern bool ExecCheckRTPerms(List *rangeTable, bool ereport_on_violation);
! extern void CheckValidResultRel(Relation resultRel, CmdType operation);
extern void InitResultRelInfo(ResultRelInfo *resultRelInfo,
Relation resultRelationDesc,
Index resultRelationIndex,
--- 177,184 ----
extern void standard_ExecutorEnd(QueryDesc *queryDesc);
extern void ExecutorRewind(QueryDesc *queryDesc);
extern bool ExecCheckRTPerms(List *rangeTable, bool ereport_on_violation);
! extern void CheckValidResultRel(Relation resultRel, bool is_partition,
! CmdType operation);
extern void InitResultRelInfo(ResultRelInfo *resultRelInfo,
Relation resultRelationDesc,
Index resultRelationIndex,
On 2017/08/30 17:20, Etsuro Fujita wrote:
On 2017/08/30 9:13, Amit Langote wrote:
On 2017/08/29 20:18, Etsuro Fujita wrote:
On 2017/08/25 22:26, Robert Haas wrote:
On Wed, Aug 23, 2017 at 4:55 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:Agreed, but I'd vote for fixing this in v10 as proposed; I agree
that just
ripping the CheckValidResultRel checks out entirely is not a good
idea,
but
that seems OK to me at least as a fix just for v10.I'm still not on-board with having this be the one case where we don't
do CheckValidResultRel. If we want to still call it but pass down
some additional information that can selectively skip certain checks,
I could probably live with that.Another idea would be to not do CheckValidResultRel for partitions in
ExecSetupPartitionTupleRouting; instead, do that the first time the
partition is chosen by ExecFindPartition, and if successfully checked,
initialize the partition's ResultRelInfo and other stuff. (We could
skip
this after the first time by checking whether we already have a valid
ResultRelInfo for the chosen partition.) That could allow us to not
only
call CheckValidResultRel the way it is, but avoid initializing useless
partitions for which tuples aren't routed at all.I too have thought about the idea of lazy initialization of the partition
ResultRelInfos. I think it would be a good idea, but definitely
something
for PG 11.Agreed. Here is a patch to skip the CheckValidResultRel checks for a
target table if it's a foreign partition to perform tuple-routing for,
as proposed by Robert.
I'll add this to the open items list for v10.
Best regards,
Etsuro Fujita
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Sep 05, 2017 at 08:35:13PM +0900, Etsuro Fujita wrote:
On 2017/08/30 17:20, Etsuro Fujita wrote:
On 2017/08/30 9:13, Amit Langote wrote:
On 2017/08/29 20:18, Etsuro Fujita wrote:
On 2017/08/25 22:26, Robert Haas wrote:
On Wed, Aug 23, 2017 at 4:55 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:Agreed, but I'd vote for fixing this in v10 as proposed; I agree
that just
ripping the CheckValidResultRel checks out entirely is not a good
idea,
but
that seems OK to me at least as a fix just for v10.I'm still not on-board with having this be the one case where we don't
do CheckValidResultRel.� If we want to still call it but pass down
some additional information that can selectively skip certain checks,
I could probably live with that.Another idea would be to not do CheckValidResultRel for partitions in
ExecSetupPartitionTupleRouting; instead, do that the first time the
partition is chosen by ExecFindPartition, and if successfully checked,
initialize the partition's ResultRelInfo and other stuff.� (We could
skip
this after the first time by checking whether we already have a valid
ResultRelInfo for the chosen partition.)� That could allow us to not
only
call CheckValidResultRel the way it is, but avoid initializing useless
partitions for which tuples aren't routed at all.I too have thought about the idea of lazy initialization of the partition
ResultRelInfos.� I think it would be a good idea, but definitely
something
for PG 11.Agreed.� Here is a patch to skip the CheckValidResultRel checks for a
target table if it's a foreign partition to perform tuple-routing for, as
proposed by Robert.I'll add this to the open items list for v10.
[Action required within three days. This is a generic notification.]
The above-described topic is currently a PostgreSQL 10 open item. Robert,
since you committed the patch believed to have created it, you own this open
item. If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know. Otherwise, please observe the policy on
open item ownership[1]/messages/by-id/20170404140717.GA2675809@tornado.leadboat.com and send a status update within three calendar days of
this message. Include a date for your subsequent status update. Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10. Consequently, I will appreciate your efforts
toward speedy resolution. Thanks.
[1]: /messages/by-id/20170404140717.GA2675809@tornado.leadboat.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2017/08/30 17:20, Etsuro Fujita wrote:
Here is a patch to skip the CheckValidResultRel checks for a
target table if it's a foreign partition to perform tuple-routing for,
as proposed by Robert.
In the patch, to skip the checks, I passed to CheckValidResultRel a new
flag indicating whether the target relation is a partition to do
tuple-routing for, but while updating another patch for tuple-routing
for foreign partitions in PG11, I noticed that it would be better to
pass ResultRelInfo than that flag. The reason is: (1) we can see
whether the relation is such a partition by looking at the
ResultRelInfo's ri_PartitionRoot, instead of that flag, and (2) this is
for tuple-routing for foreign partitions, but we could extend that
function with that argument to do the checks without throwing an error
and save the result in a new member of ResultRelInfo (eg,
ri_PartitionIsValid) so that we can use that info to determine in
ExecInsert whether a foreign partition chosen by ExecFindPartition is
valid for tuple-routing. So, I updated the patch that way. Attached is
an updated version of the patch.
(I discussed about lazy initialization of partition ResultRelInfos
before, but I changed my mind because I noticed that the change to
EXPLAIN for INSERT into a partitioned table, which I'd like to propose
in the tuple-routing-for-foereign-partitions patch, needs partition
ResultRelInfos.)
Best regards,
Etsuro Fujita
Attachments:
ExecSetupPartitionTupleRouting-v4.patchtext/plain; charset=UTF-8; name=ExecSetupPartitionTupleRouting-v4.patchDownload
*** /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
***************
*** 1097,1104 **** InitPlan(QueryDesc *queryDesc, int eflags)
* CheckValidRowMarkRel.
*/
void
! CheckValidResultRel(Relation resultRel, CmdType operation)
{
TriggerDesc *trigDesc = resultRel->trigdesc;
FdwRoutine *fdwroutine;
--- 1097,1105 ----
* CheckValidRowMarkRel.
*/
void
! CheckValidResultRel(ResultRelInfo *resultRelInfo, CmdType operation)
{
+ Relation resultRel = resultRelInfo->ri_RelationDesc;
TriggerDesc *trigDesc = resultRel->trigdesc;
FdwRoutine *fdwroutine;
***************
*** 1169,1178 **** CheckValidResultRel(Relation resultRel, CmdType operation)
break;
case RELKIND_FOREIGN_TABLE:
/* Okay only if the FDW supports it */
! fdwroutine = GetFdwRoutineForRelation(resultRel, false);
switch (operation)
{
case CMD_INSERT:
if (fdwroutine->ExecForeignInsert == NULL)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
--- 1170,1185 ----
break;
case RELKIND_FOREIGN_TABLE:
/* Okay only if the FDW supports it */
! fdwroutine = resultRelInfo->ri_FdwRoutine;
switch (operation)
{
case CMD_INSERT:
+ /*
+ * If foreign partition to do tuple-routing for, skip the
+ * check; it's disallowed elsewhere.
+ */
+ if (resultRelInfo->ri_PartitionRoot)
+ break;
if (fdwroutine->ExecForeignInsert == NULL)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
***************
*** 3308,3318 **** ExecSetupPartitionTupleRouting(Relation rel,
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
* partition from the parent's type to the partition's.
*/
--- 3315,3320 ----
***************
*** 3325,3332 **** ExecSetupPartitionTupleRouting(Relation rel,
rel,
estate->es_instrument);
! estate->es_leaf_result_relations =
! lappend(estate->es_leaf_result_relations, leaf_part_rri);
/*
* Open partition indices (remember we do not support ON CONFLICT in
--- 3327,3336 ----
rel,
estate->es_instrument);
! /*
! * Verify result relation is a valid target for INSERT.
! */
! CheckValidResultRel(leaf_part_rri, CMD_INSERT);
/*
* Open partition indices (remember we do not support ON CONFLICT in
***************
*** 3337,3342 **** ExecSetupPartitionTupleRouting(Relation rel,
--- 3341,3349 ----
leaf_part_rri->ri_IndexRelationDescs == NULL)
ExecOpenIndices(leaf_part_rri, false);
+ estate->es_leaf_result_relations =
+ lappend(estate->es_leaf_result_relations, leaf_part_rri);
+
leaf_part_rri++;
i++;
}
*** a/src/backend/executor/nodeModifyTable.c
--- b/src/backend/executor/nodeModifyTable.c
***************
*** 1854,1860 **** ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
/*
* Verify result relation is a valid target for the current operation
*/
! CheckValidResultRel(resultRelInfo->ri_RelationDesc, operation);
/*
* If there are indices on the result relation, open them and save
--- 1854,1860 ----
/*
* Verify result relation is a valid target for the current operation
*/
! CheckValidResultRel(resultRelInfo, operation);
/*
* If there are indices on the result relation, open them and save
*** a/src/include/executor/executor.h
--- b/src/include/executor/executor.h
***************
*** 177,183 **** extern void ExecutorEnd(QueryDesc *queryDesc);
extern void standard_ExecutorEnd(QueryDesc *queryDesc);
extern void ExecutorRewind(QueryDesc *queryDesc);
extern bool ExecCheckRTPerms(List *rangeTable, bool ereport_on_violation);
! extern void CheckValidResultRel(Relation resultRel, CmdType operation);
extern void InitResultRelInfo(ResultRelInfo *resultRelInfo,
Relation resultRelationDesc,
Index resultRelationIndex,
--- 177,183 ----
extern void standard_ExecutorEnd(QueryDesc *queryDesc);
extern void ExecutorRewind(QueryDesc *queryDesc);
extern bool ExecCheckRTPerms(List *rangeTable, bool ereport_on_violation);
! extern void CheckValidResultRel(ResultRelInfo *resultRelInfo, CmdType operation);
extern void InitResultRelInfo(ResultRelInfo *resultRelInfo,
Relation resultRelationDesc,
Index resultRelationIndex,
On Thu, Sep 7, 2017 at 5:13 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
On 2017/08/30 17:20, Etsuro Fujita wrote:
Here is a patch to skip the CheckValidResultRel checks for a target table
if it's a foreign partition to perform tuple-routing for, as proposed by
Robert.In the patch, to skip the checks, I passed to CheckValidResultRel a new flag
indicating whether the target relation is a partition to do tuple-routing
for, but while updating another patch for tuple-routing for foreign
partitions in PG11, I noticed that it would be better to pass ResultRelInfo
than that flag. The reason is: (1) we can see whether the relation is such
a partition by looking at the ResultRelInfo's ri_PartitionRoot, instead of
that flag, and (2) this is for tuple-routing for foreign partitions, but we
could extend that function with that argument to do the checks without
throwing an error and save the result in a new member of ResultRelInfo (eg,
ri_PartitionIsValid) so that we can use that info to determine in ExecInsert
whether a foreign partition chosen by ExecFindPartition is valid for
tuple-routing. So, I updated the patch that way. Attached is an updated
version of the patch.
OK, committed to master and v10. I am less convinced than you that
this was a must-fix issue, but it's not a very invasive change the way
you did it here.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2017/09/08 0:21, Robert Haas wrote:
On Thu, Sep 7, 2017 at 5:13 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:On 2017/08/30 17:20, Etsuro Fujita wrote:
Here is a patch to skip the CheckValidResultRel checks for a target table
if it's a foreign partition to perform tuple-routing for, as proposed by
Robert.In the patch, to skip the checks, I passed to CheckValidResultRel a new flag
indicating whether the target relation is a partition to do tuple-routing
for, but while updating another patch for tuple-routing for foreign
partitions in PG11, I noticed that it would be better to pass ResultRelInfo
than that flag. The reason is: (1) we can see whether the relation is such
a partition by looking at the ResultRelInfo's ri_PartitionRoot, instead of
that flag, and (2) this is for tuple-routing for foreign partitions, but we
could extend that function with that argument to do the checks without
throwing an error and save the result in a new member of ResultRelInfo (eg,
ri_PartitionIsValid) so that we can use that info to determine in ExecInsert
whether a foreign partition chosen by ExecFindPartition is valid for
tuple-routing. So, I updated the patch that way. Attached is an updated
version of the patch.OK, committed to master and v10. I am less convinced than you that
this was a must-fix issue, but it's not a very invasive change the way
you did it here.
Thank you!
I'll update the tuple-routing-for-foreign-partitions patch that way.
Best regards,
Etsuro Fujita
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers