From 941ef679635e6848d72edde4721c9d0ac4e9ff45 Mon Sep 17 00:00:00 2001 From: amit Date: Wed, 14 Jun 2017 11:32:01 +0900 Subject: [PATCH] Cope with differing attnos in ATExecAttachPartition code If the table being attached has different attnos from the parent for the partitioning columns which are present in the partition constraint expressions, then predicate_implied_by() will prematurely return false due to the structural inequality of the corresponding Var expressions in the partition constraint and those in the table's check constraint expressions. Fix this by mapping the partition constraint's expressions to bear the partition's attnos. Further, if the validation scan needs to be performed after all and the table being attached is a partitioned table, we will need to map the constraint expression again to change the attnos to the individual leaf partition's attnos from those of the table being attached. Another minor fix: Avoid creating an AT work queue entry for the table being attached if it's partitioned. Current coding does not lead to that happening. --- src/backend/commands/tablecmds.c | 59 +++++++++++++++++++------------ src/test/regress/expected/alter_table.out | 45 +++++++++++++++++++++++ src/test/regress/sql/alter_table.sql | 38 ++++++++++++++++++++ 3 files changed, 120 insertions(+), 22 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index b4425bc6af..981b7ae902 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -13412,7 +13412,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd) { Relation attachRel, catalog; - List *childrels; + List *attachRel_children; TupleConstr *attachRel_constr; List *partConstraint, *existConstraint; @@ -13479,10 +13479,14 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd) /* * Prevent circularity by seeing if rel is a partition of attachRel. (In * particular, this disallows making a rel a partition of itself.) + * + * We request an exclusive lock on all the partitions, because we may + * decide later in this function to scan them to validate the new + * partition constraint. */ - childrels = find_all_inheritors(RelationGetRelid(attachRel), - AccessShareLock, NULL); - if (list_member_oid(childrels, RelationGetRelid(rel))) + attachRel_children = find_all_inheritors(RelationGetRelid(attachRel), + AccessExclusiveLock, NULL); + if (list_member_oid(attachRel_children, RelationGetRelid(rel))) ereport(ERROR, (errcode(ERRCODE_DUPLICATE_TABLE), errmsg("circular inheritance not allowed"), @@ -13580,6 +13584,13 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd) partConstraint = list_make1(make_ands_explicit(partConstraint)); /* + * Adjust the generated constraint to match this partition's attribute + * numbers. + */ + partConstraint = map_partition_varattnos(partConstraint, 1, attachRel, + rel); + + /* * Check if we can do away with having to scan the table being attached to * validate the partition constraint, by *proving* that the existing * constraints of the table *imply* the partition predicate. We include @@ -13674,35 +13685,36 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd) */ if (!skip_validate) { - List *all_parts; ListCell *lc; - /* Take an exclusive lock on the partitions to be checked */ - if (attachRel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) - all_parts = find_all_inheritors(RelationGetRelid(attachRel), - AccessExclusiveLock, NULL); - else - all_parts = list_make1_oid(RelationGetRelid(attachRel)); + /* + * We already collected the list of partitions, including the table + * named in the command itself, which should appear at the head of the + * list. + */ + Assert(list_length(attachRel_children) >= 1 && + linitial_oid(attachRel_children) == RelationGetRelid(attachRel)); - foreach(lc, all_parts) + foreach(lc, attachRel_children) { AlteredTableInfo *tab; Oid part_relid = lfirst_oid(lc); Relation part_rel; Expr *constr; + /* Skip the original table if it's partitioned. */ + if (part_relid == RelationGetRelid(attachRel) && + attachRel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) + continue; + /* Lock already taken */ - if (part_relid != RelationGetRelid(attachRel)) - part_rel = heap_open(part_relid, NoLock); - else - part_rel = attachRel; + part_rel = heap_open(part_relid, NoLock); /* * Skip if it's a partitioned table. Only RELKIND_RELATION * relations (ie, leaf partitions) need to be scanned. */ - if (part_rel != attachRel && - part_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) + if (part_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) { heap_close(part_rel, NoLock); continue; @@ -13711,14 +13723,17 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd) /* Grab a work queue entry */ tab = ATGetQueueEntry(wqueue, part_rel); - /* Adjust constraint to match this partition */ + /* + * Adjust the constraint that we constructed above for the table + * being attached so that it matches this partition's attribute + * numbers. + */ constr = linitial(partConstraint); tab->partition_constraint = (Expr *) map_partition_varattnos((List *) constr, 1, - part_rel, rel); + part_rel, attachRel); /* keep our lock until commit */ - if (part_rel != attachRel) - heap_close(part_rel, NoLock); + heap_close(part_rel, NoLock); } } diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index 13d6a4b747..3ec5080fd6 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -3347,6 +3347,51 @@ ALTER TABLE part_5 DROP CONSTRAINT check_a; ALTER TABLE part_5 ADD CONSTRAINT check_a CHECK (a IN (5)), ALTER a SET NOT NULL; ALTER TABLE list_parted2 ATTACH PARTITION part_5 FOR VALUES IN (5); INFO: partition constraint for table "part_5" is implied by existing constraints +-- Check the case where attnos of the partitioning columns in the table being +-- attached differs from the parent. It should not affect the constraint- +-- checking logic that allows to skip the scan. +CREATE TABLE part_6 ( + c int, + LIKE list_parted2, + CONSTRAINT check_a CHECK (a IS NOT NULL AND a = 6) +); +ALTER TABLE part_6 DROP c; +ALTER TABLE list_parted2 ATTACH PARTITION part_6 FOR VALUES IN (6); +INFO: partition constraint for table "part_6" is implied by existing constraints +-- Similar to above, but the table being attached is a partitioned table +-- whose partition has still different attnos for the root partitioning +-- columns. +CREATE TABLE part_7 ( + LIKE list_parted2, + CONSTRAINT check_a CHECK (a IS NOT NULL AND a = 7) +) PARTITION BY LIST (b); +CREATE TABLE part_7_a_null ( + c int, + d int, + e int, + LIKE list_parted2, -- 'a' will have attnum = 4 + CONSTRAINT check_b CHECK (b IS NULL OR b = 'a'), + CONSTRAINT check_a CHECK (a IS NOT NULL AND a = 7) +); +ALTER TABLE part_7_a_null DROP c, DROP d, DROP e; +ALTER TABLE part_7 ATTACH PARTITION part_7_a_null FOR VALUES IN ('a', null); +INFO: partition constraint for table "part_7_a_null" is implied by existing constraints +ALTER TABLE list_parted2 ATTACH PARTITION part_7 FOR VALUES IN (7); +INFO: partition constraint for table "part_7" is implied by existing constraints +-- Same example, but check this time that the constraint correctly detects +-- violating rows +ALTER TABLE list_parted2 DETACH PARTITION part_7; +ALTER TABLE part_7 DROP CONSTRAINT check_a; -- thusly, scan won't be skipped +INSERT INTO part_7 (a, b) VALUES (8, null), (9, 'a'); +SELECT tableoid::regclass, a, b FROM part_7 order by a; + tableoid | a | b +---------------+---+--- + part_7_a_null | 8 | + part_7_a_null | 9 | a +(2 rows) + +ALTER TABLE list_parted2 ATTACH PARTITION part_7 FOR VALUES IN (7); +ERROR: partition constraint is violated by some row -- check that the table being attached is not already a partition ALTER TABLE list_parted2 ATTACH PARTITION part_2 FOR VALUES IN (2); ERROR: "part_2" is already a partition diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index 5dd1402ea6..e0b7b37278 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -2178,6 +2178,44 @@ ALTER TABLE part_5 DROP CONSTRAINT check_a; ALTER TABLE part_5 ADD CONSTRAINT check_a CHECK (a IN (5)), ALTER a SET NOT NULL; ALTER TABLE list_parted2 ATTACH PARTITION part_5 FOR VALUES IN (5); +-- Check the case where attnos of the partitioning columns in the table being +-- attached differs from the parent. It should not affect the constraint- +-- checking logic that allows to skip the scan. +CREATE TABLE part_6 ( + c int, + LIKE list_parted2, + CONSTRAINT check_a CHECK (a IS NOT NULL AND a = 6) +); +ALTER TABLE part_6 DROP c; +ALTER TABLE list_parted2 ATTACH PARTITION part_6 FOR VALUES IN (6); + +-- Similar to above, but the table being attached is a partitioned table +-- whose partition has still different attnos for the root partitioning +-- columns. +CREATE TABLE part_7 ( + LIKE list_parted2, + CONSTRAINT check_a CHECK (a IS NOT NULL AND a = 7) +) PARTITION BY LIST (b); +CREATE TABLE part_7_a_null ( + c int, + d int, + e int, + LIKE list_parted2, -- 'a' will have attnum = 4 + CONSTRAINT check_b CHECK (b IS NULL OR b = 'a'), + CONSTRAINT check_a CHECK (a IS NOT NULL AND a = 7) +); +ALTER TABLE part_7_a_null DROP c, DROP d, DROP e; +ALTER TABLE part_7 ATTACH PARTITION part_7_a_null FOR VALUES IN ('a', null); +ALTER TABLE list_parted2 ATTACH PARTITION part_7 FOR VALUES IN (7); + +-- Same example, but check this time that the constraint correctly detects +-- violating rows +ALTER TABLE list_parted2 DETACH PARTITION part_7; +ALTER TABLE part_7 DROP CONSTRAINT check_a; -- thusly, scan won't be skipped +INSERT INTO part_7 (a, b) VALUES (8, null), (9, 'a'); +SELECT tableoid::regclass, a, b FROM part_7 order by a; +ALTER TABLE list_parted2 ATTACH PARTITION part_7 FOR VALUES IN (7); + -- check that the table being attached is not already a partition ALTER TABLE list_parted2 ATTACH PARTITION part_2 FOR VALUES IN (2); -- 2.11.0