From b40a418dfb3d7ce23ffa568c8a8d03640ce28435 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Tue, 16 Apr 2024 13:44:34 +0200 Subject: [PATCH] Fix add/drop of not-null constraints --- src/backend/catalog/heap.c | 36 +++++++++++++++++++++++++---- src/backend/catalog/pg_constraint.c | 36 ++++++++++++++++++++--------- src/backend/commands/tablecmds.c | 26 ++++++++++++++++++++- src/include/catalog/pg_constraint.h | 2 +- 4 files changed, 83 insertions(+), 17 deletions(-) diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index cc31909012..f0278b9c01 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -2539,6 +2539,7 @@ AddRelationNewConstraints(Relation rel, CookedConstraint *nncooked; AttrNumber colnum; char *nnname; + int existing; /* Determine which column to modify */ colnum = get_attnum(RelationGetRelid(rel), strVal(linitial(cdef->keys))); @@ -2547,12 +2548,39 @@ AddRelationNewConstraints(Relation rel, strVal(linitial(cdef->keys)), RelationGetRelid(rel)); /* - * If the column already has a not-null constraint, we need only - * update its catalog status and we're done. + * If the column already has an inheritable not-null constraint, + * we need only adjust its inheritance status and we're done. If + * the constraint is there but marked NO INHERIT, then it is + * updated in the same way, but we also recurse to the children + * (if any) to add the constraint there as well. */ - if (AdjustNotNullInheritance1(RelationGetRelid(rel), colnum, - cdef->inhcount, cdef->is_no_inherit)) + existing = AdjustNotNullInheritance1(RelationGetRelid(rel), colnum, + cdef->inhcount, cdef->is_no_inherit); + if (existing == 1) + continue; /* all done! */ + else if (existing == -1) + { + List *children; + + children = find_inheritance_children(RelationGetRelid(rel), NoLock); + foreach_oid(childoid, children) + { + Relation childrel = table_open(childoid, NoLock); + + AddRelationNewConstraints(childrel, + NIL, + list_make1(copyObject(cdef)), + allow_merge, + is_local, + is_internal, + queryString); + /* these constraints are not in the return list -- good? */ + + table_close(childrel, NoLock); + } + continue; + } /* * If a constraint name is specified, check that it isn't already diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c index 604280d322..a11483b1b8 100644 --- a/src/backend/catalog/pg_constraint.c +++ b/src/backend/catalog/pg_constraint.c @@ -709,36 +709,50 @@ extractNotNullColumn(HeapTuple constrTup) * AdjustNotNullInheritance1 * Adjust inheritance count for a single not-null constraint * - * Adjust inheritance count, and possibly islocal status, for the not-null - * constraint row of the given column, if it exists, and return true. - * If no not-null constraint is found for the column, return false. + * If no not-null constraint is found for the column, return 0 + * If the constraint does exist and it's inheritable, adjust its + * inheritance count (and possibly islocal status) and return 1. + * If the constraint exists but is marked NO INHERIT, adjust it as above + * and reset connoinherit to false, and return -1. Caller is + * responsible for adding the same constraint to the children, if any. */ -bool +int AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count, bool is_no_inherit) { HeapTuple tup; + Assert(count >= 0); + tup = findNotNullConstraintAttnum(relid, attnum); if (HeapTupleIsValid(tup)) { Relation pg_constraint; Form_pg_constraint conform; + int retval = 1; pg_constraint = table_open(ConstraintRelationId, RowExclusiveLock); conform = (Form_pg_constraint) GETSTRUCT(tup); /* - * Don't let the NO INHERIT status change (but don't complain - * unnecessarily.) In the future it might be useful to let an - * inheritable constraint replace a non-inheritable one, but we'd need - * to recurse to children to get it added there. + * If the constraint already exists in this relation but it's marked + * NO INHERIT, we can just remove that flag, and instruct caller to + * recurse to add the constraint to children. + * + * If we're asked for a NO INHERIT constraint and this relation + * already has an inheritable one, throw an error. XXX does this ever + * occur, and is this the right behavior? */ - if (is_no_inherit != conform->connoinherit) + if (is_no_inherit && !conform->connoinherit) ereport(ERROR, errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("cannot change NO INHERIT status of inherited NOT NULL constraint \"%s\" on relation \"%s\"", NameStr(conform->conname), get_rel_name(relid))); + else if (!is_no_inherit && conform->connoinherit) + { + conform->connoinherit = false; + retval = -1; /* caller must add constraint on child rels */ + } if (count > 0) conform->coninhcount += count; @@ -761,10 +775,10 @@ AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count, table_close(pg_constraint, RowExclusiveLock); - return true; + return retval; } - return false; + return 0; } /* diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 027d68e5d2..470b9b79d9 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -12942,6 +12942,30 @@ dropconstraint_internal(Relation rel, HeapTuple constraintTup, DropBehavior beha con = (Form_pg_constraint) GETSTRUCT(constraintTup); constrName = NameStr(con->conname); + /* + * For not-null constraints only: if we're asked to drop it, and it's both + * a constraint defined locally and inherited, we can simply mark it as no + * longer having a local definition, and no further changes are required. + * + * XXX the reason we don't do this for CHECK constraints is that they have + * historically not behaved this way. + */ + if (con->contype == CONSTRAINT_NOTNULL && + con->conislocal && con->coninhcount > 0) + { + HeapTuple copytup; + + copytup = heap_copytuple(constraintTup); + con = (Form_pg_constraint) GETSTRUCT(copytup); + con->conislocal = false; + CatalogTupleUpdate(conrel, ©tup->t_self, copytup); + ObjectAddressSet(conobj, ConstraintRelationId, con->oid); + + CommandCounterIncrement(); + table_close(conrel, RowExclusiveLock); + return conobj; + } + /* Don't allow drop of inherited constraints */ if (con->coninhcount > 0 && !recursing) ereport(ERROR, @@ -20225,7 +20249,7 @@ ATExecDetachPartitionFinalize(Relation rel, RangeVar *name) * DetachAddConstraintIfNeeded * Subroutine for ATExecDetachPartition. Create a constraint that * takes the place of the partition constraint, but avoid creating - * a dupe if an constraint already exists which implies the needed + * a dupe if a constraint already exists which implies the needed * constraint. */ static void diff --git a/src/include/catalog/pg_constraint.h b/src/include/catalog/pg_constraint.h index 8517fdafd3..5c52d71e09 100644 --- a/src/include/catalog/pg_constraint.h +++ b/src/include/catalog/pg_constraint.h @@ -261,7 +261,7 @@ extern HeapTuple findNotNullConstraintAttnum(Oid relid, AttrNumber attnum); extern HeapTuple findNotNullConstraint(Oid relid, const char *colname); extern HeapTuple findDomainNotNullConstraint(Oid typid); extern AttrNumber extractNotNullColumn(HeapTuple constrTup); -extern bool AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count, +extern int AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count, bool is_no_inherit); extern void AdjustNotNullInheritance(Oid relid, Bitmapset *columns, int count); extern List *RelationGetNotNullConstraints(Oid relid, bool cooked); -- 2.39.2