Subject: Re: Can't find not null constraint, but \d+ shows that

From: Alvaro Herrera <[email-masked]>
Date: Wed, 10 Apr 2024 19:23:25 +0200

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 <[email-masked]>
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 <[email-masked]>
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

---

From: jian he <[email-masked]>
Date: Thu, 11 Apr 2024 14:40:04 +0800

On Wed, Apr 10, 2024 at 2:10 PM jian he <[email-masked]> wrote:
>
> DROP TABLE if exists notnull_tbl2;
> CREATE TABLE notnull_tbl2 (c0 int generated by default as IDENTITY, c1 int);
> ALTER TABLE notnull_tbl2 ADD CONSTRAINT Q PRIMARY KEY(c0, c1);
> ALTER TABLE notnull_tbl2 DROP CONSTRAINT notnull_tbl2_c0_not_null;
> ALTER TABLE notnull_tbl2 DROP c1;
> \d notnull_tbl2

> ALTER TABLE notnull_tbl2 DROP CONSTRAINT notnull_tbl2_c0_not_null;
per above sequence execution order, this should error out?

otherwise which "not null" (attribute|constraint) to anchor "generated
by default as identity" not null property?
"DROP c1" will drop the not null property for "c0" and "c1".
if "DROP CONSTRAINT notnull_tbl2_c0_not_nul" not error out, then
" ALTER TABLE notnull_tbl2 DROP c1;"
should either error out
or transform "c0" from "c0 int generated by default as identity"
to
"c0 int"

+ Column |  Type   | Collation | Nullable | Default | Storage | Stats
target | Description

this is not what we expected?
"not null" for "c0" now should be false?
am I missing something?

---

From: Tender Wang <[email-masked]>
Date: Thu, 11 Apr 2024 15:19:35 +0800

jian he <[email-masked]> 于2024年4月11日周四 14:40写道：

> I try above case on MASTER and MASTER with Alvaro V2 patch, and all work
correctly.
\d+ notnull_tbl2 will see not-null of "c0".

> + Column |  Type   | Collation | Nullable | Default | Storage | Stats
Yeah, now this is expected behavior.
Users can  drop manually not-null of "c0" if they want, and no error
reporte.

-- 
Tender Wang
OpenPie:  https://en.openpie.com/

---

From: jian he <[email-masked]>
Date: Thu, 11 Apr 2024 16:49:35 +0800

On Thu, Apr 11, 2024 at 3:19 PM Tender Wang <[email-masked]> wrote:
> Users can  drop manually not-null of "c0" if they want, and no error reporte.
>

sorry for the noise.
these two past patches confused me:
0001-Correctly-reset-attnotnull-when-constraints-dropped-.patch
v4-0001-Fix-pg_attribute-attnotnull-not-reset-when-droppe.patch

I thought dropping a column of primary key (ALTER TABLE notnull_tbl2 DROP c1)
will make the others key columns to not have "not null" property.

now I figured out that
dropping a column of primary key columns will not change other key
columns'  "not null" property.
dropping the primary key associated constraint will make all key
columns "not null" property disappear.

v2-0001-Handle-ALTER-.-DROP-NOT-NULL-when-no-pg_constrain.patch
behavior looks fine to me now.

inline drop_orphaned_notnull in ATExecDropNotNull looks fine to me.