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)
>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] 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
