diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c index 2f73085961b..70528679e57 100644 --- a/src/backend/catalog/pg_constraint.c +++ b/src/backend/catalog/pg_constraint.c @@ -756,7 +756,9 @@ AdjustNotNullInheritance(Oid relid, AttrNumber attnum, ereport(ERROR, errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("cannot change NO INHERIT status of NOT NULL constraint \"%s\" on relation \"%s\"", - NameStr(conform->conname), get_rel_name(relid))); + NameStr(conform->conname), get_rel_name(relid)), + errhint("You might need to make the existing constraint inheritable using %s.", + "ALTER TABLE ... ALTER CONSTRAINT ... INHERIT")); /* * Throw an error if the existing constraint is NOT VALID and caller @@ -767,7 +769,8 @@ AdjustNotNullInheritance(Oid relid, AttrNumber attnum, errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("incompatible NOT VALID constraint \"%s\" on relation \"%s\"", NameStr(conform->conname), get_rel_name(relid)), - errhint("You will need to use ALTER TABLE ... VALIDATE CONSTRAINT to validate it.")); + errhint("You might need to validate it using %s.", + "ALTER TABLE ... VALIDATE CONSTRAINT")); if (!is_local) { diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index b3ed69457fc..5fad1fa44c1 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -540,6 +540,7 @@ static ObjectAddress ATExecDropColumn(List **wqueue, Relation rel, const char *c static void ATPrepAddPrimaryKey(List **wqueue, Relation rel, AlterTableCmd *cmd, bool recurse, LOCKMODE lockmode, AlterTableUtilityContext *context); +static void verifyNotNullPKCompatible(HeapTuple tuple, const char *colname); static ObjectAddress ATExecAddIndex(AlteredTableInfo *tab, Relation rel, IndexStmt *stmt, bool is_rebuild, LOCKMODE lockmode); static ObjectAddress ATExecAddStatistics(AlteredTableInfo *tab, Relation rel, @@ -9438,8 +9439,26 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName, } /* - * Prepare to add a primary key on table, by adding not-null constraints + * Prepare to add a primary key on a table, by adding not-null constraints * on all columns. + * + * The not-null constraints for a primary key must cover the whole inheritance + * hierarchy (failing to ensure that leads to funny corner cases). For the + * normal case where we're asked to recurse, this routine ensures that the + * not-null constraints either exist already, or queues a requirement for them + * to be created by phase 2. + * + * For the case where we're asked not to recurse, we verify that a not-null + * constraint exists on each column of each (direct) child table, throwing an + * error if not. Not throwing an error would also work, because a not-null + * constraint would be created anyway, but it'd cause a silent scan of the + * child table to verify absence of nulls. We prefer to let the user know so + * that they can add the constraint manually without having to hold + * AccessExclusiveLock while at it. + * + * However, it's also important that we do not acquire locks on children if + * the not-null constraints already exist on the parent, to avoid risking + * deadlocks during parallel pg_restore of PKs on partitioned tables. */ static void ATPrepAddPrimaryKey(List **wqueue, Relation rel, AlterTableCmd *cmd, @@ -9447,42 +9466,13 @@ ATPrepAddPrimaryKey(List **wqueue, Relation rel, AlterTableCmd *cmd, AlterTableUtilityContext *context) { Constraint *pkconstr; + List *children; + bool got_children = false; pkconstr = castNode(Constraint, cmd->def); if (pkconstr->contype != CONSTR_PRIMARY) return; - /* - * If not recursing, we must ensure that all children have a NOT NULL - * constraint on the columns, and error out if not. - */ - if (!recurse) - { - List *children; - - children = find_inheritance_children(RelationGetRelid(rel), - lockmode); - foreach_oid(childrelid, children) - { - foreach_node(String, attname, pkconstr->keys) - { - HeapTuple tup; - Form_pg_attribute attrForm; - - tup = SearchSysCacheAttName(childrelid, strVal(attname)); - if (!tup) - elog(ERROR, "cache lookup failed for attribute %s of relation %u", - strVal(attname), childrelid); - attrForm = (Form_pg_attribute) GETSTRUCT(tup); - if (!attrForm->attnotnull) - ereport(ERROR, - errmsg("column \"%s\" of table \"%s\" is not marked NOT NULL", - strVal(attname), get_rel_name(childrelid))); - ReleaseSysCache(tup); - } - } - } - /* Verify that columns are not-null, or request that they be made so */ foreach_node(String, column, pkconstr->keys) { @@ -9498,42 +9488,46 @@ ATPrepAddPrimaryKey(List **wqueue, Relation rel, AlterTableCmd *cmd, tuple = findNotNullConstraint(RelationGetRelid(rel), strVal(column)); if (tuple != NULL) { - Form_pg_constraint conForm = (Form_pg_constraint) GETSTRUCT(tuple); - - /* a NO INHERIT constraint is no good */ - if (conForm->connoinherit) - ereport(ERROR, - errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), - errmsg("cannot create primary key on column \"%s\"", - strVal(column)), - /*- translator: third %s is a constraint characteristic such as NOT VALID */ - errdetail("The constraint \"%s\" on column \"%s\", marked %s, is incompatible with a primary key.", - NameStr(conForm->conname), strVal(column), "NO INHERIT"), - errhint("You will need to make it inheritable using %s.", - "ALTER TABLE ... ALTER CONSTRAINT ... INHERIT")); - - /* an unvalidated constraint is no good */ - if (!conForm->convalidated) - ereport(ERROR, - errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), - errmsg("cannot create primary key on column \"%s\"", - strVal(column)), - /*- translator: third %s is a constraint characteristic such as NOT VALID */ - errdetail("The constraint \"%s\" on column \"%s\", marked %s, is incompatible with a primary key.", - NameStr(conForm->conname), strVal(column), "NOT VALID"), - errhint("You will need to validate it using %s.", - "ALTER TABLE ... VALIDATE CONSTRAINT")); + verifyNotNullPKCompatible(tuple, strVal(column)); /* All good with this one; don't request another */ heap_freetuple(tuple); continue; } + else if (!recurse) + { + /* + * No constraint on this column. Asked not to recurse, we won't + * create one here, but verify that all children have one. + */ + if (!got_children) + { + children = find_inheritance_children(RelationGetRelid(rel), + lockmode); + /* only search for children on the first time through */ + got_children = true; + } + + foreach_oid(childrelid, children) + { + HeapTuple tup; + + tup = findNotNullConstraint(childrelid, strVal(column)); + if (!tup) + ereport(ERROR, + errmsg("column \"%s\" of table \"%s\" is not marked NOT NULL", + strVal(column), get_rel_name(childrelid))); + /* verify it's good enough */ + verifyNotNullPKCompatible(tup, strVal(column)); + } + } /* This column is not already not-null, so add it to the queue */ nnconstr = makeNotNullConstraint(column); newcmd = makeNode(AlterTableCmd); newcmd->subtype = AT_AddConstraint; + /* note we force recurse=true here; see above */ newcmd->recurse = true; newcmd->def = (Node *) nnconstr; @@ -9541,6 +9535,43 @@ ATPrepAddPrimaryKey(List **wqueue, Relation rel, AlterTableCmd *cmd, } } +/* + * Verify whether the given not-null constraint is compatible with a + * primary key. If not, an error is thrown. + */ +static void +verifyNotNullPKCompatible(HeapTuple tuple, const char *colname) +{ + Form_pg_constraint conForm = (Form_pg_constraint) GETSTRUCT(tuple); + + if (conForm->contype != CONSTRAINT_NOTNULL) + elog(ERROR, "constraint %u is not a not-null constraint", conForm->oid); + + /* a NO INHERIT constraint is no good */ + if (conForm->connoinherit) + ereport(ERROR, + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("cannot create primary key on column \"%s\"", colname), + /*- translator: fourth %s is a constraint characteristic such as NOT VALID */ + errdetail("The constraint \"%s\" on column \"%s\" of table \"%s\", marked %s, is incompatible with a primary key.", + NameStr(conForm->conname), colname, + get_rel_name(conForm->conrelid), "NO INHERIT"), + errhint("You might need to make the existing constraint inheritable using %s.", + "ALTER TABLE ... ALTER CONSTRAINT ... INHERIT")); + + /* an unvalidated constraint is no good */ + if (!conForm->convalidated) + ereport(ERROR, + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("cannot create primary key on column \"%s\"", colname), + /*- translator: fourth %s is a constraint characteristic such as NOT VALID */ + errdetail("The constraint \"%s\" on column \"%s\" of table \"%s\", marked %s, is incompatible with a primary key.", + NameStr(conForm->conname), colname, + get_rel_name(conForm->conrelid), "NOT VALID"), + errhint("You might need to validate it using %s.", + "ALTER TABLE ... VALIDATE CONSTRAINT")); +} + /* * ALTER TABLE ADD INDEX * diff --git a/src/test/regress/expected/constraints.out b/src/test/regress/expected/constraints.out index a8c6495ae01..92e441a16cd 100644 --- a/src/test/regress/expected/constraints.out +++ b/src/test/regress/expected/constraints.out @@ -1042,6 +1042,7 @@ ALTER TABLE ATACC2 INHERIT ATACC1; -- can't override ALTER TABLE ATACC1 ADD CONSTRAINT ditto NOT NULL a; ERROR: cannot change NO INHERIT status of NOT NULL constraint "a_is_not_null" on relation "atacc2" +HINT: You might need to make the existing constraint inheritable using ALTER TABLE ... ALTER CONSTRAINT ... INHERIT. -- dropping the NO INHERIT constraint allows this to work ALTER TABLE ATACC2 DROP CONSTRAINT a_is_not_null; ALTER TABLE ATACC1 ADD CONSTRAINT ditto NOT NULL a; @@ -1233,8 +1234,8 @@ Indexes: create table cnn_pk (a int not null no inherit); alter table cnn_pk add primary key (a); ERROR: cannot create primary key on column "a" -DETAIL: The constraint "cnn_pk_a_not_null" on column "a", marked NO INHERIT, is incompatible with a primary key. -HINT: You will need to make it inheritable using ALTER TABLE ... ALTER CONSTRAINT ... INHERIT. +DETAIL: The constraint "cnn_pk_a_not_null" on column "a" of table "cnn_pk", marked NO INHERIT, is incompatible with a primary key. +HINT: You might need to make the existing constraint inheritable using ALTER TABLE ... ALTER CONSTRAINT ... INHERIT. drop table cnn_pk; -- Ensure partitions are scanned for null values when adding a PK create table cnn2_parted(a int) partition by list (a); @@ -1389,14 +1390,15 @@ Not-null constraints: -- If we have an invalid constraint, we can't have another ALTER TABLE notnull_tbl1 ADD CONSTRAINT nn1 NOT NULL a NOT VALID NO INHERIT; ERROR: cannot change NO INHERIT status of NOT NULL constraint "nn" on relation "notnull_tbl1" +HINT: You might need to make the existing constraint inheritable using ALTER TABLE ... ALTER CONSTRAINT ... INHERIT. ALTER TABLE notnull_tbl1 ADD CONSTRAINT nn NOT NULL a; ERROR: incompatible NOT VALID constraint "nn" on relation "notnull_tbl1" -HINT: You will need to use ALTER TABLE ... VALIDATE CONSTRAINT to validate it. +HINT: You might need to validate it using ALTER TABLE ... VALIDATE CONSTRAINT. -- cannot add primary key on a column with an invalid not-null ALTER TABLE notnull_tbl1 ADD PRIMARY KEY (a); ERROR: cannot create primary key on column "a" -DETAIL: The constraint "nn" on column "a", marked NOT VALID, is incompatible with a primary key. -HINT: You will need to validate it using ALTER TABLE ... VALIDATE CONSTRAINT. +DETAIL: The constraint "nn" on column "a" of table "notnull_tbl1", marked NOT VALID, is incompatible with a primary key. +HINT: You might need to validate it using ALTER TABLE ... VALIDATE CONSTRAINT. -- ALTER column SET NOT NULL validates an invalid constraint (but this fails -- because of rows with null values) ALTER TABLE notnull_tbl1 ALTER a SET NOT NULL; @@ -1502,7 +1504,7 @@ NOTICE: merging multiple inherited definitions of column "i" ALTER TABLE notnull_inhparent ADD CONSTRAINT nn NOT NULL i NOT VALID; ALTER TABLE notnull_inhchild ADD CONSTRAINT nn1 NOT NULL i; -- error ERROR: incompatible NOT VALID constraint "nn" on relation "notnull_inhchild" -HINT: You will need to use ALTER TABLE ... VALIDATE CONSTRAINT to validate it. +HINT: You might need to validate it using ALTER TABLE ... VALIDATE CONSTRAINT. EXECUTE get_nnconstraint_info('{notnull_inhparent, notnull_inhchild, notnull_inhgrand}'); tabname | conname | convalidated | conislocal | coninhcount -------------------+---------+--------------+------------+------------- @@ -1567,6 +1569,24 @@ ERROR: constraint "nn1" conflicts with NOT VALID constraint on child table "pp_ ALTER TABLE pp_nn_1 VALIDATE CONSTRAINT nn1; ALTER TABLE pp_nn ATTACH PARTITION pp_nn_1 FOR VALUES IN (NULL,5); --ok DROP TABLE pp_nn; +-- Try a partition with an invalid constraint and create a PK on the parent. +CREATE TABLE pp_nn (a int) PARTITION BY HASH (a); +CREATE TABLE pp_nn_1 PARTITION OF pp_nn FOR VALUES WITH (MODULUS 2, REMAINDER 0); +ALTER TABLE pp_nn_1 ADD CONSTRAINT nn NOT NULL a NOT VALID; +ALTER TABLE ONLY pp_nn ADD PRIMARY KEY (a); +ERROR: cannot create primary key on column "a" +DETAIL: The constraint "nn" on column "a" of table "pp_nn_1", marked NOT VALID, is incompatible with a primary key. +HINT: You might need to validate it using ALTER TABLE ... VALIDATE CONSTRAINT. +DROP TABLE pp_nn; +-- same as above, but the constraint is NO INHERIT +CREATE TABLE pp_nn (a int) PARTITION BY HASH (a); +CREATE TABLE pp_nn_1 PARTITION OF pp_nn FOR VALUES WITH (MODULUS 2, REMAINDER 0); +ALTER TABLE pp_nn_1 ADD CONSTRAINT nn NOT NULL a NO INHERIT; +ALTER TABLE ONLY pp_nn ADD PRIMARY KEY (a); +ERROR: cannot create primary key on column "a" +DETAIL: The constraint "nn" on column "a" of table "pp_nn_1", marked NO INHERIT, is incompatible with a primary key. +HINT: You might need to make the existing constraint inheritable using ALTER TABLE ... ALTER CONSTRAINT ... INHERIT. +DROP TABLE pp_nn; -- Create table with NOT NULL INVALID constraint, for pg_upgrade. CREATE TABLE notnull_tbl1_upg (a int, b int); INSERT INTO notnull_tbl1_upg VALUES (NULL, 1), (NULL, 2), (300, 3); diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out index 2a8bfba768e..f9b0c415cfd 100644 --- a/src/test/regress/expected/inherit.out +++ b/src/test/regress/expected/inherit.out @@ -2286,6 +2286,7 @@ DETAIL: The column has an inherited not-null constraint. -- change NO INHERIT status of inherited constraint: no dice, it's inherited alter table cc2 add not null a2 no inherit; ERROR: cannot change NO INHERIT status of NOT NULL constraint "nn" on relation "cc2" +HINT: You might need to make the existing constraint inheritable using ALTER TABLE ... ALTER CONSTRAINT ... INHERIT. -- remove constraint from cc2: no dice, it's inherited alter table cc2 alter column a2 drop not null; ERROR: cannot drop inherited constraint "nn" of relation "cc2" @@ -2509,6 +2510,7 @@ CREATE TABLE inh_nn_parent (a int, NOT NULL a NO INHERIT); CREATE TABLE inh_nn_child() INHERITS (inh_nn_parent); ALTER TABLE inh_nn_parent ADD CONSTRAINT nna NOT NULL a; ERROR: cannot change NO INHERIT status of NOT NULL constraint "inh_nn_parent_a_not_null" on relation "inh_nn_parent" +HINT: You might need to make the existing constraint inheritable using ALTER TABLE ... ALTER CONSTRAINT ... INHERIT. ALTER TABLE inh_nn_parent ALTER a SET NOT NULL; ERROR: cannot change NO INHERIT status of NOT NULL constraint "inh_nn_parent_a_not_null" on relation "inh_nn_parent" DROP TABLE inh_nn_parent cascade; @@ -2520,6 +2522,7 @@ CREATE TABLE inh_nn_lvl2 () INHERITS (inh_nn_lvl1); CREATE TABLE inh_nn_lvl3 (CONSTRAINT foo NOT NULL a NO INHERIT) INHERITS (inh_nn_lvl2); ALTER TABLE inh_nn_lvl1 ADD PRIMARY KEY (a); ERROR: cannot change NO INHERIT status of NOT NULL constraint "foo" on relation "inh_nn_lvl3" +HINT: You might need to make the existing constraint inheritable using ALTER TABLE ... ALTER CONSTRAINT ... INHERIT. DROP TABLE inh_nn_lvl1, inh_nn_lvl2, inh_nn_lvl3; -- Disallow specifying conflicting NO INHERIT flags for the same constraint CREATE TABLE inh_nn1 (a int primary key, b int, not null a no inherit); diff --git a/src/test/regress/sql/constraints.sql b/src/test/regress/sql/constraints.sql index bf8f0aa181d..5d6d749c150 100644 --- a/src/test/regress/sql/constraints.sql +++ b/src/test/regress/sql/constraints.sql @@ -940,6 +940,20 @@ ALTER TABLE pp_nn_1 VALIDATE CONSTRAINT nn1; ALTER TABLE pp_nn ATTACH PARTITION pp_nn_1 FOR VALUES IN (NULL,5); --ok DROP TABLE pp_nn; +-- Try a partition with an invalid constraint and create a PK on the parent. +CREATE TABLE pp_nn (a int) PARTITION BY HASH (a); +CREATE TABLE pp_nn_1 PARTITION OF pp_nn FOR VALUES WITH (MODULUS 2, REMAINDER 0); +ALTER TABLE pp_nn_1 ADD CONSTRAINT nn NOT NULL a NOT VALID; +ALTER TABLE ONLY pp_nn ADD PRIMARY KEY (a); +DROP TABLE pp_nn; + +-- same as above, but the constraint is NO INHERIT +CREATE TABLE pp_nn (a int) PARTITION BY HASH (a); +CREATE TABLE pp_nn_1 PARTITION OF pp_nn FOR VALUES WITH (MODULUS 2, REMAINDER 0); +ALTER TABLE pp_nn_1 ADD CONSTRAINT nn NOT NULL a NO INHERIT; +ALTER TABLE ONLY pp_nn ADD PRIMARY KEY (a); +DROP TABLE pp_nn; + -- Create table with NOT NULL INVALID constraint, for pg_upgrade. CREATE TABLE notnull_tbl1_upg (a int, b int); INSERT INTO notnull_tbl1_upg VALUES (NULL, 1), (NULL, 2), (300, 3);