On 2024-Apr-15, Alvaro Herrera wrote:
> - Fourth thought: we do as in the third thought, except we also allow
> DROP CONSTRAINT a constraint that's marked "local, inherited" to be
> simply an inherited constraint (remove its "local" marker).
Here is an initial implementation of what I was thinking. Can you
please give it a try and see if it fixes this problem? At least in my
run of your original test case, it seems to work as expected.
This is still missing some cleanup and additional tests, of course.
Speaking of which, I wonder if I should modify pg16's tests so that they
leave behind tables set up in this way, to immortalize pg_upgrade
testing.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
>From b40a418dfb3d7ce23ffa568c8a8d03640ce28435 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
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
