From d9f686a72ee91f6773e5d2bc52994db8d7157a8e Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Thu, 18 Apr 2024 15:35:15 +0200 Subject: [PATCH] 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. These two changes allow pg_dump to reproduce more cases involving inheritance from versions 16 and older. Lastly, make two changes to pg_dump: 1) do not try to drop a not-null constraint that's marked as inherited; this allows a dump to restore with no errors if a table with a PK inherits from another which also has a PK; 2) avoid giving inherited constraints throwaway names, for the rare cases where such a constraint survives after the restore. Reported-by: Andrew Bille Reported-by: Justin Pryzby Discussion: https://postgr.es/m/CAJnzarwkfRu76_yi3dqVF_WL-MpvT54zMwAxFwJceXdHB76bOA@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 | 65 ++++++++++++++++++++--- src/bin/pg_dump/pg_dump.c | 26 +++++++-- src/include/catalog/pg_constraint.h | 2 +- src/test/regress/expected/constraints.out | 56 +++++++++++++++++++ src/test/regress/sql/constraints.sql | 22 ++++++++ 7 files changed, 221 insertions(+), 29 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..f72b2dcadf 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,31 @@ dropconstraint_internal(Relation rel, HeapTuple constraintTup, DropBehavior beha con = (Form_pg_constraint) GETSTRUCT(constraintTup); constrName = NameStr(con->conname); + /* + * If we're asked to drop a constraint which is both defined locally and + * inherited, we can simply mark it as no longer having a local + * definition, and no further changes are required. + * + * XXX We do this for not-null constraints only, not CHECK, because the + * latter have historically not behaved this way and it might be confusing + * to change the behavior now. + */ + 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 +16653,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 +20276,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..13a6bce511 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++); + /* + * Decide on a name for this constraint. If it is not an + * inherited constraint, give it a throwaway name to avoid any + * possible conflicts, since we're going to drop it soon + * anyway. If it is inherited then try harder, because it may + * (but not necessarily) persist after the restore. + */ + if (tbinfo->notnull_inh[j]) + /* XXX maybe try harder if the name is overlength */ + 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; } @@ -17325,10 +17338,15 @@ dumpConstraint(Archive *fout, const ConstraintInfo *coninfo) * similar code in dumpIndex! */ - /* Drop any not-null constraints that were added to support the PK */ + /* + * Drop any not-null constraints that were added to support the PK, + * but leave them alone if they have a definition coming from their + * parent. + */ if (coninfo->contype == 'p') for (int i = 0; i < tbinfo->numatts; i++) - if (tbinfo->notnull_throwaway[i]) + if (tbinfo->notnull_throwaway[i] && + !tbinfo->notnull_inh[i]) appendPQExpBuffer(q, "\nALTER TABLE ONLY %s DROP CONSTRAINT %s;", fmtQualifiedDumpable(tbinfo), tbinfo->notnull_constrs[i]); 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 --