From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Alexander Lakhin <exclusion@gmail.com>
Cc: Dean Rasheed <dean.a.rasheed@gmail.com>
Cc: Andrew Bille <andrewbille@gmail.com>
Cc: Peter Eisentraut <peter@eisentraut.org>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: cataloguing NOT NULL constraints
Date: Wed, 24 Apr 2024 19:36:17 +0200
Message-ID: <202404241736.jf2zq2qmrgov@alvherre.pgsql> (raw)
In-Reply-To: <202404221022.nljgb5dwzppl@alvherre.pgsql>
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.GD2768@phlogiston.dyndns.orgAttachments:
[text/x-diff] v2-0001-Acquire-locks-on-children-before-recursing.patch (4.6K, ../202404241736.jf2zq2qmrgov@alvherre.pgsql/2-v2-0001-Acquire-locks-on-children-before-recursing.patch)
download | inline diff:
From 238bc09bed57dcd0e4887615f3c57a580eb26d9e Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@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 <exclusion@gmail.com>
Discussion: https://postgr.es/m/5b4cd32f-1d5b-c080-c688-31736bbcd739@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.cindex 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
[text/x-diff] v2-0002-Disallow-changing-NO-INHERIT-property-of-a-constr.patch (5.5K, ../202404241736.jf2zq2qmrgov@alvherre.pgsql/3-v2-0002-Disallow-changing-NO-INHERIT-property-of-a-constr.patch)
download | inline diff:
From 2773755db4d6df08c14c0854dcf7f3d811fd8ebb Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@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.cindex 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.cindex 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.hindex 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.outindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: alvherre@alvh.no-ip.org, exclusion@gmail.com, dean.a.rasheed@gmail.com, andrewbille@gmail.com, peter@eisentraut.org, robertmhaas@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: cataloguing NOT NULL constraints
In-Reply-To: <202404241736.jf2zq2qmrgov@alvherre.pgsql>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox