Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit d9f686a

Browse files
committed
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 <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
1 parent e0d51e3 commit d9f686a

File tree

7 files changed

+221
-29
lines changed

7 files changed

+221
-29
lines changed

src/backend/catalog/heap.c

+32-4
Original file line numberDiff line numberDiff line change
@@ -2539,6 +2539,7 @@ AddRelationNewConstraints(Relation rel,
25392539
CookedConstraint *nncooked;
25402540
AttrNumber colnum;
25412541
char *nnname;
2542+
int existing;
25422543

25432544
/* Determine which column to modify */
25442545
colnum = get_attnum(RelationGetRelid(rel), strVal(linitial(cdef->keys)));
@@ -2547,12 +2548,39 @@ AddRelationNewConstraints(Relation rel,
25472548
strVal(linitial(cdef->keys)), RelationGetRelid(rel));
25482549

25492550
/*
2550-
* If the column already has a not-null constraint, we need only
2551-
* update its catalog status and we're done.
2551+
* If the column already has an inheritable not-null constraint,
2552+
* we need only adjust its inheritance status and we're done. If
2553+
* the constraint is there but marked NO INHERIT, then it is
2554+
* updated in the same way, but we also recurse to the children
2555+
* (if any) to add the constraint there as well.
25522556
*/
2553-
if (AdjustNotNullInheritance1(RelationGetRelid(rel), colnum,
2554-
cdef->inhcount, cdef->is_no_inherit))
2557+
existing = AdjustNotNullInheritance1(RelationGetRelid(rel), colnum,
2558+
cdef->inhcount, cdef->is_no_inherit);
2559+
if (existing == 1)
2560+
continue; /* all done! */
2561+
else if (existing == -1)
2562+
{
2563+
List *children;
2564+
2565+
children = find_inheritance_children(RelationGetRelid(rel), NoLock);
2566+
foreach_oid(childoid, children)
2567+
{
2568+
Relation childrel = table_open(childoid, NoLock);
2569+
2570+
AddRelationNewConstraints(childrel,
2571+
NIL,
2572+
list_make1(copyObject(cdef)),
2573+
allow_merge,
2574+
is_local,
2575+
is_internal,
2576+
queryString);
2577+
/* these constraints are not in the return list -- good? */
2578+
2579+
table_close(childrel, NoLock);
2580+
}
2581+
25552582
continue;
2583+
}
25562584

25572585
/*
25582586
* If a constraint name is specified, check that it isn't already

src/backend/catalog/pg_constraint.c

+30-13
Original file line numberDiff line numberDiff line change
@@ -565,8 +565,8 @@ ChooseConstraintName(const char *name1, const char *name2,
565565
}
566566

567567
/*
568-
* Find and return the pg_constraint tuple that implements a validated
569-
* not-null constraint for the given column of the given relation.
568+
* Find and return a copy of the pg_constraint tuple that implements a
569+
* validated not-null constraint for the given column of the given relation.
570570
*
571571
* XXX This would be easier if we had pg_attribute.notnullconstr with the OID
572572
* of the constraint that implements the not-null constraint for that column.
@@ -709,37 +709,54 @@ extractNotNullColumn(HeapTuple constrTup)
709709
* AdjustNotNullInheritance1
710710
* Adjust inheritance count for a single not-null constraint
711711
*
712-
* Adjust inheritance count, and possibly islocal status, for the not-null
713-
* constraint row of the given column, if it exists, and return true.
714-
* If no not-null constraint is found for the column, return false.
712+
* If no not-null constraint is found for the column, return 0.
713+
* Caller can create one.
714+
* If the constraint does exist and it's inheritable, adjust its
715+
* inheritance count (and possibly islocal status) and return 1.
716+
* No further action needs to be taken.
717+
* If the constraint exists but is marked NO INHERIT, adjust it as above
718+
* and reset connoinherit to false, and return -1. Caller is
719+
* responsible for adding the same constraint to the children, if any.
715720
*/
716-
bool
721+
int
717722
AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count,
718723
bool is_no_inherit)
719724
{
720725
HeapTuple tup;
721726

727+
Assert(count >= 0);
728+
722729
tup = findNotNullConstraintAttnum(relid, attnum);
723730
if (HeapTupleIsValid(tup))
724731
{
725732
Relation pg_constraint;
726733
Form_pg_constraint conform;
734+
int retval = 1;
727735

728736
pg_constraint = table_open(ConstraintRelationId, RowExclusiveLock);
729737
conform = (Form_pg_constraint) GETSTRUCT(tup);
730738

731739
/*
732-
* Don't let the NO INHERIT status change (but don't complain
733-
* unnecessarily.) In the future it might be useful to let an
734-
* inheritable constraint replace a non-inheritable one, but we'd need
735-
* to recurse to children to get it added there.
740+
* If we're asked for a NO INHERIT constraint and this relation
741+
* already has an inheritable one, throw an error.
736742
*/
737-
if (is_no_inherit != conform->connoinherit)
743+
if (is_no_inherit && !conform->connoinherit)
738744
ereport(ERROR,
739745
errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
740746
errmsg("cannot change NO INHERIT status of inherited NOT NULL constraint \"%s\" on relation \"%s\"",
741747
NameStr(conform->conname), get_rel_name(relid)));
742748

749+
/*
750+
* If the constraint already exists in this relation but it's marked
751+
* NO INHERIT, we can just remove that flag, and instruct caller to
752+
* recurse to add the constraint to children.
753+
*/
754+
if (!is_no_inherit && conform->connoinherit)
755+
{
756+
conform->connoinherit = false;
757+
retval = -1; /* caller must add constraint on child rels */
758+
}
759+
743760
if (count > 0)
744761
conform->coninhcount += count;
745762

@@ -761,10 +778,10 @@ AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count,
761778

762779
table_close(pg_constraint, RowExclusiveLock);
763780

764-
return true;
781+
return retval;
765782
}
766783

767-
return false;
784+
return 0;
768785
}
769786

770787
/*

src/backend/commands/tablecmds.c

+58-7
Original file line numberDiff line numberDiff line change
@@ -9336,7 +9336,6 @@ ATPrepAddPrimaryKey(List **wqueue, Relation rel, AlterTableCmd *cmd,
93369336
{
93379337
List *children;
93389338
List *newconstrs = NIL;
9339-
ListCell *lc;
93409339
IndexStmt *indexstmt;
93419340

93429341
/* No work if not creating a primary key */
@@ -9351,11 +9350,19 @@ ATPrepAddPrimaryKey(List **wqueue, Relation rel, AlterTableCmd *cmd,
93519350
!rel->rd_rel->relhassubclass)
93529351
return;
93539352

9354-
children = find_inheritance_children(RelationGetRelid(rel), lockmode);
9353+
/*
9354+
* Acquire locks all the way down the hierarchy. The recursion to lower
9355+
* levels occurs at execution time as necessary, so we don't need to do it
9356+
* here, and we don't need the returned list either.
9357+
*/
9358+
(void) find_all_inheritors(RelationGetRelid(rel), lockmode, NULL);
93559359

9356-
foreach(lc, indexstmt->indexParams)
9360+
/*
9361+
* Construct the list of constraints that we need to add to each child
9362+
* relation.
9363+
*/
9364+
foreach_node(IndexElem, elem, indexstmt->indexParams)
93579365
{
9358-
IndexElem *elem = lfirst_node(IndexElem, lc);
93599366
Constraint *nnconstr;
93609367

93619368
Assert(elem->expr == NULL);
@@ -9374,9 +9381,10 @@ ATPrepAddPrimaryKey(List **wqueue, Relation rel, AlterTableCmd *cmd,
93749381
newconstrs = lappend(newconstrs, nnconstr);
93759382
}
93769383

9377-
foreach(lc, children)
9384+
/* Finally, add AT subcommands to add each constraint to each child. */
9385+
children = find_inheritance_children(RelationGetRelid(rel), NoLock);
9386+
foreach_oid(childrelid, children)
93789387
{
9379-
Oid childrelid = lfirst_oid(lc);
93809388
Relation childrel = table_open(childrelid, NoLock);
93819389
AlterTableCmd *newcmd = makeNode(AlterTableCmd);
93829390
ListCell *lc2;
@@ -12942,6 +12950,31 @@ dropconstraint_internal(Relation rel, HeapTuple constraintTup, DropBehavior beha
1294212950
con = (Form_pg_constraint) GETSTRUCT(constraintTup);
1294312951
constrName = NameStr(con->conname);
1294412952

12953+
/*
12954+
* If we're asked to drop a constraint which is both defined locally and
12955+
* inherited, we can simply mark it as no longer having a local
12956+
* definition, and no further changes are required.
12957+
*
12958+
* XXX We do this for not-null constraints only, not CHECK, because the
12959+
* latter have historically not behaved this way and it might be confusing
12960+
* to change the behavior now.
12961+
*/
12962+
if (con->contype == CONSTRAINT_NOTNULL &&
12963+
con->conislocal && con->coninhcount > 0)
12964+
{
12965+
HeapTuple copytup;
12966+
12967+
copytup = heap_copytuple(constraintTup);
12968+
con = (Form_pg_constraint) GETSTRUCT(copytup);
12969+
con->conislocal = false;
12970+
CatalogTupleUpdate(conrel, &copytup->t_self, copytup);
12971+
ObjectAddressSet(conobj, ConstraintRelationId, con->oid);
12972+
12973+
CommandCounterIncrement();
12974+
table_close(conrel, RowExclusiveLock);
12975+
return conobj;
12976+
}
12977+
1294512978
/* Don't allow drop of inherited constraints */
1294612979
if (con->coninhcount > 0 && !recursing)
1294712980
ereport(ERROR,
@@ -16620,7 +16653,25 @@ MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel)
1662016653
errmsg("too many inheritance parents"));
1662116654
if (child_con->contype == CONSTRAINT_NOTNULL &&
1662216655
child_con->connoinherit)
16656+
{
16657+
/*
16658+
* If the child has children, it's not possible to turn a NO
16659+
* INHERIT constraint into an inheritable one: we would need
16660+
* to recurse to create constraints in those children, but
16661+
* this is not a good place to do that.
16662+
*/
16663+
if (child_rel->rd_rel->relhassubclass)
16664+
ereport(ERROR,
16665+
errmsg("cannot add NOT NULL constraint to column \"%s\" of relation \"%s\" with inheritance children",
16666+
get_attname(RelationGetRelid(child_rel),
16667+
extractNotNullColumn(child_tuple),
16668+
false),
16669+
RelationGetRelationName(child_rel)),
16670+
errdetail("Existing constraint \"%s\" is marked NO INHERIT.",
16671+
NameStr(child_con->conname)));
16672+
1662316673
child_con->connoinherit = false;
16674+
}
1662416675

1662516676
/*
1662616677
* In case of partitions, an inherited constraint must be
@@ -20225,7 +20276,7 @@ ATExecDetachPartitionFinalize(Relation rel, RangeVar *name)
2022520276
* DetachAddConstraintIfNeeded
2022620277
* Subroutine for ATExecDetachPartition. Create a constraint that
2022720278
* takes the place of the partition constraint, but avoid creating
20228-
* a dupe if an constraint already exists which implies the needed
20279+
* a dupe if a constraint already exists which implies the needed
2022920280
* constraint.
2023020281
*/
2023120282
static void

src/bin/pg_dump/pg_dump.c

+22-4
Original file line numberDiff line numberDiff line change
@@ -9096,8 +9096,21 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
90969096
}
90979097
else if (use_throwaway_notnull)
90989098
{
9099-
tbinfo->notnull_constrs[j] =
9100-
psprintf("pgdump_throwaway_notnull_%d", notnullcount++);
9099+
/*
9100+
* Decide on a name for this constraint. If it is not an
9101+
* inherited constraint, give it a throwaway name to avoid any
9102+
* possible conflicts, since we're going to drop it soon
9103+
* anyway. If it is inherited then try harder, because it may
9104+
* (but not necessarily) persist after the restore.
9105+
*/
9106+
if (tbinfo->notnull_inh[j])
9107+
/* XXX maybe try harder if the name is overlength */
9108+
tbinfo->notnull_constrs[j] =
9109+
psprintf("%s_%s_not_null",
9110+
tbinfo->dobj.name, tbinfo->attnames[j]);
9111+
else
9112+
tbinfo->notnull_constrs[j] =
9113+
psprintf("pgdump_throwaway_notnull_%d", notnullcount++);
91019114
tbinfo->notnull_throwaway[j] = true;
91029115
tbinfo->notnull_inh[j] = false;
91039116
}
@@ -17325,10 +17338,15 @@ dumpConstraint(Archive *fout, const ConstraintInfo *coninfo)
1732517338
* similar code in dumpIndex!
1732617339
*/
1732717340

17328-
/* Drop any not-null constraints that were added to support the PK */
17341+
/*
17342+
* Drop any not-null constraints that were added to support the PK,
17343+
* but leave them alone if they have a definition coming from their
17344+
* parent.
17345+
*/
1732917346
if (coninfo->contype == 'p')
1733017347
for (int i = 0; i < tbinfo->numatts; i++)
17331-
if (tbinfo->notnull_throwaway[i])
17348+
if (tbinfo->notnull_throwaway[i] &&
17349+
!tbinfo->notnull_inh[i])
1733217350
appendPQExpBuffer(q, "\nALTER TABLE ONLY %s DROP CONSTRAINT %s;",
1733317351
fmtQualifiedDumpable(tbinfo),
1733417352
tbinfo->notnull_constrs[i]);

src/include/catalog/pg_constraint.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -261,7 +261,7 @@ extern HeapTuple findNotNullConstraintAttnum(Oid relid, AttrNumber attnum);
261261
extern HeapTuple findNotNullConstraint(Oid relid, const char *colname);
262262
extern HeapTuple findDomainNotNullConstraint(Oid typid);
263263
extern AttrNumber extractNotNullColumn(HeapTuple constrTup);
264-
extern bool AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count,
264+
extern int AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count,
265265
bool is_no_inherit);
266266
extern void AdjustNotNullInheritance(Oid relid, Bitmapset *columns, int count);
267267
extern List *RelationGetNotNullConstraints(Oid relid, bool cooked);

src/test/regress/expected/constraints.out

+56
Original file line numberDiff line numberDiff line change
@@ -321,6 +321,62 @@ ALTER TABLE ATACC1 ADD NOT NULL a NO INHERIT;
321321
Inherits: atacc1
322322

323323
DROP TABLE ATACC1, ATACC2;
324+
-- overridding a no-inherit constraint with an inheritable one
325+
CREATE TABLE ATACC2 (a int, CONSTRAINT a_is_not_null NOT NULL a NO INHERIT);
326+
CREATE TABLE ATACC1 (a int);
327+
CREATE TABLE ATACC3 (a int) INHERITS (ATACC2);
328+
NOTICE: merging column "a" with inherited definition
329+
INSERT INTO ATACC3 VALUES (null); -- make sure we scan atacc3
330+
ALTER TABLE ATACC2 INHERIT ATACC1;
331+
ALTER TABLE ATACC1 ADD CONSTRAINT ditto NOT NULL a;
332+
ERROR: column "a" of relation "atacc3" contains null values
333+
DELETE FROM ATACC3;
334+
ALTER TABLE ATACC1 ADD CONSTRAINT ditto NOT NULL a;
335+
\d+ ATACC[123]
336+
Table "public.atacc1"
337+
Column | Type | Collation | Nullable | Default | Storage | Stats target | Description
338+
--------+---------+-----------+----------+---------+---------+--------------+-------------
339+
a | integer | | not null | | plain | |
340+
Not-null constraints:
341+
"ditto" NOT NULL "a"
342+
Child tables: atacc2
343+
344+
Table "public.atacc2"
345+
Column | Type | Collation | Nullable | Default | Storage | Stats target | Description
346+
--------+---------+-----------+----------+---------+---------+--------------+-------------
347+
a | integer | | not null | | plain | |
348+
Not-null constraints:
349+
"a_is_not_null" NOT NULL "a" (local, inherited)
350+
Inherits: atacc1
351+
Child tables: atacc3
352+
353+
Table "public.atacc3"
354+
Column | Type | Collation | Nullable | Default | Storage | Stats target | Description
355+
--------+---------+-----------+----------+---------+---------+--------------+-------------
356+
a | integer | | not null | | plain | |
357+
Not-null constraints:
358+
"ditto" NOT NULL "a" (inherited)
359+
Inherits: atacc2
360+
361+
ALTER TABLE ATACC2 DROP CONSTRAINT a_is_not_null;
362+
ALTER TABLE ATACC1 DROP CONSTRAINT ditto;
363+
\d+ ATACC3
364+
Table "public.atacc3"
365+
Column | Type | Collation | Nullable | Default | Storage | Stats target | Description
366+
--------+---------+-----------+----------+---------+---------+--------------+-------------
367+
a | integer | | | | plain | |
368+
Inherits: atacc2
369+
370+
DROP TABLE ATACC1, ATACC2, ATACC3;
371+
-- The same cannot be achieved this way
372+
CREATE TABLE ATACC2 (a int, CONSTRAINT a_is_not_null NOT NULL a NO INHERIT);
373+
CREATE TABLE ATACC1 (a int, CONSTRAINT ditto NOT NULL a);
374+
CREATE TABLE ATACC3 (a int) INHERITS (ATACC2);
375+
NOTICE: merging column "a" with inherited definition
376+
ALTER TABLE ATACC2 INHERIT ATACC1;
377+
ERROR: cannot add NOT NULL constraint to column "a" of relation "atacc2" with inheritance children
378+
DETAIL: Existing constraint "a_is_not_null" is marked NO INHERIT.
379+
DROP TABLE ATACC1, ATACC2, ATACC3;
324380
--
325381
-- Check constraints on INSERT INTO
326382
--

src/test/regress/sql/constraints.sql

+22
Original file line numberDiff line numberDiff line change
@@ -212,6 +212,28 @@ ALTER TABLE ATACC1 ADD NOT NULL a NO INHERIT;
212212
\d+ ATACC2
213213
DROP TABLE ATACC1, ATACC2;
214214

215+
-- overridding a no-inherit constraint with an inheritable one
216+
CREATE TABLE ATACC2 (a int, CONSTRAINT a_is_not_null NOT NULL a NO INHERIT);
217+
CREATE TABLE ATACC1 (a int);
218+
CREATE TABLE ATACC3 (a int) INHERITS (ATACC2);
219+
INSERT INTO ATACC3 VALUES (null); -- make sure we scan atacc3
220+
ALTER TABLE ATACC2 INHERIT ATACC1;
221+
ALTER TABLE ATACC1 ADD CONSTRAINT ditto NOT NULL a;
222+
DELETE FROM ATACC3;
223+
ALTER TABLE ATACC1 ADD CONSTRAINT ditto NOT NULL a;
224+
\d+ ATACC[123]
225+
ALTER TABLE ATACC2 DROP CONSTRAINT a_is_not_null;
226+
ALTER TABLE ATACC1 DROP CONSTRAINT ditto;
227+
\d+ ATACC3
228+
DROP TABLE ATACC1, ATACC2, ATACC3;
229+
230+
-- The same cannot be achieved this way
231+
CREATE TABLE ATACC2 (a int, CONSTRAINT a_is_not_null NOT NULL a NO INHERIT);
232+
CREATE TABLE ATACC1 (a int, CONSTRAINT ditto NOT NULL a);
233+
CREATE TABLE ATACC3 (a int) INHERITS (ATACC2);
234+
ALTER TABLE ATACC2 INHERIT ATACC1;
235+
DROP TABLE ATACC1, ATACC2, ATACC3;
236+
215237
--
216238
-- Check constraints on INSERT INTO
217239
--

0 commit comments

Comments
 (0)