On 2024-Apr-10, Alvaro Herrera wrote:
> One thing missing here is pg_dump support. If you just dump this table,
> it'll end up with no constraint at all. That's obviously bad, so I
> propose we have pg_dump add a regular NOT NULL constraint for those, to
> avoid perpetuating the weird situation further.
Here's another crude patchset, this time including the pg_dump aspect.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"On the other flipper, one wrong move and we're Fatal Exceptions"
(T.U.X.: Term Unit X - http://www.thelinuxreview.com/TUX/)
>From 3a00358847f1792c01e608909dc8a4a0edb8739c Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Wed, 10 Apr 2024 13:02:35 +0200
Subject: [PATCH v2 1/2] Handle ALTER .. DROP NOT NULL when no pg_constraint
row exists
---
src/backend/commands/tablecmds.c | 67 +++++++++++++++++++----
src/test/regress/expected/constraints.out | 55 +++++++++++++++++++
src/test/regress/sql/constraints.sql | 29 ++++++++++
3 files changed, 139 insertions(+), 12 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 865c6331c1..f692001e55 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -448,6 +448,7 @@ static ObjectAddress ATExecDropNotNull(Relation rel, const char *colName, bool r
LOCKMODE lockmode);
static bool set_attnotnull(List **wqueue, Relation rel,
AttrNumber attnum, bool recurse, LOCKMODE lockmode);
+static void drop_orphaned_notnull(Oid relationOid, AttrNumber attnum);
static ObjectAddress ATExecSetNotNull(List **wqueue, Relation rel,
char *constrname, char *colName,
bool recurse, bool recursing,
@@ -7678,17 +7679,23 @@ ATExecDropNotNull(Relation rel, const char *colName, bool recurse,
}
/*
- * Find the constraint that makes this column NOT NULL.
+ * Find the constraint that makes this column NOT NULL, and drop it if we
+ * see one. dropconstraint_internal() will do necessary consistency
+ * checking. If there isn't one, there are two possibilities: either the
+ * column is marked attnotnull because it's part of the primary key, and
+ * then we just throw an appropriate error; or it's a leftover marking that
+ * we can remove. However, before doing the latter, to avoid breaking
+ * consistency any further, prevent this if the column is part of the
+ * replica identity.
*/
conTup = findNotNullConstraint(RelationGetRelid(rel), colName);
if (conTup == NULL)
{
Bitmapset *pkcols;
+ Bitmapset *ircols;
/*
- * There's no not-null constraint, so throw an error. If the column
- * is in a primary key, we can throw a specific error. Otherwise,
- * this is unexpected.
+ * If the column is in a primary key, throw a specific error message.
*/
pkcols = RelationGetIndexAttrBitmap(rel, INDEX_ATTR_BITMAP_PRIMARY_KEY);
if (bms_is_member(attnum - FirstLowInvalidHeapAttributeNumber,
@@ -7697,16 +7704,25 @@ ATExecDropNotNull(Relation rel, const char *colName, bool recurse,
errcode(ERRCODE_INVALID_TABLE_DEFINITION),
errmsg("column \"%s\" is in a primary key", colName));
- /* this shouldn't happen */
- elog(ERROR, "could not find not-null constraint on column \"%s\", relation \"%s\"",
- colName, RelationGetRelationName(rel));
+ /* Also throw an error if the column is in the replica identity */
+ ircols = RelationGetIndexAttrBitmap(rel, INDEX_ATTR_BITMAP_IDENTITY_KEY);
+ if (bms_is_member(attnum - FirstLowInvalidHeapAttributeNumber, ircols))
+ ereport(ERROR,
+ errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+ errmsg("column \"%s\" is in index used as replica identity",
+ get_attname(RelationGetRelid(rel), attnum, false)));
+
+ /* Otherwise, just remove the attnotnull marking and do nothing else. */
+ drop_orphaned_notnull(RelationGetRelid(rel), attnum);
}
+ else
+ {
+ readyRels = NIL;
+ dropconstraint_internal(rel, conTup, DROP_RESTRICT, recurse, false,
+ false, &readyRels, lockmode);
- readyRels = NIL;
- dropconstraint_internal(rel, conTup, DROP_RESTRICT, recurse, false,
- false, &readyRels, lockmode);
-
- heap_freetuple(conTup);
+ heap_freetuple(conTup);
+ }
InvokeObjectPostAlterHook(RelationRelationId,
RelationGetRelid(rel), attnum);
@@ -7796,6 +7812,33 @@ set_attnotnull(List **wqueue, Relation rel, AttrNumber attnum, bool recurse,
return retval;
}
+/*
+ * In rare cases, a column can end up with an "orphaned" attnotnull marking
+ * with no corresponding pg_constraint row. If the user then does ALTER TABLE
+ * DROP NOT NULL, this takes care of resetting that.
+ */
+static void
+drop_orphaned_notnull(Oid relationOid, AttrNumber attnum)
+{
+ Relation attr_rel;
+ HeapTuple tuple;
+ Form_pg_attribute attForm;
+
+ attr_rel = table_open(AttributeRelationId, RowExclusiveLock);
+
+ tuple = SearchSysCacheCopyAttNum(relationOid, attnum);
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for attribute %d of relation %u",
+ attnum, relationOid);
+ attForm = (Form_pg_attribute) GETSTRUCT(tuple);
+
+ attForm->attnotnull = false;
+ CatalogTupleUpdate(attr_rel, &tuple->t_self, tuple);
+
+ table_close(attr_rel, RowExclusiveLock);
+}
+
+
/*
* ALTER TABLE ALTER COLUMN SET NOT NULL
*
diff --git a/src/test/regress/expected/constraints.out b/src/test/regress/expected/constraints.out
index d9d8408e86..f0c166dc4f 100644
--- a/src/test/regress/expected/constraints.out
+++ b/src/test/regress/expected/constraints.out
@@ -866,6 +866,61 @@ select conname, contype, conkey from pg_constraint where conrelid = 'notnull_tbl
foobar | n | {1}
(1 row)
+DROP TABLE notnull_tbl1;
+-- make sure attnotnull is reset correctly when a PK is dropped indirectly
+CREATE TABLE notnull_tbl1 (c0 int, c1 int, PRIMARY KEY (c0, c1));
+ALTER TABLE notnull_tbl1 DROP c1;
+\d+ notnull_tbl1
+ Table "public.notnull_tbl1"
+ Column | Type | Collation | Nullable | Default | Storage | Stats target | Description
+--------+---------+-----------+----------+---------+---------+--------------+-------------
+ c0 | integer | | not null | | plain | |
+
+ALTER TABLE notnull_tbl1 ALTER c0 DROP NOT NULL;
+\d+ notnull_tbl1
+ Table "public.notnull_tbl1"
+ Column | Type | Collation | Nullable | Default | Storage | Stats target | Description
+--------+---------+-----------+----------+---------+---------+--------------+-------------
+ c0 | integer | | | | plain | |
+
+DROP TABLE notnull_tbl1;
+-- again
+CREATE DOMAIN notnull_dom1 AS INTEGER;
+CREATE TABLE notnull_tbl1 (c0 notnull_dom1, c1 int, PRIMARY KEY (c0, c1));
+DROP DOMAIN notnull_dom1 CASCADE;
+NOTICE: drop cascades to column c0 of table notnull_tbl1
+\d+ notnull_tbl1
+ Table "public.notnull_tbl1"
+ Column | Type | Collation | Nullable | Default | Storage | Stats target | Description
+--------+---------+-----------+----------+---------+---------+--------------+-------------
+ c1 | integer | | not null | | plain | |
+
+ALTER TABLE notnull_tbl1 ALTER c1 SET NOT NULL;
+\d+ notnull_tbl1
+ Table "public.notnull_tbl1"
+ Column | Type | Collation | Nullable | Default | Storage | Stats target | Description
+--------+---------+-----------+----------+---------+---------+--------------+-------------
+ c1 | integer | | not null | | plain | |
+Not-null constraints:
+ "notnull_tbl1_c1_not_null" NOT NULL "c1"
+
+DROP TABLE notnull_tbl1;
+-- with a REPLICA IDENTITY column
+CREATE DOMAIN notnull_dom1 AS INTEGER;
+CREATE TABLE notnull_tbl1 (c0 notnull_dom1, c1 int UNIQUE, PRIMARY KEY (c0, c1));
+ALTER TABLE notnull_tbl1 REPLICA IDENTITY USING INDEX notnull_tbl1_c1_key;
+DROP DOMAIN notnull_dom1 CASCADE;
+NOTICE: drop cascades to column c0 of table notnull_tbl1
+ALTER TABLE notnull_tbl1 ALTER c1 DROP NOT NULL;
+ERROR: column "c1" is in index used as replica identity
+DROP TABLE notnull_tbl1;
+-- with an identity generated column
+CREATE DOMAIN notnull_dom1 AS INTEGER;
+CREATE TABLE notnull_tbl1 (c0 notnull_dom1, c1 int generated by default as identity, PRIMARY KEY (c0, c1));
+DROP DOMAIN notnull_dom1 CASCADE;
+NOTICE: drop cascades to column c0 of table notnull_tbl1
+ALTER TABLE notnull_tbl1 ALTER c1 DROP NOT NULL;
+ERROR: column "c1" of relation "notnull_tbl1" is an identity column
DROP TABLE notnull_tbl1;
-- nope
CREATE TABLE notnull_tbl2 (a INTEGER CONSTRAINT blah NOT NULL, b INTEGER CONSTRAINT blah NOT NULL);
diff --git a/src/test/regress/sql/constraints.sql b/src/test/regress/sql/constraints.sql
index 87d685ae39..e946c29f18 100644
--- a/src/test/regress/sql/constraints.sql
+++ b/src/test/regress/sql/constraints.sql
@@ -599,6 +599,35 @@ ALTER TABLE notnull_tbl1 ADD CONSTRAINT foobar NOT NULL a;
select conname, contype, conkey from pg_constraint where conrelid = 'notnull_tbl1'::regclass;
DROP TABLE notnull_tbl1;
+-- make sure attnotnull is reset correctly when a PK is dropped indirectly
+CREATE TABLE notnull_tbl1 (c0 int, c1 int, PRIMARY KEY (c0, c1));
+ALTER TABLE notnull_tbl1 DROP c1;
+\d+ notnull_tbl1
+ALTER TABLE notnull_tbl1 ALTER c0 DROP NOT NULL;
+\d+ notnull_tbl1
+DROP TABLE notnull_tbl1;
+-- again
+CREATE DOMAIN notnull_dom1 AS INTEGER;
+CREATE TABLE notnull_tbl1 (c0 notnull_dom1, c1 int, PRIMARY KEY (c0, c1));
+DROP DOMAIN notnull_dom1 CASCADE;
+\d+ notnull_tbl1
+ALTER TABLE notnull_tbl1 ALTER c1 SET NOT NULL;
+\d+ notnull_tbl1
+DROP TABLE notnull_tbl1;
+-- with a REPLICA IDENTITY column
+CREATE DOMAIN notnull_dom1 AS INTEGER;
+CREATE TABLE notnull_tbl1 (c0 notnull_dom1, c1 int UNIQUE, PRIMARY KEY (c0, c1));
+ALTER TABLE notnull_tbl1 REPLICA IDENTITY USING INDEX notnull_tbl1_c1_key;
+DROP DOMAIN notnull_dom1 CASCADE;
+ALTER TABLE notnull_tbl1 ALTER c1 DROP NOT NULL;
+DROP TABLE notnull_tbl1;
+-- with an identity generated column
+CREATE DOMAIN notnull_dom1 AS INTEGER;
+CREATE TABLE notnull_tbl1 (c0 notnull_dom1, c1 int generated by default as identity, PRIMARY KEY (c0, c1));
+DROP DOMAIN notnull_dom1 CASCADE;
+ALTER TABLE notnull_tbl1 ALTER c1 DROP NOT NULL;
+DROP TABLE notnull_tbl1;
+
-- nope
CREATE TABLE notnull_tbl2 (a INTEGER CONSTRAINT blah NOT NULL, b INTEGER CONSTRAINT blah NOT NULL);
--
2.39.2
>From fe2c032c958b5855434232c9b699ef75a887e249 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Wed, 10 Apr 2024 19:10:52 +0200
Subject: [PATCH v2 2/2] support this in pg_dump
---
src/bin/pg_dump/pg_dump.c | 30 ++++++++++++++++++++++--------
1 file changed, 22 insertions(+), 8 deletions(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index c52e961b30..7dcbfaf8dc 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -8788,12 +8788,15 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
"), E',\n ') AS attfdwoptions,\n");
/*
- * Find out any NOT NULL markings for each column. In 17 and up we have
- * to read pg_constraint, and keep track whether it's NO INHERIT; in older
- * versions we rely on pg_attribute.attnotnull.
+ * Find out any NOT NULL markings for each column. In 17 and up we
+ * read pg_constraint to obtain the constraint name. notnull_noinherit
+ * is set according to the NO INHERIT property. For versions prior to 17,
+ * we store an empty string as the name when a constraint is marked as
+ * attnotnull (this cues dumpTableSchema to print the NOT NULL clause
+ * without a name); also, such cases are never NO INHERIT.
*
- * We also track whether the constraint was defined directly in this table
- * or via an ancestor, for binary upgrade.
+ * We track in notnull_inh whether the constraint was defined directly in
+ * this table or via an ancestor, for binary upgrade.
*
* Lastly, we need to know if the PK for the table involves each column;
* for columns that are there we need a NOT NULL marking even if there's
@@ -8801,13 +8804,24 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
* NULLs after the data is loaded when the PK is created, later in the
* dump; for this case we add throwaway constraints that are dropped once
* the PK is created.
+ *
+ * Another complication arises from columns that have attnotnull set, but
+ * for which no corresponding not-null nor PK constraint exists. This can
+ * happen if, for example, a primary key is dropped indirectly -- say,
+ * because one of its columns is dropped. This is an irregular condition,
+ * so we don't work hard to preserve it, and instead act as though an
+ * unnamed not-null constraint exists.
*/
if (fout->remoteVersion >= 170000)
appendPQExpBufferStr(q,
- "co.conname AS notnull_name,\n"
- "co.connoinherit AS notnull_noinherit,\n"
+ "CASE WHEN co.conname IS NOT NULL THEN co.conname "
+ " WHEN a.attnotnull AND copk.conname IS NULL THEN '' ELSE NULL END AS notnull_name,\n"
+ "CASE WHEN co.conname IS NOT NULL THEN co.connoinherit "
+ " WHEN a.attnotnull THEN false ELSE NULL END AS notnull_noinherit,\n"
"copk.conname IS NOT NULL as notnull_is_pk,\n"
- "coalesce(NOT co.conislocal, true) AS notnull_inh,\n");
+ "CASE WHEN co.conname IS NOT NULL THEN "
+ " coalesce(NOT co.conislocal, true) "
+ "ELSE false END as notnull_inh,\n");
else
appendPQExpBufferStr(q,
"CASE WHEN a.attnotnull THEN '' ELSE NULL END AS notnull_name,\n"
--
2.39.2
