From 4ba765e88b2a3569690ded7277edb276c1200f8f Mon Sep 17 00:00:00 2001 From: amit Date: Thu, 15 Dec 2016 16:27:04 +0900 Subject: [PATCH 3/7] Fix a bug in how we generate partition constraints Firstly, since we always want to recurse when calling RelationGetPartitionQual(), ie, consider the parent's partition constraint (if any), get rid of the argument recurse; also in the module-local generate_partition_qual() that it calls. Move the code for doing parent attnos to child attnos mapping for Vars in partition constraint expressions to a separate function map_partition_varattnos() and call it from the appropriate places. Doing it in get_qual_from_partbound(), as is now, would produce wrong result in certain multi-level partitioning cases, because it only considers the current pair of parent-child relations. In certain multi-level partitioning cases, attnums for the same key attribute(s) might differ between different pairs of consecutive levels causing the same attribute to be numbered differently in different Vars of the same expression tree. Remember that we apply the whole partition constraint (list of constraints of partitions at various levels) to a single (leaf partition) relation. With this commit, in generate_partition_qual(), we first generate the the whole partition constraint (considering all levels of partitioning) and then do the mapping from the root parent attnums to leaf partition attnums. Reported by: n/a Patch by: Amit Langote Reports: n/a --- src/backend/catalog/partition.c | 103 ++++++++++++++++-------------- src/backend/commands/tablecmds.c | 9 ++- src/backend/executor/execMain.c | 4 +- src/backend/optimizer/util/plancat.c | 2 +- src/include/catalog/partition.h | 3 +- src/test/regress/expected/alter_table.out | 30 +++++++++ src/test/regress/sql/alter_table.sql | 25 ++++++++ 7 files changed, 122 insertions(+), 54 deletions(-) diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c index fca874752f..34ab812b44 100644 --- a/src/backend/catalog/partition.c +++ b/src/backend/catalog/partition.c @@ -122,7 +122,7 @@ static List *get_qual_for_list(PartitionKey key, PartitionBoundSpec *spec); static List *get_qual_for_range(PartitionKey key, PartitionBoundSpec *spec); static Oid get_partition_operator(PartitionKey key, int col, StrategyNumber strategy, bool *need_relabel); -static List *generate_partition_qual(Relation rel, bool recurse); +static List *generate_partition_qual(Relation rel); static PartitionRangeBound *make_one_range_bound(PartitionKey key, int index, List *datums, bool lower); @@ -850,10 +850,6 @@ get_qual_from_partbound(Relation rel, Relation parent, Node *bound) PartitionBoundSpec *spec = (PartitionBoundSpec *) bound; PartitionKey key = RelationGetPartitionKey(parent); List *my_qual = NIL; - TupleDesc parent_tupdesc = RelationGetDescr(parent); - AttrNumber parent_attno; - AttrNumber *partition_attnos; - bool found_whole_row; Assert(key != NULL); @@ -874,38 +870,51 @@ get_qual_from_partbound(Relation rel, Relation parent, Node *bound) (int) key->strategy); } - /* - * Translate vars in the generated expression to have correct attnos. Note - * that the vars in my_qual bear attnos dictated by key which carries - * physical attnos of the parent. We must allow for a case where physical - * attnos of a partition can be different from the parent. - */ - partition_attnos = (AttrNumber *) - palloc0(parent_tupdesc->natts * sizeof(AttrNumber)); - for (parent_attno = 1; parent_attno <= parent_tupdesc->natts; - parent_attno++) + return my_qual; +} + +/* + * map_partition_varattnos - maps varattno of any Vars in expr from the + * parent attno to partition attno. + * + * We must allow for a case where physical attnos of a partition can be + * different from the parent's. + */ +List * +map_partition_varattnos(List *expr, Relation partrel, Relation parent) +{ + TupleDesc tupdesc = RelationGetDescr(parent); + AttrNumber attno; + AttrNumber *part_attnos; + bool found_whole_row; + + if (expr == NIL) + return NIL; + + part_attnos = (AttrNumber *) palloc0(tupdesc->natts * sizeof(AttrNumber)); + for (attno = 1; attno <= tupdesc->natts; attno++) { - Form_pg_attribute attribute = parent_tupdesc->attrs[parent_attno - 1]; + Form_pg_attribute attribute = tupdesc->attrs[attno - 1]; char *attname = NameStr(attribute->attname); - AttrNumber partition_attno; + AttrNumber part_attno; if (attribute->attisdropped) continue; - partition_attno = get_attnum(RelationGetRelid(rel), attname); - partition_attnos[parent_attno - 1] = partition_attno; + part_attno = get_attnum(RelationGetRelid(partrel), attname); + part_attnos[attno - 1] = part_attno; } - my_qual = (List *) map_variable_attnos((Node *) my_qual, - 1, 0, - partition_attnos, - parent_tupdesc->natts, - &found_whole_row); - /* there can never be a whole-row reference here */ + expr = (List *) map_variable_attnos((Node *) expr, + 1, 0, + part_attnos, + tupdesc->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"); - return my_qual; + return expr; } /* @@ -914,13 +923,13 @@ get_qual_from_partbound(Relation rel, Relation parent, Node *bound) * Returns a list of partition quals */ List * -RelationGetPartitionQual(Relation rel, bool recurse) +RelationGetPartitionQual(Relation rel) { /* Quick exit */ if (!rel->rd_rel->relispartition) return NIL; - return generate_partition_qual(rel, recurse); + return generate_partition_qual(rel); } /* @@ -1480,7 +1489,7 @@ get_partition_operator(PartitionKey key, int col, StrategyNumber strategy, * into cache memory. */ static List * -generate_partition_qual(Relation rel, bool recurse) +generate_partition_qual(Relation rel) { HeapTuple tuple; MemoryContext oldcxt; @@ -1494,6 +1503,10 @@ generate_partition_qual(Relation rel, bool recurse) /* Guard against stack overflow due to overly deep partition tree */ check_stack_depth(); + /* Recursive callers may not have checked themselves */ + if (!rel->rd_rel->relispartition) + return NIL; + /* Grab at least an AccessShareLock on the parent table */ parent = heap_open(get_partition_parent(RelationGetRelid(rel)), AccessShareLock); @@ -1501,20 +1514,18 @@ generate_partition_qual(Relation rel, bool recurse) /* Quick copy */ if (rel->rd_partcheck) { - if (parent->rd_rel->relispartition && recurse) - result = list_concat(generate_partition_qual(parent, true), - copyObject(rel->rd_partcheck)); - else - result = copyObject(rel->rd_partcheck); + result = list_concat(generate_partition_qual(parent), + copyObject(rel->rd_partcheck)); - heap_close(parent, AccessShareLock); + /* Mark Vars with correct attnos */ + result = map_partition_varattnos(result, rel, parent); + + /* Keep the parent locked until commit */ + heap_close(parent, NoLock); return result; } /* Get pg_class.relpartbound */ - if (!rel->rd_rel->relispartition) /* should not happen */ - elog(ERROR, "relation \"%s\" has relispartition = false", - RelationGetRelationName(rel)); tuple = SearchSysCache1(RELOID, RelationGetRelid(rel)); boundDatum = SysCacheGetAttr(RELOID, tuple, Anum_pg_class_relpartbound, @@ -1527,18 +1538,16 @@ generate_partition_qual(Relation rel, bool recurse) my_qual = get_qual_from_partbound(rel, parent, bound); - /* If requested, add parent's quals to the list (if any) */ - if (parent->rd_rel->relispartition && recurse) - { - List *parent_check; - - parent_check = generate_partition_qual(parent, true); - result = list_concat(parent_check, my_qual); - } + /* Add the parent's quals to the list (if any) */ + if (parent->rd_rel->relispartition) + result = list_concat(generate_partition_qual(parent), my_qual); else result = my_qual; - /* Save a copy of my_qual in the relcache */ + /* Mark Vars with correct attnos */ + result = map_partition_varattnos(result, rel, parent); + + /* Save a copy of *only* this rel's partition qual in the relcache */ oldcxt = MemoryContextSwitchTo(CacheMemoryContext); rel->rd_partcheck = copyObject(my_qual); MemoryContextSwitchTo(oldcxt); diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index c03edea18d..3c08551d38 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -13151,7 +13151,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd) */ partConstraint = list_concat(get_qual_from_partbound(attachRel, rel, cmd->bound), - RelationGetPartitionQual(rel, true)); + RelationGetPartitionQual(rel)); partConstraint = (List *) eval_const_expressions(NULL, (Node *) partConstraint); partConstraint = (List *) canonicalize_qual((Expr *) partConstraint); @@ -13325,6 +13325,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd) Oid part_relid = lfirst_oid(lc); Relation part_rel; Expr *constr; + List *my_constr; /* Lock already taken */ if (part_relid != RelationGetRelid(attachRel)) @@ -13347,8 +13348,10 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd) tab = ATGetQueueEntry(wqueue, part_rel); constr = linitial(partConstraint); - tab->partition_constraint = make_ands_implicit((Expr *) constr); - + my_constr = make_ands_implicit((Expr *) constr); + tab->partition_constraint = map_partition_varattnos(my_constr, + part_rel, + rel); /* keep our lock until commit */ if (part_rel != attachRel) heap_close(part_rel, NoLock); diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index 32c8f28beb..6a82c18571 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -1259,8 +1259,8 @@ InitResultRelInfo(ResultRelInfo *resultRelInfo, resultRelInfo->ri_projectReturning = NULL; if (load_partition_check) resultRelInfo->ri_PartitionCheck = - RelationGetPartitionQual(resultRelationDesc, - true); + RelationGetPartitionQual(resultRelationDesc); + /* * The following gets set to NULL unless we are initializing leaf * partitions for tuple-routing. diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c index 72272d9bb7..150229ed6d 100644 --- a/src/backend/optimizer/util/plancat.c +++ b/src/backend/optimizer/util/plancat.c @@ -1228,7 +1228,7 @@ get_relation_constraints(PlannerInfo *root, } /* Append partition predicates, if any */ - pcqual = RelationGetPartitionQual(relation, true); + pcqual = RelationGetPartitionQual(relation); if (pcqual) { /* diff --git a/src/include/catalog/partition.h b/src/include/catalog/partition.h index bf38df5d29..78220d6ac6 100644 --- a/src/include/catalog/partition.h +++ b/src/include/catalog/partition.h @@ -77,7 +77,8 @@ extern bool partition_bounds_equal(PartitionKey key, extern void check_new_partition_bound(char *relname, Relation parent, Node *bound); extern Oid get_partition_parent(Oid relid); extern List *get_qual_from_partbound(Relation rel, Relation parent, Node *bound); -extern List *RelationGetPartitionQual(Relation rel, bool recurse); +extern List *map_partition_varattnos(List *expr, Relation partrel, Relation parent); +extern List *RelationGetPartitionQual(Relation rel); /* For tuple routing */ extern PartitionDispatch *RelationGetPartitionDispatchInfo(Relation rel, diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index 62e18961d3..0a1d1db54e 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -3317,3 +3317,33 @@ drop cascades to table part_2 drop cascades to table part_5 drop cascades to table part_5_a drop cascades to table part_1 +-- more tests for certain multi-level partitioning scenarios +create table p (a int, b int) partition by range (a, b); +create table p1 (b int, a int not null) partition by range (b); +create table p11 (like p1); +alter table p11 drop a; +alter table p11 add a int; +alter table p11 drop a; +alter table p11 add a int not null; +-- attnum for key attribute 'a' is different in p, p1, and p11 +select attrelid::regclass, attname, attnum +from pg_attribute +where attname = 'a' + and (attrelid = 'p'::regclass + or attrelid = 'p1'::regclass + or attrelid = 'p11'::regclass); + attrelid | attname | attnum +----------+---------+-------- + p | a | 1 + p1 | a | 2 + p11 | a | 4 +(3 rows) + +alter table p1 attach partition p11 for values from (2) to (5); +insert into p1 (a, b) values (2, 3); +-- check that partition validation scan correctly detects violating rows +alter table p attach partition p1 for values from (1, 2) to (1, 10); +ERROR: partition constraint is violated by some row +-- cleanup +drop table p, p1 cascade; +NOTICE: drop cascades to table p11 diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index b285a406d9..ce7e85b6ad 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -2169,3 +2169,28 @@ ALTER TABLE list_parted2 ALTER COLUMN b TYPE text; -- cleanup DROP TABLE list_parted, list_parted2, range_parted CASCADE; + +-- more tests for certain multi-level partitioning scenarios +create table p (a int, b int) partition by range (a, b); +create table p1 (b int, a int not null) partition by range (b); +create table p11 (like p1); +alter table p11 drop a; +alter table p11 add a int; +alter table p11 drop a; +alter table p11 add a int not null; +-- attnum for key attribute 'a' is different in p, p1, and p11 +select attrelid::regclass, attname, attnum +from pg_attribute +where attname = 'a' + and (attrelid = 'p'::regclass + or attrelid = 'p1'::regclass + or attrelid = 'p11'::regclass); + +alter table p1 attach partition p11 for values from (2) to (5); + +insert into p1 (a, b) values (2, 3); +-- check that partition validation scan correctly detects violating rows +alter table p attach partition p1 for values from (1, 2) to (1, 10); + +-- cleanup +drop table p, p1 cascade; -- 2.11.0