From ffed8a642aac8d8cd8011c8fdcebcf174277fb00 Mon Sep 17 00:00:00 2001 From: Amit Langote Date: Thu, 14 Nov 2024 17:11:46 +0900 Subject: [PATCH v3] Allow inherited constraints to be marked NO INHERIT in leaf partitions For inheritance child tables, inherited constraints can't be marked NO INHERIT, as that would prevent their propagation to grandchildren. Since leaf partitions have no children, marking their inherited constraints as NO INHERIT is harmless. Note that this change applies only to CHECK constraints. Reported-by: Alvaro Herrera Author: Amit Langote Reviewed-by: Alvaro Herrera Discussion: https://postgr.es/m/202411051201.zody6mld7vkw@alvherre.pgsql --- doc/src/sgml/ref/alter_table.sgml | 3 +-- src/backend/catalog/heap.c | 18 ++++++++++--- src/backend/commands/tablecmds.c | 17 +++++++++--- src/test/regress/expected/alter_table.out | 16 ++++++++++++ src/test/regress/expected/create_table.out | 28 ++++++++++++++++++++ src/test/regress/sql/alter_table.sql | 17 ++++++++++++ src/test/regress/sql/create_table.sql | 30 ++++++++++++++++++++++ 7 files changed, 119 insertions(+), 10 deletions(-) diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index 6098ebed43..24c027600c 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -1020,8 +1020,7 @@ WITH ( MODULUS numeric_literal, REM target table. The table to be attached must have all the same columns as the target table and no more; moreover, the column types must also match. Also, it must have all the NOT NULL and - CHECK constraints of the target table, not marked - NO INHERIT. Currently + CHECK constraints of the target table. Currently FOREIGN KEY constraints are not considered. UNIQUE and PRIMARY KEY constraints from the parent table will be created in the partition, if they don't diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index 003af4bf21..5a4073d055 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -2707,8 +2707,15 @@ 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\"", @@ -2717,9 +2724,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. It's fine to allow doing so for + * leaf partitions, as described above. */ - 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 ccd9645e7d..2feae4a526 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -377,7 +377,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, @@ -16153,7 +16154,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. @@ -16358,7 +16359,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; @@ -16455,8 +16457,15 @@ MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel) /* * 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, but only for CHECK constraints. */ - if (child_con->connoinherit) + if (child_con->connoinherit && + (!child_is_partition || + RELKIND_HAS_PARTITIONS(child_rel->rd_rel->relkind) || + child_con->contype == CONSTRAINT_NOTNULL)) 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 2212c8dbb5..5f1cf3cc2b 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -4282,6 +4282,22 @@ 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 NOT NULL, 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; -- not ok +ERROR: column "a" in child table "child1_with_noinherit_check" must be marked NOT NULL +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); +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 +DROP TABLE parted_parent_with_check; +-- -- DETACH PARTITION -- -- check that the table is partitioned at all diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out index 76604705a9..2f01720dc6 100644 --- a/src/test/regress/expected/create_table.out +++ b/src/test/regress/expected/create_table.out @@ -257,6 +257,34 @@ CREATE TABLE partitioned ( CONSTRAINT check_a CHECK (a > 0) NO INHERIT ) PARTITION BY RANGE (a); ERROR: cannot add NO INHERIT constraint to partitioned table "partitioned" +-- also when creating the partitioned table as partition +CREATE TABLE partitioned_with_check ( + a int NOT NULL, + CONSTRAINT check_a CHECK (a > 0) +) PARTITION BY RANGE (a); +CREATE TABLE partitioned_with_check_part1 + PARTITION OF partitioned_with_check ( + CONSTRAINT check_a CHECK (a > 0) NO INHERIT +) FOR VALUES FROM (1) TO (2) +PARTITION BY LIST (a); -- not ok +ERROR: constraint "check_a" conflicts with inherited constraint on relation "partitioned_with_check_part1" +-- fine if the partition is a leaf partition though +CREATE TABLE partitioned_with_check_part1 + PARTITION OF partitioned_with_check ( + CONSTRAINT check_a CHECK (a > 0) NO INHERIT +) FOR VALUES FROM (1) TO (2); -- ok +NOTICE: merging constraint "check_a" with inherited definition +-- similar tests for plain inheritance +CREATE TABLE ct_parent_with_check ( + a int, + CONSTRAINT check_a CHECK (a > 0)); +CREATE TABLE ct_child1_with_noinherit_check ( + a int, + CONSTRAINT check_a CHECK (a > 0) NO INHERIT +) INHERITS (ct_parent_with_check); -- not ok +NOTICE: merging column "a" with inherited definition +ERROR: constraint "check_a" conflicts with inherited constraint on relation "ct_child1_with_noinherit_check" +DROP TABLE partitioned_with_check, ct_parent_with_check; -- some checks after successful creation of a partitioned table CREATE FUNCTION plusone(a int) RETURNS INT AS $$ SELECT a+1; $$ LANGUAGE SQL; CREATE TABLE partitioned ( diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index 637e3dac38..d593e210ad 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -2738,6 +2738,23 @@ 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 NOT NULL, 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; -- not ok +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); +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 +DROP TABLE parted_parent_with_check; + -- -- DETACH PARTITION -- diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql index 37a227148e..79dfdab9ab 100644 --- a/src/test/regress/sql/create_table.sql +++ b/src/test/regress/sql/create_table.sql @@ -194,6 +194,36 @@ CREATE TABLE partitioned ( CONSTRAINT check_a CHECK (a > 0) NO INHERIT ) PARTITION BY RANGE (a); +-- also when creating the partitioned table as partition +CREATE TABLE partitioned_with_check ( + a int NOT NULL, + CONSTRAINT check_a CHECK (a > 0) +) PARTITION BY RANGE (a); + +CREATE TABLE partitioned_with_check_part1 + PARTITION OF partitioned_with_check ( + CONSTRAINT check_a CHECK (a > 0) NO INHERIT +) FOR VALUES FROM (1) TO (2) +PARTITION BY LIST (a); -- not ok + +-- fine if the partition is a leaf partition though +CREATE TABLE partitioned_with_check_part1 + PARTITION OF partitioned_with_check ( + CONSTRAINT check_a CHECK (a > 0) NO INHERIT +) FOR VALUES FROM (1) TO (2); -- ok + +-- similar tests for plain inheritance +CREATE TABLE ct_parent_with_check ( + a int, + CONSTRAINT check_a CHECK (a > 0)); + +CREATE TABLE ct_child1_with_noinherit_check ( + a int, + CONSTRAINT check_a CHECK (a > 0) NO INHERIT +) INHERITS (ct_parent_with_check); -- not ok + +DROP TABLE partitioned_with_check, ct_parent_with_check; + -- some checks after successful creation of a partitioned table CREATE FUNCTION plusone(a int) RETURNS INT AS $$ SELECT a+1; $$ LANGUAGE SQL; -- 2.43.0