Feedback-ID: ia2694551:Fastmail
Date: Tue, 5 Mar 2024 13:36:11 +0100
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Dean Rasheed <dean.a.rasheed@gmail.com>
Cc: Aleksander Alekseev <aleksander@timescale.com>, 
	PostgreSQL-development <pgsql-hackers@postgresql.org>, Amul Sul <sulamul@gmail.com>, vignesh C <vignesh21@gmail.com>
Subject: Re: Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY
 column(s).
Message-ID: <202403051236.to67sdlvgxk6@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="qek6jpjdi3ls6ksm"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAEZATCWZfdZVnsiJdCSZfMNsOSrTtEZ2gjiZ-HQoNc2FMYxGhA@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/202403051236.to67sdlvgxk6%40alvherre.pgsql>
Precedence: bulk


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

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)

--qek6jpjdi3ls6ksm
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename="0001-Don-t-lose-attnotnull-if-a-deferred-PK-supports-it.patch"

From cc7032c3f62949d20bc98d4b3c70cb2cb8a0dd5e Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
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 <sulamul@gmail.com>
Discussion: https://postgr.es/m/CAAJ_b94QonkgsbDXofakHDnORQNgafd1y3Oa5QXfpQNJyXyQ7A@mail.gmail.com
---
 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


--qek6jpjdi3ls6ksm--