On 2024-Apr-22, Alvaro Herrera wrote:
> > On d9f686a72~1 this script results in:
> > ERROR: cannot change NO INHERIT status of inherited NOT NULL constraint
> > "t_a_not_null" on relation "t"
>
> Right. Now I'm beginning to wonder if allowing ADD CONSTRAINT to mutate
> a pre-existing NO INHERIT constraint into a inheritable constraint
> (while accepting a constraint name in the command that we don't heed) is
> really what we want. Maybe we should throw some error when the affected
> constraint is the topmost one, and only accept the inheritance status
> change when we're recursing.
So I added a restriction that we only accept such a change when
recursively adding a constraint, or during binary upgrade. This should
limit the damage: you're no longer able to change an existing constraint
from NO INHERIT to YES INHERIT merely by doing another ALTER TABLE ADD
CONSTRAINT.
One thing that has me a little nervous about this whole business is
whether we're set up to error out where some child table down the
hierarchy has nulls, and we add a not-null constraint to it but fail to
do a verification scan. I tried a couple of cases and AFAICS it works
correctly, but maybe there are other cases I haven't thought about where
it doesn't.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"You're _really_ hosed if the person doing the hiring doesn't understand
relational systems: you end up with a whole raft of programmers, none of
whom has had a Date with the clue stick." (Andrew Sullivan)
https://postgr.es/m/20050809113420.gd2...@phlogiston.dyndns.org
>From 238bc09bed57dcd0e4887615f3c57a580eb26d9e Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Mon, 22 Apr 2024 11:32:04 +0200
Subject: [PATCH v2 1/2] Acquire locks on children before recursing
ALTER TABLE ADD CONSTRAINT was missing this, as evidenced by assertion
failures. While at it, create a small routine to encapsulate the weird
find_all_inheritors() call.
Reported-by: Alexander Lakhin <exclus...@gmail.com>
Discussion: https://postgr.es/m/5b4cd32f-1d5b-c080-c688-31736bbcd...@gmail.com
---
src/backend/commands/tablecmds.c | 40 +++++++++++++++++++++-----------
1 file changed, 27 insertions(+), 13 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3556240c8e..9058a0de7a 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -427,6 +427,7 @@ static void ATSimplePermissions(AlterTableType cmdtype, Relation rel, int allowe
static void ATSimpleRecursion(List **wqueue, Relation rel,
AlterTableCmd *cmd, bool recurse, LOCKMODE lockmode,
AlterTableUtilityContext *context);
+static void ATLockAllDescendants(Oid relid, LOCKMODE lockmode);
static void ATCheckPartitionsNotInUse(Relation rel, LOCKMODE lockmode);
static void ATTypedTableRecursion(List **wqueue, Relation rel, AlterTableCmd *cmd,
LOCKMODE lockmode,
@@ -1621,9 +1622,7 @@ RemoveRelations(DropStmt *drop)
* will lock those objects in the other order.
*/
if (state.actual_relkind == RELKIND_PARTITIONED_INDEX)
- (void) find_all_inheritors(state.heapOid,
- state.heap_lockmode,
- NULL);
+ ATLockAllDescendants(state.heapOid, state.heap_lockmode);
/* OK, we're ready to delete this one */
obj.classId = RelationRelationId;
@@ -4979,10 +4978,12 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
break;
case AT_AddConstraint: /* ADD CONSTRAINT */
ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
- /* Recursion occurs during execution phase */
- /* No command-specific prep needed except saving recurse flag */
if (recurse)
+ {
+ /* if recursing, set flag and lock all descendants */
cmd->recurse = true;
+ ATLockAllDescendants(RelationGetRelid(rel), lockmode);
+ }
pass = AT_PASS_ADD_CONSTR;
break;
case AT_AddIndexConstraint: /* ADD CONSTRAINT USING INDEX */
@@ -6721,6 +6722,21 @@ ATSimpleRecursion(List **wqueue, Relation rel,
}
}
+/*
+ * ATLockAllDescendants
+ *
+ * Acquire lock on all descendant relations of the given relation.
+ */
+static void
+ATLockAllDescendants(Oid relid, LOCKMODE lockmode)
+{
+ /*
+ * This is only used in DDL code, so it doesn't matter that we leak the
+ * list storage; it'll be gone soon enough.
+ */
+ (void) find_all_inheritors(relid, lockmode, NULL);
+}
+
/*
* Obtain list of partitions of the given table, locking them all at the given
* lockmode and ensuring that they all pass CheckTableNotInUse.
@@ -9370,10 +9386,9 @@ ATPrepAddPrimaryKey(List **wqueue, Relation rel, AlterTableCmd *cmd,
/*
* Acquire locks all the way down the hierarchy. The recursion to lower
- * levels occurs at execution time as necessary, so we don't need to do it
- * here, and we don't need the returned list either.
+ * levels occurs at execution time as necessary.
*/
- (void) find_all_inheritors(RelationGetRelid(rel), lockmode, NULL);
+ ATLockAllDescendants(RelationGetRelid(rel), lockmode);
/*
* Construct the list of constraints that we need to add to each child
@@ -9819,10 +9834,10 @@ ATAddCheckNNConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
/*
* Propagate to children as appropriate. Unlike most other ALTER
* routines, we have to do this one level of recursion at a time; we can't
- * use find_all_inheritors to do it in one pass.
+ * use find_all_inheritors to do it in one pass. We have all locks
+ * already, however.
*/
- children =
- find_inheritance_children(RelationGetRelid(rel), lockmode);
+ children = find_inheritance_children(RelationGetRelid(rel), NoLock);
/*
* Check if ONLY was specified with ALTER TABLE. If so, allow the
@@ -11258,8 +11273,7 @@ CloneFkReferencing(List **wqueue, Relation parentRel, Relation partRel)
*/
pkrel = table_open(constrForm->confrelid, ShareRowExclusiveLock);
if (pkrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- (void) find_all_inheritors(RelationGetRelid(pkrel),
- ShareRowExclusiveLock, NULL);
+ ATLockAllDescendants(RelationGetRelid(pkrel), ShareRowExclusiveLock);
DeconstructFkConstraintRow(tuple, &numfks, conkey, confkey,
conpfeqop, conppeqop, conffeqop,
--
2.39.2
>From 2773755db4d6df08c14c0854dcf7f3d811fd8ebb Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Wed, 24 Apr 2024 16:54:39 +0200
Subject: [PATCH v2 2/2] Disallow changing NO INHERIT property of a constraint
... unless it happens during binary upgrade, or to make an existing
constraint into an inherited one of a constraint in a parent relation.
---
src/backend/catalog/heap.c | 20 +++++++++++++++-----
src/backend/catalog/pg_constraint.c | 15 +++++++++++----
src/include/catalog/pg_constraint.h | 2 +-
src/test/regress/expected/inherit.out | 2 +-
4 files changed, 28 insertions(+), 11 deletions(-)
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index f0278b9c01..136cc42a92 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -2549,13 +2549,23 @@ AddRelationNewConstraints(Relation rel,
/*
* 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.
+ * we need only adjust its coninhcount and we're done. In certain
+ * cases (see below), if the constraint is there but marked NO
+ * INHERIT, then we mark it as no longer such and coninhcount
+ * updated, plus we must also recurse to the children (if any) to
+ * add the constraint there.
+ *
+ * We only allow the inheritability status to change during binary
+ * upgrade (where it's used to add the not-null constraints for
+ * children of tables with primary keys), or when we're recursing
+ * processing a table down an inheritance hierarchy; directly
+ * allowing a constraint to change from NO INHERIT to INHERIT
+ * during ALTER TABLE ADD CONSTRAINT would be far too surprising
+ * behavior.
*/
existing = AdjustNotNullInheritance1(RelationGetRelid(rel), colnum,
- cdef->inhcount, cdef->is_no_inherit);
+ cdef->inhcount, cdef->is_no_inherit,
+ IsBinaryUpgrade || allow_merge);
if (existing == 1)
continue; /* all done! */
else if (existing == -1)
diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c
index aaf3537d3f..6b8496e085 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -721,7 +721,7 @@ extractNotNullColumn(HeapTuple constrTup)
*/
int
AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count,
- bool is_no_inherit)
+ bool is_no_inherit, bool allow_noinherit_change)
{
HeapTuple tup;
@@ -744,16 +744,23 @@ AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count,
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\"",
+ errmsg("cannot change NO INHERIT status of NOT NULL constraint \"%s\" on relation \"%s\"",
NameStr(conform->conname), get_rel_name(relid)));
/*
* 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.
+ * NO INHERIT, we can just remove that flag (provided caller allows
+ * such a change), and instruct caller to recurse to add the
+ * constraint to children.
*/
if (!is_no_inherit && conform->connoinherit)
{
+ if (!allow_noinherit_change)
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("cannot change NO INHERIT status of NOT NULL constraint \"%s\" in relation \"%s\"",
+ NameStr(conform->conname), get_rel_name(relid)));
+
conform->connoinherit = false;
retval = -1; /* caller must add constraint on child rels */
}
diff --git a/src/include/catalog/pg_constraint.h b/src/include/catalog/pg_constraint.h
index 5c52d71e09..68bf55fdf7 100644
--- a/src/include/catalog/pg_constraint.h
+++ b/src/include/catalog/pg_constraint.h
@@ -262,7 +262,7 @@ extern HeapTuple findNotNullConstraint(Oid relid, const char *colname);
extern HeapTuple findDomainNotNullConstraint(Oid typid);
extern AttrNumber extractNotNullColumn(HeapTuple constrTup);
extern int AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count,
- bool is_no_inherit);
+ bool is_no_inherit, bool allow_noinherit_change);
extern void AdjustNotNullInheritance(Oid relid, Bitmapset *columns, int count);
extern List *RelationGetNotNullConstraints(Oid relid, bool cooked);
diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index 5a5c23fc3b..203ac6c52e 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -2126,7 +2126,7 @@ ERROR: cannot define not-null constraint on column "a2" with NO INHERIT
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 inherited NOT NULL constraint "nn" on relation "cc2"
+ERROR: cannot change NO INHERIT status of NOT NULL constraint "nn" on relation "cc2"
-- 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"
--
2.39.2
