On 2017/07/26 16:58, Amit Langote wrote: > Rajkumar Raghuwanshi reported [1] on the "UPDATE partition key" thread > that whole-row vars don't play nicely with partitioned table operations. > > For example, when used to convert WITH CHECK OPTION constraint expressions > and RETURNING target list expressions, it will error out if the > expressions contained whole-row vars. That's a bug, because whole-row > vars are legal in those expressions. I think the decision to output error > upon encountering a whole-row in the input expression was based on the > assumption that it will only ever be used to convert partition constraint > expressions, which cannot contain those. So, we can relax that > requirement so that its other users are not bitten by it. > > Attached fixes that.
Attached a revised version of the patch. Updated patch moves the if (found_whole_row) elog(ERROR) bit in map_partition_varattnos to the callers. Only the callers know if whole-row vars are not expected in the expression it's getting converted from map_partition_varattnos. Given the current message text (mentioning "partition key"), it didn't seem appropriate to have the elog inside this function, since it's callable from places where it wouldn't make sense. Thanks, Amit
From 967bef28bb9ac25bb773934963f61c19c3ae7478 Mon Sep 17 00:00:00 2001 From: amit <amitlangot...@gmail.com> Date: Wed, 26 Jul 2017 16:45:46 +0900 Subject: [PATCH] Fix map_partition_varattnos to sometimes accept wholerow vars It was thought that it would never encount wholerow vars in its input expressions (partition constraint expressions for example). But then it was used to convert expressions where wholerow vars are legal, such as, WCO constraint expressions and RETURNING target list members. So, add an argument to tell it whether or not to error on finding wholerows. Adds test in insert.sql and updatable_views.sql. Reported by: Rajkumar Raghuwanshi Report: https://postgr.es/m/CAKcux6%3Dz38gH4K6YAFi%2BYvo5tHTwBL4tam4VM33CAPZ5dDMk1Q%40mail.gmail.com --- src/backend/catalog/partition.c | 17 ++++++++++------- src/backend/commands/tablecmds.c | 8 +++++++- src/backend/executor/nodeModifyTable.c | 18 ++++++++++++++---- src/include/catalog/partition.h | 3 ++- src/test/regress/expected/insert.out | 10 ++++++++++ src/test/regress/expected/updatable_views.out | 10 ++++++++++ src/test/regress/sql/insert.sql | 6 ++++++ src/test/regress/sql/updatable_views.sql | 9 +++++++++ 8 files changed, 68 insertions(+), 13 deletions(-) diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c index e20ddce2db..824898939e 100644 --- a/src/backend/catalog/partition.c +++ b/src/backend/catalog/partition.c @@ -904,10 +904,10 @@ get_qual_from_partbound(Relation rel, Relation parent, */ List * map_partition_varattnos(List *expr, int target_varno, - Relation partrel, Relation parent) + Relation partrel, Relation parent, + bool *found_whole_row) { AttrNumber *part_attnos; - bool found_whole_row; if (expr == NIL) return NIL; @@ -915,14 +915,12 @@ map_partition_varattnos(List *expr, int target_varno, part_attnos = convert_tuples_by_name_map(RelationGetDescr(partrel), RelationGetDescr(parent), gettext_noop("could not convert row type")); + *found_whole_row = false; expr = (List *) map_variable_attnos((Node *) expr, target_varno, 0, part_attnos, RelationGetDescr(parent)->natts, - &found_whole_row); - /* There can never be a whole-row reference here */ - if (found_whole_row) - elog(ERROR, "unexpected whole-row reference found in partition key"); + found_whole_row); return expr; } @@ -1783,6 +1781,7 @@ generate_partition_qual(Relation rel) List *my_qual = NIL, *result = NIL; Relation parent; + bool found_whole_row; /* Guard against stack overflow due to overly deep partition tree */ check_stack_depth(); @@ -1825,7 +1824,11 @@ generate_partition_qual(Relation rel) * in it to bear this relation's attnos. It's safe to assume varno = 1 * here. */ - result = map_partition_varattnos(result, 1, rel, parent); + result = map_partition_varattnos(result, 1, rel, parent, + &found_whole_row); + /* There can never be a whole-row reference here */ + if (found_whole_row) + elog(ERROR, "unexpected whole-row reference found in partition key"); /* Save a copy in the relcache */ oldcxt = MemoryContextSwitchTo(CacheMemoryContext); diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index bb00858ad1..cc5d3d6faf 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -13713,6 +13713,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd) Oid part_relid = lfirst_oid(lc); Relation part_rel; Expr *constr; + bool found_whole_row; /* Lock already taken */ if (part_relid != RelationGetRelid(attachRel)) @@ -13738,7 +13739,12 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd) constr = linitial(partConstraint); tab->partition_constraint = (Expr *) map_partition_varattnos((List *) constr, 1, - part_rel, rel); + part_rel, rel, + &found_whole_row); + /* There can never be a whole-row reference here */ + if (found_whole_row) + elog(ERROR, "unexpected whole-row reference found in partition key"); + /* keep our lock until commit */ if (part_rel != attachRel) heap_close(part_rel, NoLock); diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index 77ba15dd90..670601ab94 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -1988,11 +1988,16 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags) List *mapped_wcoList; List *wcoExprs = NIL; ListCell *ll; + bool found_whole_row; - /* varno = node->nominalRelation */ + /* + * We are passing node->nominalRelation as the varno. wcoList + * might contain whole-row vars which is legal. + */ mapped_wcoList = map_partition_varattnos(wcoList, node->nominalRelation, - partrel, rel); + partrel, rel, + &found_whole_row); foreach(ll, mapped_wcoList) { WithCheckOption *wco = castNode(WithCheckOption, lfirst(ll)); @@ -2061,11 +2066,16 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags) { Relation partrel = resultRelInfo->ri_RelationDesc; List *rlist; + bool found_whole_row; - /* varno = node->nominalRelation */ + /* + * We are passing node->nominalRelation as the varno. + * returningList might contain wholerow vars which is legal. + */ rlist = map_partition_varattnos(returningList, node->nominalRelation, - partrel, rel); + partrel, rel, + &found_whole_row); resultRelInfo->ri_projectReturning = ExecBuildProjectionInfo(rlist, econtext, slot, &mtstate->ps, resultRelInfo->ri_RelationDesc->rd_att); diff --git a/src/include/catalog/partition.h b/src/include/catalog/partition.h index f10879a162..434ded37d7 100644 --- a/src/include/catalog/partition.h +++ b/src/include/catalog/partition.h @@ -80,7 +80,8 @@ extern Oid get_partition_parent(Oid relid); extern List *get_qual_from_partbound(Relation rel, Relation parent, PartitionBoundSpec *spec); extern List *map_partition_varattnos(List *expr, int target_varno, - Relation partrel, Relation parent); + Relation partrel, Relation parent, + bool *found_whole_row); extern List *RelationGetPartitionQual(Relation rel); extern Expr *get_partition_qual_relid(Oid relid); diff --git a/src/test/regress/expected/insert.out b/src/test/regress/expected/insert.out index 0dcc86fef4..b642de39d5 100644 --- a/src/test/regress/expected/insert.out +++ b/src/test/regress/expected/insert.out @@ -659,3 +659,13 @@ select tableoid::regclass, * from mcrparted order by a, b; (11 rows) drop table mcrparted; +-- check that wholerow vars in the RETUNING list work with partitioned tables +create table returningwrtest (a int) partition by list (a); +create table returningwrtest1 partition of returningwrtest for values in (1); +insert into returningwrtest values (1) returning returningwrtest; + returningwrtest +----------------- + (1) +(1 row) + +drop table returningwrtest; diff --git a/src/test/regress/expected/updatable_views.out b/src/test/regress/expected/updatable_views.out index eab5c0334c..9804f9d7f4 100644 --- a/src/test/regress/expected/updatable_views.out +++ b/src/test/regress/expected/updatable_views.out @@ -2428,3 +2428,13 @@ ERROR: new row violates check option for view "ptv_wco" DETAIL: Failing row contains (1, 2, null). drop view ptv, ptv_wco; drop table pt, pt1, pt11; +-- check that wholerow vars appearing in WITH CHECK OPTION constraint expressions +-- work fine with partitioned tables +create table returningwrtest (a int) partition by list (a); +create table returningwrtest1 partition of returningwrtest for values in (1); +create view returningwrtest_v as select * from returningwrtest where returningwrtest = '(2)'::returningwrtest with check option; +insert into returningwrtest_v values (1); +ERROR: new row violates check option for view "returningwrtest_v" +DETAIL: Failing row contains (1). +drop view returningwrtest_v; +drop table returningwrtest; diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql index 6adf25da40..2eff832e59 100644 --- a/src/test/regress/sql/insert.sql +++ b/src/test/regress/sql/insert.sql @@ -399,3 +399,9 @@ insert into mcrparted values ('aaa', 0), ('b', 0), ('bz', 10), ('c', -10), ('commons', 0), ('d', -10), ('e', 0); select tableoid::regclass, * from mcrparted order by a, b; drop table mcrparted; + +-- check that wholerow vars in the RETUNING list work with partitioned tables +create table returningwrtest (a int) partition by list (a); +create table returningwrtest1 partition of returningwrtest for values in (1); +insert into returningwrtest values (1) returning returningwrtest; +drop table returningwrtest; diff --git a/src/test/regress/sql/updatable_views.sql b/src/test/regress/sql/updatable_views.sql index 2ede44c02b..ea88f51a32 100644 --- a/src/test/regress/sql/updatable_views.sql +++ b/src/test/regress/sql/updatable_views.sql @@ -1141,3 +1141,12 @@ create view ptv_wco as select * from pt where a = 0 with check option; insert into ptv_wco values (1, 2); drop view ptv, ptv_wco; drop table pt, pt1, pt11; + +-- check that wholerow vars appearing in WITH CHECK OPTION constraint expressions +-- work fine with partitioned tables +create table returningwrtest (a int) partition by list (a); +create table returningwrtest1 partition of returningwrtest for values in (1); +create view returningwrtest_v as select * from returningwrtest where returningwrtest = '(2)'::returningwrtest with check option; +insert into returningwrtest_v values (1); +drop view returningwrtest_v; +drop table returningwrtest; -- 2.11.0
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
