Re: pg17 issues with not-null contraints
On Mon, May 06, 2024 at 06:34:16PM +0200, Alvaro Herrera wrote: > On 2024-May-06, Justin Pryzby wrote: > > > > (Do you really want the partition to be > > > created without the primary key already there?) > > > > Why not ? The PK will be added when I attach it one moment later. > > > > CREATE TABLE part (LIKE parent); > > ALTER TABLE parent ATTACH PARTITION part ... > > Well, if you load data in the meantime, you'll spend time during `ALTER > TABLE parent` for the index to be created. (On the other hand, you may > want to first create the table, then load data, then create the > indexes.) To be clear, I'm referring to the case of CREATE+ATTACH to avoid a strong lock while creating a partition in advance of loading data. See: 20220718143304.gc18...@telsasoft.com f170b572d2b4cc232c5b6d391b4ecf3e368594b7 898e5e3290a72d288923260143930fb32036c00c > This would also solve your complaint, because then the table would have > the not-null constraint in all cases. I agree that it would solve my complaint, but at this time I've no further opinion. -- Justin
Re: pg17 issues with not-null contraints
On 2024-May-06, Justin Pryzby wrote: > > (Do you really want the partition to be > > created without the primary key already there?) > > Why not ? The PK will be added when I attach it one moment later. > > CREATE TABLE part (LIKE parent); > ALTER TABLE parent ATTACH PARTITION part ... Well, if you load data in the meantime, you'll spend time during `ALTER TABLE parent` for the index to be created. (On the other hand, you may want to first create the table, then load data, then create the indexes.) > Do you really think that after ATTACH, the constraints should be > different depending on whether the child was created INCLUDING INDEXES ? > I'll continue to think about this, but I still find that surprising. I don't think I have a choice about this, because the standard says that the resulting table must have NOT NULL on all columns which have a nullability characteristic is known not nullable; and the primary key forces that to be the case. Thinking again, maybe this is wrong in the opposite direction: perhaps we should have not-null constraints on those columns even if INCLUDING CONSTRAINTS is given, because failing to do that (which is the current behavior) is non-conformant. In turn, this suggests that in order to make the partitioning behavior consistent, we should _in addition_ make CREATE TABLE PARTITION OF add explicit not-null constraints to the columns of the primary key of the partitioned table. This would also solve your complaint, because then the table would have the not-null constraint in all cases. This might be taking the whole affair too far, though; not sure. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/ "We’ve narrowed the problem down to the customer’s pants being in a situation of vigorous combustion" (Robert Haas, Postgres expert extraordinaire)
Re: pg17 issues with not-null contraints
On Mon, May 06, 2024 at 05:56:54PM +0200, Alvaro Herrera wrote: > On 2024-May-04, Alvaro Herrera wrote: > > On 2024-May-03, Justin Pryzby wrote: > > > > > But if it's created with LIKE: > > > postgres=# CREATE TABLE t1 (LIKE t); > > > postgres=# ALTER TABLE t ATTACH PARTITION t1 DEFAULT ; > > > > > > ..one also sees: > > > > > > Not-null constraints: > > > "t1_i_not_null" NOT NULL "i" > > > > Hmm, I think the problem here is not ATTACH; the not-null constraint is > > there immediately after CREATE. I think this is all right actually, > > because we derive a not-null constraint from the primary key and this is > > definitely intentional. But I also think that if you do CREATE TABLE t1 > > (LIKE t INCLUDING CONSTRAINTS) then you should get only the primary key > > and no separate not-null constraint. That will make the table more > > similar to the one being copied. > > I misspoke -- it's INCLUDING INDEXES that we need here, not INCLUDING > CONSTRAINTS ... and it turns out we already do it that way, so with this > script > > CREATE TABLE t (i int PRIMARY KEY) PARTITION BY RANGE (i); > CREATE TABLE t1 (LIKE t INCLUDING INDEXES); > ALTER TABLE t ATTACH PARTITION t1 DEFAULT ; > > you end up with this > > 55432 17devel 71313=# \d+ t > Partitioned table "public.t" > Column │ Type │ Collation │ Nullable │ Default │ Storage │ Compression │ > Stats target │ Description > ┼─┼───┼──┼─┼─┼─┼──┼─ > i │ integer │ │ not null │ │ plain │ │ > │ > Partition key: RANGE (i) > Indexes: > "t_pkey" PRIMARY KEY, btree (i) > Partitions: t1 DEFAULT > > 55432 17devel 71313=# \d+ t1 >Table "public.t1" > Column │ Type │ Collation │ Nullable │ Default │ Storage │ Compression │ > Stats target │ Description > ┼─┼───┼──┼─┼─┼─┼──┼─ > i │ integer │ │ not null │ │ plain │ │ > │ > Partition of: t DEFAULT > No partition constraint > Indexes: > "t1_pkey" PRIMARY KEY, btree (i) > Access method: heap > > which I think is what you want. (Do you really want the partition to be > created without the primary key already there?) Why not ? The PK will be added when I attach it one moment later. CREATE TABLE part (LIKE parent); ALTER TABLE parent ATTACH PARTITION part ... Do you really think that after ATTACH, the constraints should be different depending on whether the child was created INCLUDING INDEXES ? I'll continue to think about this, but I still find that surprising. -- Justin
Re: pg17 issues with not-null contraints
On 2024-May-04, Alvaro Herrera wrote: > On 2024-May-03, Justin Pryzby wrote: > > > But if it's created with LIKE: > > postgres=# CREATE TABLE t1 (LIKE t); > > postgres=# ALTER TABLE t ATTACH PARTITION t1 DEFAULT ; > > > > ..one also sees: > > > > Not-null constraints: > > "t1_i_not_null" NOT NULL "i" > > Hmm, I think the problem here is not ATTACH; the not-null constraint is > there immediately after CREATE. I think this is all right actually, > because we derive a not-null constraint from the primary key and this is > definitely intentional. But I also think that if you do CREATE TABLE t1 > (LIKE t INCLUDING CONSTRAINTS) then you should get only the primary key > and no separate not-null constraint. That will make the table more > similar to the one being copied. I misspoke -- it's INCLUDING INDEXES that we need here, not INCLUDING CONSTRAINTS ... and it turns out we already do it that way, so with this script CREATE TABLE t (i int PRIMARY KEY) PARTITION BY RANGE (i); CREATE TABLE t1 (LIKE t INCLUDING INDEXES); ALTER TABLE t ATTACH PARTITION t1 DEFAULT ; you end up with this 55432 17devel 71313=# \d+ t Partitioned table "public.t" Column │ Type │ Collation │ Nullable │ Default │ Storage │ Compression │ Stats target │ Description ┼─┼───┼──┼─┼─┼─┼──┼─ i │ integer │ │ not null │ │ plain │ │ │ Partition key: RANGE (i) Indexes: "t_pkey" PRIMARY KEY, btree (i) Partitions: t1 DEFAULT 55432 17devel 71313=# \d+ t1 Table "public.t1" Column │ Type │ Collation │ Nullable │ Default │ Storage │ Compression │ Stats target │ Description ┼─┼───┼──┼─┼─┼─┼──┼─ i │ integer │ │ not null │ │ plain │ │ │ Partition of: t DEFAULT No partition constraint Indexes: "t1_pkey" PRIMARY KEY, btree (i) Access method: heap which I think is what you want. (Do you really want the partition to be created without the primary key already there?) Now maybe in https://www.postgresql.org/docs/devel/sql-createtable.html we need some explanation for this. Right now we have INCLUDING INDEXES Indexes, PRIMARY KEY, UNIQUE, and EXCLUDE constraints on the original table will be created on the new table. Names for the new indexes and constraints are chosen according to the default rules, regardless of how the originals were named. (This behavior avoids possible duplicate-name failures for the new indexes.) Maybe something like this before the naming considerations: When creating a table like another that has a primary key and indexes are excluded, a not-null constraint will be added to every column of the primary key. resulting in INCLUDING INDEXES Indexes, PRIMARY KEY, UNIQUE, and EXCLUDE constraints on the original table will be created on the new table. [When/If ?] indexes are excluded while creating a table like another that has a primary key, a not-null constraint will be added to every column of the primary key. Names for the new indexes and constraints are chosen according to the default rules, regardless of how the originals were named. (This behavior avoids possible duplicate-name failures for the new indexes.) What do you think? -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/
Re: pg17 issues with not-null contraints
On 2024-May-03, Justin Pryzby wrote: > But if it's created with LIKE: > postgres=# CREATE TABLE t1 (LIKE t); > postgres=# ALTER TABLE t ATTACH PARTITION t1 DEFAULT ; > > ..one also sees: > > Not-null constraints: > "t1_i_not_null" NOT NULL "i" Hmm, I think the problem here is not ATTACH; the not-null constraint is there immediately after CREATE. I think this is all right actually, because we derive a not-null constraint from the primary key and this is definitely intentional. But I also think that if you do CREATE TABLE t1 (LIKE t INCLUDING CONSTRAINTS) then you should get only the primary key and no separate not-null constraint. That will make the table more similar to the one being copied. Does that make sense to you? -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "Estoy de acuerdo contigo en que la verdad absoluta no existe... El problema es que la mentira sí existe y tu estás mintiendo" (G. Lama)
Re: pg17 issues with not-null contraints
On another thread [0], Alexander Lakhin pointed out, indirectly, that partitions created using LIKE+ATTACH now have different not-null constraints from partitions created using PARTITION OF. postgres=# CREATE TABLE t (i int PRIMARY KEY) PARTITION BY RANGE (i); postgres=# CREATE TABLE t1 PARTITION OF t DEFAULT ; postgres=# \d+ t1 ... Partition of: t DEFAULT No partition constraint Indexes: "t1_pkey" PRIMARY KEY, btree (i) Access method: heap But if it's created with LIKE: postgres=# CREATE TABLE t1 (LIKE t); postgres=# ALTER TABLE t ATTACH PARTITION t1 DEFAULT ; ..one also sees: Not-null constraints: "t1_i_not_null" NOT NULL "i" It looks like ATTACH may have an issue with constraints implied by pkey. [0] https://www.postgresql.org/message-id/8034d1c6-5f0e-e858-9af9-45d5e246515e%40gmail.com -- Justin
Re: pg17 issues with not-null contraints
On Tue, Apr 30, 2024 at 01:52:02PM -0400, Robert Haas wrote: > On Thu, Apr 18, 2024 at 12:52 PM Justin Pryzby wrote: > > I'm not totally clear on what's intended in v17 - maybe it'd be dead > > code, and maybe it shouldn't even be applied to master branch. But I do > > think it's worth patching earlier versions (even though it'll be less > > useful than having done so 5 years ago). > > This thread is still on the open items list, but I'm not sure whether > there's still stuff here that needs to be fixed for the current > release. If not, this thread should be moved to the "resolved before > 17beta1" section. If so, we should try to reach consensus on what the > remaining issues are and what we're going to do about them. I think the only thing that's relevant for v17 is this: On Tue, Apr 16, 2024 at 08:11:49PM +0200, Alvaro Herrera wrote: > Speaking of which, I wonder if I should modify pg16's tests so that they > leave behind tables set up in this way, to immortalize pg_upgrade testing. The patch on the other thread for pg_upgrade --check is an old issue affecting all stable releases. -- Justin
Re: pg17 issues with not-null contraints
On Thu, Apr 18, 2024 at 12:52 PM Justin Pryzby wrote: > I'm not totally clear on what's intended in v17 - maybe it'd be dead > code, and maybe it shouldn't even be applied to master branch. But I do > think it's worth patching earlier versions (even though it'll be less > useful than having done so 5 years ago). This thread is still on the open items list, but I'm not sure whether there's still stuff here that needs to be fixed for the current release. If not, this thread should be moved to the "resolved before 17beta1" section. If so, we should try to reach consensus on what the remaining issues are and what we're going to do about them. -- Robert Haas EDB: http://www.enterprisedb.com
Re: pg17 issues with not-null contraints
On Thu, Apr 18, 2024 at 06:23:30PM +0200, Alvaro Herrera wrote: > On 2024-Apr-18, Justin Pryzby wrote: > > > BTW, this works up to v16 (although maybe it should not): > > > > | CREATE TABLE ip(id int PRIMARY KEY); CREATE TABLE ic(id int) INHERITS > > (ip); ALTER TABLE ic ALTER id DROP NOT NULL; > > > It'd still be nice to detect the issue in advance rather than failing > > halfway through the upgrade. > > Maybe we can have pg_upgrade --check look for cases we might have > trouble upgrading. (I mean: such cases would fail if you have rows with > nulls in the affected columns, but the schema upgrade should be > successful. Is that what you have in mind?) Before v16, pg_upgrade failed in the middle of restoring the schema, without being caught during --check. The patch to implement that was forgotten and never progressed. I'm not totally clear on what's intended in v17 - maybe it'd be dead code, and maybe it shouldn't even be applied to master branch. But I do think it's worth patching earlier versions (even though it'll be less useful than having done so 5 years ago). -- Justin
Re: pg17 issues with not-null contraints
On 2024-Apr-18, Justin Pryzby wrote: > That seems like it could be important. I considered but never actually > test your patch by pg_upgrading across major versions. It would be a welcome contribution for sure. I've been doing it rather haphazardly, which is not great. > BTW, this works up to v16 (although maybe it should not): > > | CREATE TABLE ip(id int PRIMARY KEY); CREATE TABLE ic(id int) INHERITS (ip); > ALTER TABLE ic ALTER id DROP NOT NULL; > > Under v17, this fails. Maybe that's okay, but it should probably be > called out in the release notes. Sure, we should mention that. > | ERROR: cannot drop inherited constraint "ic_id_not_null" of relation "ic" > > That's the issue that I mentioned in the 6 year old thread. In the > future (upgrading *from* v17) it won't be possible anymore, right? Yeah, trying to drop the constraint in 17 fails as it should; it was one of the goals of this whole thing in fact. > It'd still be nice to detect the issue in advance rather than failing > halfway through the upgrade. Maybe we can have pg_upgrade --check look for cases we might have trouble upgrading. (I mean: such cases would fail if you have rows with nulls in the affected columns, but the schema upgrade should be successful. Is that what you have in mind?) > I have a rebased patch while I'll send on that thread. I guess it's > mostly unrelated to your patch but it'd be nice if you could take a > look. Okay. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Industry suffers from the managerial dogma that for the sake of stability and continuity, the company should be independent of the competence of individual employees." (E. Dijkstra)
Re: pg17 issues with not-null contraints
On Tue, Apr 16, 2024 at 08:11:49PM +0200, Alvaro Herrera wrote: > This is still missing some cleanup and additional tests, of course. > Speaking of which, I wonder if I should modify pg16's tests so that they > leave behind tables set up in this way, to immortalize pg_upgrade > testing. That seems like it could be important. I considered but never actually test your patch by pg_upgrading across major versions. BTW, this works up to v16 (although maybe it should not): | CREATE TABLE ip(id int PRIMARY KEY); CREATE TABLE ic(id int) INHERITS (ip); ALTER TABLE ic ALTER id DROP NOT NULL; Under v17, this fails. Maybe that's okay, but it should probably be called out in the release notes. | ERROR: cannot drop inherited constraint "ic_id_not_null" of relation "ic" That's the issue that I mentioned in the 6 year old thread. In the future (upgrading *from* v17) it won't be possible anymore, right? It'd still be nice to detect the issue in advance rather than failing halfway through the upgrade. I have a rebased patch while I'll send on that thread. I guess it's mostly unrelated to your patch but it'd be nice if you could take a look. -- Justin
Re: pg17 issues with not-null contraints
On 2024-Apr-15, Justin Pryzby wrote: > Here's a couple more issues affecting upgrades from v16 to v17. > > postgres=# CREATE TABLE a(i int NOT NULL); CREATE TABLE b(i int PRIMARY KEY) > INHERITS (a); > pg_restore: error: could not execute query: ERROR: constraint > "pgdump_throwaway_notnull_0" of relation "b" does not exist > > postgres=# CREATE TABLE a(i int CONSTRAINT a NOT NULL PRIMARY KEY); CREATE > TABLE b()INHERITS(a); ALTER TABLE b ADD CONSTRAINT pkb PRIMARY KEY (i); > pg_restore: error: could not execute query: ERROR: cannot drop inherited > constraint "pgdump_throwaway_notnull_0" of relation "b" I pushed a fix now, and it should also cover these two issues, which required only minor changes over what I posted yesterday. Also, thank you for pointing out that the patch also fixed Andrew's problem. It did, except there was a locking problem which required an additional tweak. Thanks for reporting these. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/ "At least to kernel hackers, who really are human, despite occasional rumors to the contrary" (LWN.net)
Re: pg17 issues with not-null contraints
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 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 Reported-by: Justin Pryzby 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) +
Re: pg17 issues with not-null contraints
On Tue, Apr 16, 2024 at 08:11:49PM +0200, Alvaro Herrera wrote: > On 2024-Apr-15, Alvaro Herrera wrote: > > > - Fourth thought: we do as in the third thought, except we also allow > > DROP CONSTRAINT a constraint that's marked "local, inherited" to be > > simply an inherited constraint (remove its "local" marker). > > Here is an initial implementation of what I was thinking. Can you > please give it a try and see if it fixes this problem? At least in my > run of your original test case, it seems to work as expected. Yes, this fixes the issue I reported. 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 -- Justin
Re: pg17 issues with not-null contraints
On 2024-Apr-15, Justin Pryzby wrote: > Here's a couple more issues affecting upgrades from v16 to v17. > > postgres=# CREATE TABLE a(i int NOT NULL); CREATE TABLE b(i int PRIMARY KEY) > INHERITS (a); > pg_restore: error: could not execute query: ERROR: constraint > "pgdump_throwaway_notnull_0" of relation "b" does not exist This one requires a separate pg_dump fix, which should --I hope-- be pretty simple. > postgres=# CREATE TABLE a(i int CONSTRAINT a NOT NULL PRIMARY KEY); CREATE > TABLE b()INHERITS(a); ALTER TABLE b ADD CONSTRAINT pkb PRIMARY KEY (i); > pg_restore: error: could not execute query: ERROR: cannot drop inherited > constraint "pgdump_throwaway_notnull_0" of relation "b" This one seems to be fixed with the patch I just posted. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "I am amazed at [the pgsql-sql] mailing list for the wonderful support, and lack of hesitasion in answering a lost soul's question, I just wished the rest of the mailing list could be like this." (Fotis) https://postgr.es/m/200606261359.k5qdxe2p004...@auth-smtp.hol.gr
Re: pg17 issues with not-null contraints
On 2024-Apr-15, Alvaro Herrera wrote:
> - Fourth thought: we do as in the third thought, except we also allow
> DROP CONSTRAINT a constraint that's marked "local, inherited" to be
> simply an inherited constraint (remove its "local" marker).
Here is an initial implementation of what I was thinking. Can you
please give it a try and see if it fixes this problem? At least in my
run of your original test case, it seems to work as expected.
This is still missing some cleanup and additional tests, of course.
Speaking of which, I wonder if I should modify pg16's tests so that they
leave behind tables set up in this way, to immortalize pg_upgrade
testing.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
>From b40a418dfb3d7ce23ffa568c8a8d03640ce28435 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Tue, 16 Apr 2024 13:44:34 +0200
Subject: [PATCH] Fix add/drop of not-null constraints
---
src/backend/catalog/heap.c | 36 +
src/backend/catalog/pg_constraint.c | 36 -
src/backend/commands/tablecmds.c| 26 -
src/include/catalog/pg_constraint.h | 2 +-
4 files changed, 83 insertions(+), 17 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..a11483b1b8 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -709,36 +709,50 @@ 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
+ * If the constraint does exist and it's inheritable, adjust its
+ * inheritance count (and possibly islocal status) and return 1.
+ * 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 the constraint already exists in this relation but it's
Re: pg17 issues with not-null contraints
On Mon, Apr 15, 2024 at 03:47:38PM +0200, Alvaro Herrera wrote: > On 2024-Apr-15, Justin Pryzby wrote: > > > I think there are other issues related to b0e96f3119 (Catalog not-null > > constraints) - if I dump a v16 server using v17 tools, the backup can't > > be restored into the v16 server. I'm okay ignoring a line or two like > > 'unrecognized configuration parameter "transaction_timeout", but not > > 'syntax error at or near "NO"'. > > This doesn't look something that we can handle at all. The assumption > is that pg_dump's output is going to be fed to a server that's at least > the same version. Running on older versions is just not supported. You're right - the docs say: |Also, it is not guaranteed that pg_dump's output can be loaded into a |server of an older major version — not even if the dump was taken from a |server of that version Here's a couple more issues affecting upgrades from v16 to v17. postgres=# CREATE TABLE a(i int NOT NULL); CREATE TABLE b(i int PRIMARY KEY) INHERITS (a); pg_restore: error: could not execute query: ERROR: constraint "pgdump_throwaway_notnull_0" of relation "b" does not exist postgres=# CREATE TABLE a(i int CONSTRAINT a NOT NULL PRIMARY KEY); CREATE TABLE b()INHERITS(a); ALTER TABLE b ADD CONSTRAINT pkb PRIMARY KEY (i); pg_restore: error: could not execute query: ERROR: cannot drop inherited constraint "pgdump_throwaway_notnull_0" of relation "b" -- Justin
Re: pg17 issues with not-null contraints
On 2024-Apr-15, Alvaro Herrera wrote: > On 2024-Apr-15, Justin Pryzby wrote: > > postgres=# CREATE TABLE iparent(id serial PRIMARY KEY); CREATE TABLE child > > (id int) INHERITS (iparent); ALTER TABLE child ALTER id DROP NOT NULL; > > ALTER TABLE child ADD CONSTRAINT p PRIMARY KEY (id); > > > > $ pg_dump -h /tmp -p 5678 postgres -Fc |pg_restore -1 -h /tmp -p 5679 -d > > postgres > > ERROR: cannot change NO INHERIT status of inherited NOT NULL constraint > > "pgdump_throwaway_notnull_0" on relation "child" > > STATEMENT: ALTER TABLE ONLY public.iparent > > ADD CONSTRAINT iparent_pkey PRIMARY KEY (id); > Hmm, apparently if the table is "iparent", the primary key is created in > the child first; if the table is "parent", then the PK is created first > there. I think the problem is that the ADD CONSTRAINT for the PK should > not be recursing at all in this case ... seeing in particular that the > command specifies ONLY. Should be a simple fix, looking now. So the problem is that the ADD CONSTRAINT PRIMARY KEY in the parent table wants to recurse to the child, so that a NOT NULL constraint is created on each column. If the child is created first, there's already a NOT NULL NO INHERIT constraint in it which was created for its own primary key, so the internal recursion in the parent's ADD PK fails. A fix doesn't look all that simple: - As I said in my earlier reply, my first thought was to have ALTER TABLE ADD PRIMARY KEY not recurse if the command is ALTER TABLE ONLY. This doesn't work, because the point of that recursion is precisely to handle this case, so if we do that, we break the other stuff that this was added to solve. - Second thought was to add a bespoke dependency in pg_dump.c so that the child PK is dumped after the parent PK. I looked at the code, didn't like the idea of adding such a hack, went looking for other ideas. - Third thought was to hack AdjustNotNullInheritance1() so that it changes the conisnoinherit flag in this particular case. Works great, except that once we mark this constraint as inherited, we cannot drop it; and since it's a constraint marked "throwaway", pg_dump expects to be able to drop it, which means the ALTER TABLE DROP CONSTRAINT throws an error, and a constraint named pgdump_throwaway_notnull_0 remains in place. - Fourth thought: we do as in the third thought, except we also allow DROP CONSTRAINT a constraint that's marked "local, inherited" to be simply an inherited constraint (remove its "local" marker). I'm going to try to implement this fourth idea, which seems promising. I think if we do that, the end result will be identical to the case where the child is created after the parent. However, we'll also need that constraint to have a name better than pgdump_throwaway_notnull_NN. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Re: pg17 issues with not-null contraints
On 2024-Apr-15, Justin Pryzby wrote:
> 9b581c5341 can break dump/restore from old versions, including
> pgupgrade.
>
> postgres=# CREATE TABLE iparent(id serial PRIMARY KEY); CREATE TABLE child
> (id int) INHERITS (iparent); ALTER TABLE child ALTER id DROP NOT NULL; ALTER
> TABLE child ADD CONSTRAINT p PRIMARY KEY (id);
>
> $ pg_dump -h /tmp -p 5678 postgres -Fc |pg_restore -1 -h /tmp -p 5679 -d
> postgres
> ERROR: cannot change NO INHERIT status of inherited NOT NULL constraint
> "pgdump_throwaway_notnull_0" on relation "child"
> STATEMENT: ALTER TABLE ONLY public.iparent
> ADD CONSTRAINT iparent_pkey PRIMARY KEY (id);
>
> Strangely, if I name the table "parent", it seems to work, which might
> indicate an ordering/dependency issue.
Hmm, apparently if the table is "iparent", the primary key is created in
the child first; if the table is "parent", then the PK is created first
there. I think the problem is that the ADD CONSTRAINT for the PK should
not be recursing at all in this case ... seeing in particular that the
command specifies ONLY. Should be a simple fix, looking now.
> I think there are other issues related to b0e96f3119 (Catalog not-null
> constraints) - if I dump a v16 server using v17 tools, the backup can't
> be restored into the v16 server. I'm okay ignoring a line or two like
> 'unrecognized configuration parameter "transaction_timeout", but not
> 'syntax error at or near "NO"'.
This doesn't look something that we can handle at all. The assumption
is that pg_dump's output is going to be fed to a server that's at least
the same version. Running on older versions is just not supported.
> The other version checks in pg_dump.c are used to construct sql for
> querying the source db, but this is used to create the sql to restore
> the target, using syntax that didn't exist until v17.
>
> if (print_notnull)
> {
> if (tbinfo->notnull_constrs[j][0] == '\0')
> appendPQExpBufferStr(q, " NOT NULL");
> else
> appendPQExpBuffer(q, " CONSTRAINT %s NOT NULL",
>
> fmtId(tbinfo->notnull_constrs[j]));
>
> if (tbinfo->notnull_noinh[j])
> appendPQExpBufferStr(q, " NO INHERIT");
> }
If you have ideas on what to do about this, I'm all ears, but keep in
mind that pg_dump doesn't necessarily know what the target version is.
>
> This other thread is 6 years old and forgotten again, but still seems
> relevant.
> https://www.postgresql.org/message-id/flat/b8794d6a-38f0-9d7c-ad4b-e85adf860fc9%40enterprisedb.com
I only skimmed very briefly, but it looks related to commit c3709100be73
that I pushed earlier today. Or if you have some specific case that
fails to be handled please let me know. (Maybe we should have the
regress tests leave some tables behind to ensure the pg_upgrade behavior
is what we want, if we continue to break it.)
> BTW, these comments are out of date:
>
> +* In versions 16 and up, we need pg_constraint for explicit NOT NULL
> + if (fout->remoteVersion >= 17)
>
> + * that we needn't specify that again for the child. (Versions >= 16 no
> + if (fout->remoteVersion < 17)
Thanks, will fix. But I'm probably touching this code in the fix for
Andrew Bille's problem, so I might not do so immediately.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Estoy de acuerdo contigo en que la verdad absoluta no existe...
El problema es que la mentira sí existe y tu estás mintiendo" (G. Lama)
