Feedback-ID: ia2694551:Fastmail
Date: Wed, 10 Apr 2024 15:58:49 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Tender Wang <tndrwang@gmail.com>
Cc: jian he <jian.universality@gmail.com>, 
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Can't find not null constraint, but \d+ shows that
Message-ID: <202404101358.iet3euazs5zf@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="ujvasav73b4rary6"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <202404101054.c2qhyi62by5n@alvherre.pgsql>
Archived-At: <https://www.postgresql.org/message-id/202404101358.iet3euazs5zf%40alvherre.pgsql>
Precedence: bulk


--ujvasav73b4rary6
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

It turns out that trying to close all holes that lead to columns marked
not-null without a pg_constraint row is not possible within the ALTER
TABLE framework, because it can happen outside it also.  Consider this

CREATE DOMAIN dom1 AS integer;
CREATE TABLE notnull_tbl (a dom1, b int, PRIMARY KEY (a, b));
DROP DOMAIN dom1 CASCADE;

In this case you'll end up with b having attnotnull=true and no
constraint; and no amount of messing with tablecmds.c will fix it.

So I propose to instead allow those constraints, and treat them as
second-class citizens.  We allow dropping them with ALTER TABLE DROP NOT
NULL, and we allow to create a backing full-fledged constraint with SET
NOT NULL or ADD CONSTRAINT.  So here's a partial crude initial patch to
do that.


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.

Another thing I wonder if whether I should use the existing
set_attnotnull() instead of adding drop_orphaned_notnull().  Or we could
just inline the code in ATExecDropNotNull, since it's small and
self-contained.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Postgres is bloatware by design: it was built to house
 PhD theses." (Joey Hellerstein, SIGMOD annual conference 2002)

--ujvasav73b4rary6
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename="0001-Handle-ALTER-.-DROP-NOT-NULL-when-no-pg_constraint-r.patch"

From 3a00358847f1792c01e608909dc8a4a0edb8739c Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Wed, 10 Apr 2024 13:02:35 +0200
Subject: [PATCH] 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


--ujvasav73b4rary6--