On 2024-Apr-16, Justin Pryzby wrote: > Yes, this fixes the issue I reported.
Excellent, thanks for confirming. > BTW, that seems to be the same issue Andrew reported in January. > https://www.postgresql.org/message-id/CAJnzarwkfRu76_yi3dqVF_WL-MpvT54zMwAxFwJceXdHB76bOA%40mail.gmail.com That's really good news -- I was worried it would require much more invasive changes. I tested his case and noticed two additional issues, first that we fail to acquire locks down the hierarchy, so recursing down like ATPrepAddPrimaryKey does fails to pin down the children properly; and second, that the constraint left behind by restoring the dump preserves the "throaway" name. I made pg_dump use a different name when the table has a parent, just in case we end up not dropping the constraint. I'm going to push this early tomorrow. CI run: https://cirrus-ci.com/build/5754149453692928 -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
>From e1b48c62ab46f6aaed366daacdd0560374b1cf07 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera <alvhe...@alvh.no-ip.org> Date: Wed, 17 Apr 2024 19:23:04 +0200 Subject: [PATCH v2] Fix restore of not-null constraints with inheritance In tables with primary keys, pg_dump creates tables with primary keys by initially dumping them with throw-away not-null constraints (marked "no inherit" so that they don't create problems elsewhere), to later drop them once the primary key is restored. Because of a unrelated consideration, on tables with children we add not-null constraints to all columns of the primary key when it is created. If both a table and its child have primary keys, and pg_dump happens to emit the child table first (and its throw-away not-null) and later its parent table, the creation of the parent's PK will fail because the throw-away not-null constraint collides with the permanent not-null constraint that the PK wants to add, so the dump fails to restore. We can work around this problem by letting the primary key "take over" the child's not-null. This requires no changes to pg_dump, just two changes to ALTER TABLE: first, the ability to convert a no-inherit not-null constraint into a regular inheritable one (including recursing down to children, if there are any); second, the ability to "drop" a constraint that is defined both directly in the table and inherited from a parent (which simply means to mark it as no longer having a local definition). Secondarily, change ATPrepAddPrimaryKey() to acquire locks all the way down the inheritance hierarchy, in case we need to recurse when propagating constraints. In addition, change pg_dump to avoid giving a constraint a throwaway name for tables that are inheritance children. This is out of fear that the column might require the above shenanigans and the constraint ends up persisting later. We don't want such constraints being created with unsightly names. These two changes allow pg_dump to reproduce more cases involving inheritance from versions 16 and older. Reported-by: Andrew Bille <andrewbi...@gmail.com> Reported-by: Justin Pryzby <pry...@telsasoft.com> Discussion: https://postgr.es/m/cajnzarwkfru76_yi3dqvf_wl-mpvt54zmwaxfwjcexdhb76...@mail.gmail.com Discussion: https://postgr.es/m/Zh0aAH7tbZb-9HbC@pryzbyj2023 --- src/backend/catalog/heap.c | 36 +++++++++++-- src/backend/catalog/pg_constraint.c | 43 ++++++++++----- src/backend/commands/tablecmds.c | 64 ++++++++++++++++++++--- src/bin/pg_dump/pg_dump.c | 17 +++++- src/include/catalog/pg_constraint.h | 2 +- src/test/regress/expected/constraints.out | 56 ++++++++++++++++++++ src/test/regress/sql/constraints.sql | 22 ++++++++ 7 files changed, 213 insertions(+), 27 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..778b7c381d 100644 --- a/src/backend/catalog/pg_constraint.c +++ b/src/backend/catalog/pg_constraint.c @@ -565,8 +565,8 @@ ChooseConstraintName(const char *name1, const char *name2, } /* - * Find and return the pg_constraint tuple that implements a validated - * not-null constraint for the given column of the given relation. + * Find and return a copy of the pg_constraint tuple that implements a + * validated not-null constraint for the given column of the given relation. * * XXX This would be easier if we had pg_attribute.notnullconstr with the OID * of the constraint that implements the not-null constraint for that column. @@ -709,37 +709,54 @@ 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. + * Caller can create one. + * If the constraint does exist and it's inheritable, adjust its + * inheritance count (and possibly islocal status) and return 1. + * No further action needs to be taken. + * 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 we're asked for a NO INHERIT constraint and this relation + * already has an inheritable one, throw an error. */ - 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))); + /* + * 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 (!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 +778,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..e82db9afd5 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -9336,7 +9336,6 @@ ATPrepAddPrimaryKey(List **wqueue, Relation rel, AlterTableCmd *cmd, { List *children; List *newconstrs = NIL; - ListCell *lc; IndexStmt *indexstmt; /* No work if not creating a primary key */ @@ -9351,11 +9350,19 @@ ATPrepAddPrimaryKey(List **wqueue, Relation rel, AlterTableCmd *cmd, !rel->rd_rel->relhassubclass) return; - children = find_inheritance_children(RelationGetRelid(rel), lockmode); + /* + * 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. + */ + (void) find_all_inheritors(RelationGetRelid(rel), lockmode, NULL); - foreach(lc, indexstmt->indexParams) + /* + * Construct the list of constraints that we need to add to each child + * relation. + */ + foreach_node(IndexElem, elem, indexstmt->indexParams) { - IndexElem *elem = lfirst_node(IndexElem, lc); Constraint *nnconstr; Assert(elem->expr == NULL); @@ -9374,9 +9381,10 @@ ATPrepAddPrimaryKey(List **wqueue, Relation rel, AlterTableCmd *cmd, newconstrs = lappend(newconstrs, nnconstr); } - foreach(lc, children) + /* Finally, add AT subcommands to add each constraint to each child. */ + children = find_inheritance_children(RelationGetRelid(rel), NoLock); + foreach_oid(childrelid, children) { - Oid childrelid = lfirst_oid(lc); Relation childrel = table_open(childrelid, NoLock); AlterTableCmd *newcmd = makeNode(AlterTableCmd); ListCell *lc2; @@ -12942,6 +12950,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, @@ -16620,7 +16652,25 @@ MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel) errmsg("too many inheritance parents")); if (child_con->contype == CONSTRAINT_NOTNULL && child_con->connoinherit) + { + /* + * If the child has children, it's not possible to turn a NO + * INHERIT constraint into an inheritable one: we would need + * to recurse to create constraints in those children, but + * this is not a good place to do that. + */ + if (child_rel->rd_rel->relhassubclass) + ereport(ERROR, + errmsg("cannot add NOT NULL constraint to column \"%s\" of relation \"%s\" with inheritance children", + get_attname(RelationGetRelid(child_rel), + extractNotNullColumn(child_tuple), + false), + RelationGetRelationName(child_rel)), + errdetail("Existing constraint \"%s\" is marked NO INHERIT.", + NameStr(child_con->conname))); + child_con->connoinherit = false; + } /* * In case of partitions, an inherited constraint must be @@ -20225,7 +20275,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/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index c52e961b30..1a09c7f4bf 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -9096,8 +9096,21 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables) } else if (use_throwaway_notnull) { - tbinfo->notnull_constrs[j] = - psprintf("pgdump_throwaway_notnull_%d", notnullcount++); + /* + * When the table is an inheritance child, we don't dare use + * the throwaway name template, because the parent might have + * a primary key that requires this column to be not-null. In + * that case this constraint would survive. Because of this, + * just cons up a name. + */ + if (tbinfo->numParents > 0) + tbinfo->notnull_constrs[j] = + psprintf("%s_%s_not_null", + tbinfo->dobj.name, + tbinfo->attnames[j]); + else + tbinfo->notnull_constrs[j] = + psprintf("pgdump_throwaway_notnull_%d", notnullcount++); tbinfo->notnull_throwaway[j] = true; tbinfo->notnull_inh[j] = false; } 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); diff --git a/src/test/regress/expected/constraints.out b/src/test/regress/expected/constraints.out index 51157181c6..d50dd1f61a 100644 --- a/src/test/regress/expected/constraints.out +++ b/src/test/regress/expected/constraints.out @@ -321,6 +321,62 @@ ALTER TABLE ATACC1 ADD NOT NULL a NO INHERIT; Inherits: atacc1 DROP TABLE ATACC1, ATACC2; +-- overridding a no-inherit constraint with an inheritable one +CREATE TABLE ATACC2 (a int, CONSTRAINT a_is_not_null NOT NULL a NO INHERIT); +CREATE TABLE ATACC1 (a int); +CREATE TABLE ATACC3 (a int) INHERITS (ATACC2); +NOTICE: merging column "a" with inherited definition +INSERT INTO ATACC3 VALUES (null); -- make sure we scan atacc3 +ALTER TABLE ATACC2 INHERIT ATACC1; +ALTER TABLE ATACC1 ADD CONSTRAINT ditto NOT NULL a; +ERROR: column "a" of relation "atacc3" contains null values +DELETE FROM ATACC3; +ALTER TABLE ATACC1 ADD CONSTRAINT ditto NOT NULL a; +\d+ ATACC[123] + Table "public.atacc1" + Column | Type | Collation | Nullable | Default | Storage | Stats target | Description +--------+---------+-----------+----------+---------+---------+--------------+------------- + a | integer | | not null | | plain | | +Not-null constraints: + "ditto" NOT NULL "a" +Child tables: atacc2 + + Table "public.atacc2" + Column | Type | Collation | Nullable | Default | Storage | Stats target | Description +--------+---------+-----------+----------+---------+---------+--------------+------------- + a | integer | | not null | | plain | | +Not-null constraints: + "a_is_not_null" NOT NULL "a" (local, inherited) +Inherits: atacc1 +Child tables: atacc3 + + Table "public.atacc3" + Column | Type | Collation | Nullable | Default | Storage | Stats target | Description +--------+---------+-----------+----------+---------+---------+--------------+------------- + a | integer | | not null | | plain | | +Not-null constraints: + "ditto" NOT NULL "a" (inherited) +Inherits: atacc2 + +ALTER TABLE ATACC2 DROP CONSTRAINT a_is_not_null; +ALTER TABLE ATACC1 DROP CONSTRAINT ditto; +\d+ ATACC3 + Table "public.atacc3" + Column | Type | Collation | Nullable | Default | Storage | Stats target | Description +--------+---------+-----------+----------+---------+---------+--------------+------------- + a | integer | | | | plain | | +Inherits: atacc2 + +DROP TABLE ATACC1, ATACC2, ATACC3; +-- The same cannot be achieved this way +CREATE TABLE ATACC2 (a int, CONSTRAINT a_is_not_null NOT NULL a NO INHERIT); +CREATE TABLE ATACC1 (a int, CONSTRAINT ditto NOT NULL a); +CREATE TABLE ATACC3 (a int) INHERITS (ATACC2); +NOTICE: merging column "a" with inherited definition +ALTER TABLE ATACC2 INHERIT ATACC1; +ERROR: cannot add NOT NULL constraint to column "a" of relation "atacc2" with inheritance children +DETAIL: Existing constraint "a_is_not_null" is marked NO INHERIT. +DROP TABLE ATACC1, ATACC2, ATACC3; -- -- Check constraints on INSERT INTO -- diff --git a/src/test/regress/sql/constraints.sql b/src/test/regress/sql/constraints.sql index 2efb63e9d8..7a39b504a3 100644 --- a/src/test/regress/sql/constraints.sql +++ b/src/test/regress/sql/constraints.sql @@ -212,6 +212,28 @@ ALTER TABLE ATACC1 ADD NOT NULL a NO INHERIT; \d+ ATACC2 DROP TABLE ATACC1, ATACC2; +-- overridding a no-inherit constraint with an inheritable one +CREATE TABLE ATACC2 (a int, CONSTRAINT a_is_not_null NOT NULL a NO INHERIT); +CREATE TABLE ATACC1 (a int); +CREATE TABLE ATACC3 (a int) INHERITS (ATACC2); +INSERT INTO ATACC3 VALUES (null); -- make sure we scan atacc3 +ALTER TABLE ATACC2 INHERIT ATACC1; +ALTER TABLE ATACC1 ADD CONSTRAINT ditto NOT NULL a; +DELETE FROM ATACC3; +ALTER TABLE ATACC1 ADD CONSTRAINT ditto NOT NULL a; +\d+ ATACC[123] +ALTER TABLE ATACC2 DROP CONSTRAINT a_is_not_null; +ALTER TABLE ATACC1 DROP CONSTRAINT ditto; +\d+ ATACC3 +DROP TABLE ATACC1, ATACC2, ATACC3; + +-- The same cannot be achieved this way +CREATE TABLE ATACC2 (a int, CONSTRAINT a_is_not_null NOT NULL a NO INHERIT); +CREATE TABLE ATACC1 (a int, CONSTRAINT ditto NOT NULL a); +CREATE TABLE ATACC3 (a int) INHERITS (ATACC2); +ALTER TABLE ATACC2 INHERIT ATACC1; +DROP TABLE ATACC1, ATACC2, ATACC3; + -- -- Check constraints on INSERT INTO -- -- 2.39.2
