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.ets...@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
*** /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,
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
