]> git.ipfire.org Git - thirdparty/postgresql.git/commitdiff
Fix restore of not-null constraints with inheritance
authorAlvaro Herrera <alvherre@alvh.no-ip.org>
Thu, 18 Apr 2024 13:35:15 +0000 (15:35 +0200)
committerAlvaro Herrera <alvherre@alvh.no-ip.org>
Thu, 18 Apr 2024 13:35:15 +0000 (15:35 +0200)
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 <andrewbille@gmail.com>
Reported-by: Justin Pryzby <pryzby@telsasoft.com>
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
src/backend/catalog/pg_constraint.c
src/backend/commands/tablecmds.c
src/bin/pg_dump/pg_dump.c
src/include/catalog/pg_constraint.h
src/test/regress/expected/constraints.out
src/test/regress/sql/constraints.sql

index cc31909012d513dbc129b4d630970e2e26acc0ed..f0278b9c0175d35b3fa4d762ef939d9b35e7958e 100644 (file)
@@ -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
index 604280d322ed95920dfe989e4ed98f8253857361..778b7c381dfddb9fb4b54c30fcdf76d3993d7df0 100644 (file)
@@ -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;
 }
 
 /*
index 027d68e5d2ae87a0f5542140376b35171478bc0d..f72b2dcadfb97771e830bfccf933172a40e2d13c 100644 (file)
@@ -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, &copytup->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
index c52e961b30914448efd249eb73c655aac926b666..13a6bce5119d0f932ffac78428a4523faf12d2c3 100644 (file)
@@ -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]);
index 8517fdafd3332c3cdde2cef92c2da5a8d68d272a..5c52d71e091a87546a007376c41e30036a0ecae7 100644 (file)
@@ -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);
index 51157181c640abbebe4fe9949a2b62397ebee67f..d50dd1f61abc458f22172d747e3724dd47f3839a 100644 (file)
@@ -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
 --
index 2efb63e9d8f53ebcb1e045f62890a21cdf97fbe2..7a39b504a319419adb347bea312de733ef7de4bc 100644 (file)
@@ -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
 --