Subject: Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s).

From: Alvaro Herrera <[email-masked]>
Date: Tue, 5 Mar 2024 13:36:11 +0100

On 2024-Mar-04, Dean Rasheed wrote:

> I don't think that this is the right fix. ISTM that the real issue is
> that dropping a NOT NULL constraint should not mark the column as
> nullable if it is part of a PK, whether or not that PK is deferrable
> -- a deferrable PK still marks a  column as not nullable.

Yeah.  As I said upthread, a good fix seems to require no longer relying
on RelationGetIndexAttrBitmap to obtain the columns in the primary key,
because that function does not include deferred primary keys.  I came up
with the attached POC, which seems to fix the reported problem, but of
course it needs more polish, a working test case, and verifying whether
the new function should be used in more places -- in particular, whether
it can be used to revert the changes to RelationGetIndexList that
b0e96f311985 did.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"La persona que no quería pecar / estaba obligada a sentarse
 en duras y empinadas sillas    / desprovistas, por cierto
 de blandos atenuantes"                          (Patricio Vogel)
>From cc7032c3f62949d20bc98d4b3c70cb2cb8a0dd5e Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <[email-masked]>
Date: Tue, 5 Mar 2024 13:32:50 +0100
Subject: [PATCH] Don't lose attnotnull if a deferred PK supports it

When dropping a NOT NULL constraint on a column that has a deferred
primary key, we would reset attnotnull, but that's bogus.

XXX this patch is nowhere near final.

Reported-by: Amul Sul <[email-masked]>
Discussion: https://postgr.es/m/[email-masked]
---
 src/backend/commands/tablecmds.c | 71 ++++++++++++++++++++++++++++++--
 1 file changed, 68 insertions(+), 3 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index c61f9305c2..84c7871dda 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -12704,6 +12704,73 @@ ATExecDropConstraint(Relation rel, const char *constrName,
 	table_close(conrel, RowExclusiveLock);
 }
 
+/*
+ * dropconstraint_getpkcols -- subroutine for dropconstraint_internal
+ *
+ * This is a workaround to the fact that RelationGetIndexAttrBitmap does
+ * not consider a DEFERRABLE PRIMARY KEY to be a real primary key.  Maybe
+ * this code should be elsewhere.
+ */
+static Bitmapset *
+dropconstraint_getpkcols(Relation rel)
+{
+	Relation	indrel;
+	SysScanDesc	indscan;
+	ScanKeyData	skey;
+	HeapTuple	htup;
+	Bitmapset  *b = NULL;
+
+	/*
+	 * We try first to obtain a list of PK columns from the cache; if there
+	 * is one, we're done.
+	 */
+	b = RelationGetIndexAttrBitmap(rel, INDEX_ATTR_BITMAP_PRIMARY_KEY);
+	if (b != NULL)
+		return b;
+
+	/* Prepare to scan pg_index for entries having indrelid = this rel. */
+	ScanKeyInit(&skey,
+				Anum_pg_index_indrelid,
+				BTEqualStrategyNumber, F_OIDEQ,
+				ObjectIdGetDatum(RelationGetRelid(rel)));
+
+	indrel = table_open(IndexRelationId, AccessShareLock);
+	indscan = systable_beginscan(indrel, IndexIndrelidIndexId, true,
+								 NULL, 1, &skey);
+
+	while (HeapTupleIsValid(htup = systable_getnext(indscan)))
+	{
+		Form_pg_index index = (Form_pg_index) GETSTRUCT(htup);
+
+		/*
+		 * Ignore any indexes that are currently being dropped and those that
+		 * aren't a primary key.
+		 */
+		if (!index->indislive)
+			continue;
+		if (!index->indisprimary)
+			continue;
+
+		/*
+		 * If this primary key was IMMEDIATE, it would have been returned
+		 * by RelationGetIndexAttrBitmap.
+		 */
+		Assert(!index->indimmediate);
+
+		for (int i = 0; i < index->indnatts; i++)
+		{
+			int			attrnum = index->indkey.values[i];
+
+			b = bms_add_member(b, attrnum - FirstLowInvalidHeapAttributeNumber);
+		}
+	}
+
+	systable_endscan(indscan);
+	table_close(indrel, AccessShareLock);
+
+	return b;
+}
+
 /*
  * Remove a constraint, using its pg_constraint tuple
  *
@@ -12837,9 +12904,7 @@ dropconstraint_internal(Relation rel, HeapTuple constraintTup, DropBehavior beha
 		 * We want to test columns for their presence in the primary key, but
 		 * only if we're not dropping it.
 		 */
-		pkcols = dropping_pk ? NULL :
-			RelationGetIndexAttrBitmap(rel,
-									   INDEX_ATTR_BITMAP_PRIMARY_KEY);
+		pkcols = dropping_pk ? NULL : dropconstraint_getpkcols(rel);
 		ircols = RelationGetIndexAttrBitmap(rel, INDEX_ATTR_BITMAP_IDENTITY_KEY);
 
 		foreach(lc, unconstrained_cols)
-- 
2.39.2

---

From: Dean Rasheed <[email-masked]>
Date: Tue, 5 Mar 2024 14:58:10 +0000

> it can be used to revert the changes to RelationGetIndexList that

Looking at the other places that call RelationGetIndexAttrBitmap()
with INDEX_ATTR_BITMAP_PRIMARY_KEY, they all appear to want to include
deferrable PKs, since they are relying on the result to see which
columns are not nullable.

So there are other bugs here. For example:

CREATE TABLE foo (id int PRIMARY KEY DEFERRABLE, val text);
CREATE TABLE bar (LIKE foo);

now fails to mark bar.id as not nullable, whereas prior to
b0e96f311985 it would have been.

So I think RelationGetIndexAttrBitmap() should include deferrable PKs,
but not all the changes made to RelationGetIndexList() by b0e96f311985
need reverting.

Regards,
Dean