From d62fa6af0c800ac12ba395a183fbefbea1942184 Mon Sep 17 00:00:00 2001 From: Amit Langote Date: Wed, 13 Nov 2024 18:13:56 +0900 Subject: [PATCH v2 1/2] 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. This change applies to both CHECK constraints and NOT NULL constraints, the latter of which has been allowed to be marked NO INHERIT in leaf partitions since commit 14e87ffa5c54. A test added in commit 14e87ffa5c54 involving a leaf partition has been updated to reflect this change. 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 | 27 ++++++++--- src/backend/commands/tablecmds.c | 15 +++++-- src/test/regress/expected/alter_table.out | 29 ++++++++---- src/test/regress/expected/create_table.out | 51 +++++++++++++++++++++ src/test/regress/sql/alter_table.sql | 29 ++++++++---- src/test/regress/sql/create_table.sql | 52 ++++++++++++++++++++++ 7 files changed, 178 insertions(+), 28 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..e3aca9f418 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\"", @@ -2904,9 +2914,14 @@ AddRelationNotNullConstraints(Relation rel, List *constraints, { /* * If we get a constraint from the parent, having a local NO - * INHERIT one doesn't work. + * INHERIT one doesn't work, because the constraint inherited + * from the parent won't be propagated to grand children. + * That's not a concern if the child table is a leaf partition + * which cannot have child tables, so we allow it to have + * NO INHERIT constraints. */ - if (constr->is_no_inherit) + if (constr->is_no_inherit && + !rel->rd_rel->relispartition) ereport(ERROR, (errcode(ERRCODE_DATATYPE_MISMATCH), errmsg("cannot define not-null constraint on column \"%s\" with NO INHERIT", diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index ccd9645e7d..84b6617539 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,13 @@ 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. */ - if (child_con->connoinherit) + 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 2212c8dbb5..9cfa7257ee 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -4026,14 +4026,6 @@ SELECT conislocal, coninhcount FROM pg_constraint WHERE conrelid = 'part_1'::reg f | 1 (1 row) --- check that NOT NULL NO INHERIT cannot be merged to a normal NOT NULL -CREATE TABLE part_fail (a int NOT NULL NO INHERIT, - b char(2) COLLATE "C", - CONSTRAINT check_a CHECK (a > 0) -); -ALTER TABLE list_parted ATTACH PARTITION part_fail FOR VALUES IN (2); -ERROR: constraint "part_fail_a_not_null" conflicts with non-inherited constraint on child table "part_fail" -DROP TABLE part_fail; -- check that the new partition won't overlap with an existing partition CREATE TABLE fail_part (LIKE part_1 INCLUDING CONSTRAINTS); ALTER TABLE list_parted ATTACH PARTITION fail_part FOR VALUES IN (1); @@ -4282,6 +4274,27 @@ 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_notnull (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_notnull; -- not ok +ERROR: column "a" in child table "child1_with_noinherit_check" must be marked NOT NULL +CREATE TABLE child2_with_noinherit_notnull (a int NOT NULL NO INHERIT, CONSTRAINT check_a CHECK (a > 0)); +ALTER TABLE child2_with_noinherit_notnull INHERIT parent_with_check_notnull; -- not ok +ERROR: constraint "child2_with_noinherit_notnull_a_not_null" conflicts with non-inherited constraint on child table "child2_with_noinherit_notnull" +DROP TABLE parent_with_check_notnull, child1_with_noinherit_check, child2_with_noinherit_notnull; +-- 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_notnull (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_notnull ATTACH PARTITION part1_with_noinherit_check FOR VALUES IN (1); -- ok +CREATE TABLE part2_with_noinherit_notnull (a int NOT NULL NO INHERIT, CONSTRAINT check_a CHECK (a > 0)); +ALTER TABLE parted_parent_with_check_notnull ATTACH PARTITION part2_with_noinherit_notnull FOR VALUES IN (2); -- ok +DROP TABLE parted_parent_with_check_notnull; +-- -- 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..bfe2d5ab99 100644 --- a/src/test/regress/expected/create_table.out +++ b/src/test/regress/expected/create_table.out @@ -257,6 +257,57 @@ 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_not_null ( + a int NOT NULL +) PARTITION BY RANGE (a); +CREATE TABLE partitioned_with_not_null_part1 + PARTITION OF partitioned_with_not_null ( + a NOT NULL NO INHERIT +) FOR VALUES FROM (1) TO (2) +PARTITION BY LIST (a); -- not ok +ERROR: not-null constraints on partitioned tables cannot be NO INHERIT +-- fine if the partition is a leaf partition though +CREATE TABLE partitioned_with_not_null_part1 + PARTITION OF partitioned_with_not_null ( + a NOT NULL NO INHERIT +) FOR VALUES FROM (1) TO (2); -- ok +-- test cases with check constraint +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_notnull ( + a int NOT NULL, + 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_notnull); -- not ok +NOTICE: merging column "a" with inherited definition +ERROR: constraint "check_a" conflicts with inherited constraint on relation "ct_child1_with_noinherit_check" +CREATE TABLE ct_child2_with_noinherit_not_null ( + a int NOT NULL NO INHERIT, + CONSTRAINT check_a CHECK (a > 0) +) INHERITS (ct_parent_with_check_notnull); -- not ok +NOTICE: merging column "a" with inherited definition +NOTICE: merging constraint "check_a" with inherited definition +ERROR: cannot define not-null constraint on column "a" with NO INHERIT +DETAIL: The column has an inherited not-null constraint. +DROP TABLE partitioned_with_not_null, partitioned_with_check, ct_parent_with_check_notnull; -- 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..8a3f8b140b 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -2484,14 +2484,6 @@ ALTER TABLE list_parted ATTACH PARTITION part_1 FOR VALUES IN (1); SELECT attislocal, attinhcount FROM pg_attribute WHERE attrelid = 'part_1'::regclass AND attnum > 0; SELECT conislocal, coninhcount FROM pg_constraint WHERE conrelid = 'part_1'::regclass AND conname = 'check_a'; --- check that NOT NULL NO INHERIT cannot be merged to a normal NOT NULL -CREATE TABLE part_fail (a int NOT NULL NO INHERIT, - b char(2) COLLATE "C", - CONSTRAINT check_a CHECK (a > 0) -); -ALTER TABLE list_parted ATTACH PARTITION part_fail FOR VALUES IN (2); -DROP TABLE part_fail; - -- check that the new partition won't overlap with an existing partition CREATE TABLE fail_part (LIKE part_1 INCLUDING CONSTRAINTS); ALTER TABLE list_parted ATTACH PARTITION fail_part FOR VALUES IN (1); @@ -2738,6 +2730,27 @@ 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_notnull (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_notnull; -- not ok +CREATE TABLE child2_with_noinherit_notnull (a int NOT NULL NO INHERIT, CONSTRAINT check_a CHECK (a > 0)); +ALTER TABLE child2_with_noinherit_notnull INHERIT parent_with_check_notnull; -- not ok +DROP TABLE parent_with_check_notnull, child1_with_noinherit_check, child2_with_noinherit_notnull; + +-- 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_notnull (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_notnull ATTACH PARTITION part1_with_noinherit_check FOR VALUES IN (1); -- ok +CREATE TABLE part2_with_noinherit_notnull (a int NOT NULL NO INHERIT, CONSTRAINT check_a CHECK (a > 0)); +ALTER TABLE parted_parent_with_check_notnull ATTACH PARTITION part2_with_noinherit_notnull FOR VALUES IN (2); -- ok +DROP TABLE parted_parent_with_check_notnull; + -- -- DETACH PARTITION -- diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql index 37a227148e..ffc73e292d 100644 --- a/src/test/regress/sql/create_table.sql +++ b/src/test/regress/sql/create_table.sql @@ -194,6 +194,58 @@ 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_not_null ( + a int NOT NULL +) PARTITION BY RANGE (a); + +CREATE TABLE partitioned_with_not_null_part1 + PARTITION OF partitioned_with_not_null ( + a NOT NULL 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_not_null_part1 + PARTITION OF partitioned_with_not_null ( + a NOT NULL NO INHERIT +) FOR VALUES FROM (1) TO (2); -- ok + +-- test cases with check constraint +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_notnull ( + a int NOT NULL, + 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_notnull); -- not ok + +CREATE TABLE ct_child2_with_noinherit_not_null ( + a int NOT NULL NO INHERIT, + CONSTRAINT check_a CHECK (a > 0) +) INHERITS (ct_parent_with_check_notnull); -- not ok + +DROP TABLE partitioned_with_not_null, partitioned_with_check, ct_parent_with_check_notnull; + -- 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