From af738a2f975e36a082218b31f9f73e84540921cf Mon Sep 17 00:00:00 2001 From: Amit Langote Date: Thu, 7 Nov 2024 18:48:37 +0900 Subject: [PATCH v1] Allow inherited NO INHERIT check constraints in leaf partitions Preventing inherited NO INHERIT constraints is crucial for regular inheritance child tables, as the NO INHERIT marking would block these constraints from propagating to grandchildren. Since leaf partitions cannot have children, allowing inherited NO INHERIT constraints on them is harmless. Reported-by: Alvaro Herrera Discussion: https://postgr.es/m/202411051201.zody6mld7vkw@alvherre.pgsql --- src/backend/catalog/heap.c | 17 +++++++++++---- src/backend/commands/tablecmds.c | 18 +++++++++++----- src/test/regress/expected/alter_table.out | 25 +++++++++++++++++++++++ src/test/regress/sql/alter_table.sql | 22 ++++++++++++++++++++ 4 files changed, 73 insertions(+), 9 deletions(-) diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index c54a543c53..efbe4b3f20 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -2576,8 +2576,14 @@ MergeWithExistingConstraint(Relation rel, const char *ccname, Node *expr, errmsg("constraint \"%s\" for relation \"%s\" already exists", ccname, RelationGetRelationName(rel)))); - /* If the child constraint is "no inherit" then cannot merge */ - if (con->connoinherit) + /* + * If the child constraint is "no inherit" then cannot merge because + * that would prevent lower-level children from inheriting it. That's + * not a concern if the child table is a leaf partition which cannot + * have child tables, so we allow the merge. + */ + if (con->connoinherit && !rel->rd_rel->relispartition && + !RELKIND_HAS_PARTITIONS(rel->rd_rel->relkind)) ereport(ERROR, (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), errmsg("constraint \"%s\" conflicts with non-inherited constraint on relation \"%s\"", @@ -2586,9 +2592,12 @@ MergeWithExistingConstraint(Relation rel, const char *ccname, Node *expr, /* * Must not change an existing inherited constraint to "no inherit" * status. That's because inherited constraints should be able to - * propagate to lower-level children. + * propagate to lower-level children. Again, it's fine to allow doing + * so for leaf partitions. */ - if (con->coninhcount > 0 && is_no_inherit) + if (con->coninhcount > 0 && is_no_inherit && + (!rel->rd_rel->relispartition || + RELKIND_HAS_PARTITIONS(rel->rd_rel->relkind))) ereport(ERROR, (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), errmsg("constraint \"%s\" conflicts with inherited constraint on relation \"%s\"", diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 4345b96de5..f4d581445b 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -376,7 +376,8 @@ static List *MergeCheckConstraint(List *constraints, const char *name, Node *exp static void MergeChildAttribute(List *inh_columns, int exist_attno, int newcol_attno, const ColumnDef *newdef); static ColumnDef *MergeInheritedAttribute(List *inh_columns, int exist_attno, const ColumnDef *newdef); static void MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel, bool ispartition); -static void MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel); +static void MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel, + bool child_is_partition); static void StoreCatalogInheritance(Oid relationId, List *supers, bool child_is_partition); static void StoreCatalogInheritance1(Oid relationId, Oid parentOid, @@ -15899,7 +15900,7 @@ CreateInheritance(Relation child_rel, Relation parent_rel, bool ispartition) MergeAttributesIntoExisting(child_rel, parent_rel, ispartition); /* Match up the constraints and bump coninhcount as needed */ - MergeConstraintsIntoExisting(child_rel, parent_rel); + MergeConstraintsIntoExisting(child_rel, parent_rel, ispartition); /* * OK, it looks valid. Make the catalog entries that show inheritance. @@ -16094,7 +16095,8 @@ MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel, bool ispart * XXX See MergeWithExistingConstraint too if you change this code. */ static void -MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel) +MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel, + bool child_is_partition) { Relation constraintrel; SysScanDesc parent_scan; @@ -16153,8 +16155,14 @@ MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel) errmsg("child table \"%s\" has different definition for check constraint \"%s\"", RelationGetRelationName(child_rel), NameStr(parent_con->conname)))); - /* If the child constraint is "no inherit" then cannot merge */ - if (child_con->connoinherit) + /* + * If the child constraint is "no inherit" then cannot merge + * because that would prevent lower-level children from inheriting + * it. That's not a concern if the child table is a leaf partition + * which cannot have child tables, so we allow the merge. + */ + if (child_con->connoinherit && !child_is_partition && + !RELKIND_HAS_PARTITIONS(child_rel->rd_rel->relkind)) ereport(ERROR, (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), errmsg("constraint \"%s\" conflicts with non-inherited constraint on child table \"%s\"", diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index 143ae7c09c..5f5fb17ae5 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -4275,6 +4275,31 @@ ALTER TABLE hash_parted ATTACH PARTITION fail_part FOR VALUES WITH (MODULUS 3, R ERROR: every hash partition modulus must be a factor of the next larger modulus DETAIL: The new modulus 3 is not a factor of 4, the modulus of existing partition "hpart_1". DROP TABLE fail_part; +-- Prevent marking a constraint as NO INHERIT on a child table when it +-- inherits a constraint with the same name from its parent, to ensure that +-- the grandchildren consistently inherit the constraint. +CREATE TABLE parent_with_check (a int, CONSTRAINT check_a CHECK (a > 0)); +CREATE TABLE child1_with_noinherit_check (a int, CONSTRAINT check_a CHECK (a > 0) NO INHERIT); +ALTER TABLE child1_with_noinherit_check INHERIT parent_with_check; +ERROR: constraint "check_a" conflicts with non-inherited constraint on child table "child1_with_noinherit_check" +CREATE TABLE child2_with_noinherit_check (a int, CONSTRAINT check_a CHECK (a > 0) NO INHERIT) INHERITS (parent_with_check); +NOTICE: merging column "a" with inherited definition +ERROR: constraint "check_a" conflicts with inherited constraint on relation "child2_with_noinherit_check" +DROP TABLE parent_with_check, child1_with_noinherit_check; +-- No need to have that restriction for leaf partitions though, because they +-- cannot have any children of their own +CREATE TABLE parted_parent_with_check (a int, CONSTRAINT check_a CHECK (a > 0)) PARTITION BY LIST (a); +-- Case 1: ALTER TABLE parent ATTACH PARTITION leaf_partition ... +CREATE TABLE part1_with_noinherit_check (a int, CONSTRAINT check_a CHECK (a > 0) NO INHERIT); +ALTER TABLE parted_parent_with_check ATTACH PARTITION part1_with_noinherit_check FOR VALUES IN (1); -- ok +-- Case 2: CREATE TABLE leaf_partition PARTITION OF parent ... +CREATE TABLE part2_with_noinherit_check PARTITION OF parted_parent_with_check (CONSTRAINT check_a CHECK (a > 0) NO INHERIT) FOR VALUES IN (2); -- ok +NOTICE: merging constraint "check_a" with inherited definition +-- Case 3: CREATE TABLE non_leaf_partition PARTITION OF parent ... PARTITION BY +CREATE TABLE part3_with_noinherit_check PARTITION OF parted_parent_with_check (CONSTRAINT check_a CHECK (a > 0) NO INHERIT) FOR VALUES IN (3) + PARTITION BY LIST (a); -- not ok +ERROR: constraint "check_a" conflicts with inherited constraint on relation "part3_with_noinherit_check" +DROP TABLE parted_parent_with_check; -- -- DETACH PARTITION -- diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index c5dd43a15c..12f807e466 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -2733,6 +2733,28 @@ ALTER TABLE hash_parted ATTACH PARTITION fail_part FOR VALUES WITH (MODULUS 8, R ALTER TABLE hash_parted ATTACH PARTITION fail_part FOR VALUES WITH (MODULUS 3, REMAINDER 2); DROP TABLE fail_part; +-- Prevent marking a constraint as NO INHERIT on a child table when it +-- inherits a constraint with the same name from its parent, to ensure that +-- the grandchildren consistently inherit the constraint. +CREATE TABLE parent_with_check (a int, CONSTRAINT check_a CHECK (a > 0)); +CREATE TABLE child1_with_noinherit_check (a int, CONSTRAINT check_a CHECK (a > 0) NO INHERIT); +ALTER TABLE child1_with_noinherit_check INHERIT parent_with_check; +CREATE TABLE child2_with_noinherit_check (a int, CONSTRAINT check_a CHECK (a > 0) NO INHERIT) INHERITS (parent_with_check); +DROP TABLE parent_with_check, child1_with_noinherit_check; + +-- No need to have that restriction for leaf partitions though, because they +-- cannot have any children of their own +CREATE TABLE parted_parent_with_check (a int, CONSTRAINT check_a CHECK (a > 0)) PARTITION BY LIST (a); +-- Case 1: ALTER TABLE parent ATTACH PARTITION leaf_partition ... +CREATE TABLE part1_with_noinherit_check (a int, CONSTRAINT check_a CHECK (a > 0) NO INHERIT); +ALTER TABLE parted_parent_with_check ATTACH PARTITION part1_with_noinherit_check FOR VALUES IN (1); -- ok +-- Case 2: CREATE TABLE leaf_partition PARTITION OF parent ... +CREATE TABLE part2_with_noinherit_check PARTITION OF parted_parent_with_check (CONSTRAINT check_a CHECK (a > 0) NO INHERIT) FOR VALUES IN (2); -- ok +-- Case 3: CREATE TABLE non_leaf_partition PARTITION OF parent ... PARTITION BY +CREATE TABLE part3_with_noinherit_check PARTITION OF parted_parent_with_check (CONSTRAINT check_a CHECK (a > 0) NO INHERIT) FOR VALUES IN (3) + PARTITION BY LIST (a); -- not ok +DROP TABLE parted_parent_with_check; + -- -- DETACH PARTITION -- -- 2.43.0