Feedback-ID: ia2694551:Fastmail
Date: Thu, 7 Mar 2024 18:32:35 +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: <202403071732.6set44hjbzj3@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="3qs7zxxmfn4jzib6"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <202403071510.nhy6pdq66zuo@alvherre.pgsql>
Archived-At: 
 <https://www.postgresql.org/message-id/202403071732.6set44hjbzj3%40alvherre.pgsql>
Precedence: bulk


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

On 2024-Mar-07, Alvaro Herrera wrote:

> Maybe we can add a flag RelationData->rd_ispkdeferred, so that
> RelationGetPrimaryKeyIndex returned InvalidOid for deferrable PKs; then
> logical replication would continue to not know about this PK, which
> perhaps is what we want.  I'll do some testing with this.

This seems to work okay.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
“Cuando no hay humildad las personas se degradan” (A. Christie)

--3qs7zxxmfn4jzib6
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename="0001-Admit-deferrable-PKs-into-rd_pkindex-but-flag-them.patch"

From 8e3f913ca6a5bdbdbccac8d750ee9a6b6889f9aa Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Thu, 7 Mar 2024 16:44:35 +0100
Subject: [PATCH] Admit deferrable PKs into rd_pkindex, but flag them

... and don't return them as replica identity.
---
 src/backend/utils/cache/relcache.c            | 26 ++++++++++++++-----
 src/include/utils/rel.h                       |  1 +
 src/test/regress/expected/publication.out     | 10 +++++++
 .../regress/expected/replica_identity.out     |  5 ++++
 src/test/regress/sql/publication.sql          |  9 +++++++
 src/test/regress/sql/replica_identity.sql     |  4 +++
 6 files changed, 48 insertions(+), 7 deletions(-)

diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 4d339ee795..0460796cf5 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -4761,6 +4761,7 @@ RelationGetIndexList(Relation relation)
 	char		replident = relation->rd_rel->relreplident;
 	Oid			pkeyIndex = InvalidOid;
 	Oid			candidateIndex = InvalidOid;
+	bool		pkdeferred = false;
 	MemoryContext oldcxt;
 
 	/* Quick exit if we already computed the list. */
@@ -4802,12 +4803,12 @@ RelationGetIndexList(Relation relation)
 		result = lappend_oid(result, index->indexrelid);
 
 		/*
-		 * Non-unique, non-immediate or predicate indexes aren't interesting
-		 * for either oid indexes or replication identity indexes, so don't
-		 * check them.
+		 * Non-unique or predicate indexes aren't interesting for either oid
+		 * indexes or replication identity indexes, so don't check them.
+		 * Deferred ones are not useful for replication identity either; but
+		 * we do include them if they are PKs.
 		 */
 		if (!index->indisunique ||
-			!index->indimmediate ||
 			!heap_attisnull(htup, Anum_pg_index_indpred, NULL))
 			continue;
 
@@ -4832,7 +4833,13 @@ RelationGetIndexList(Relation relation)
 		if (index->indisprimary &&
 			(index->indisvalid ||
 			 relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE))
+		{
 			pkeyIndex = index->indexrelid;
+			pkdeferred = !index->indimmediate;
+		}
+
+		if (!index->indimmediate)
+			continue;
 
 		if (!index->indisvalid)
 			continue;
@@ -4854,7 +4861,8 @@ RelationGetIndexList(Relation relation)
 	oldlist = relation->rd_indexlist;
 	relation->rd_indexlist = list_copy(result);
 	relation->rd_pkindex = pkeyIndex;
-	if (replident == REPLICA_IDENTITY_DEFAULT && OidIsValid(pkeyIndex))
+	relation->rd_ispkdeferred = pkdeferred;
+	if (replident == REPLICA_IDENTITY_DEFAULT && OidIsValid(pkeyIndex) && !pkdeferred)
 		relation->rd_replidindex = pkeyIndex;
 	else if (replident == REPLICA_IDENTITY_INDEX && OidIsValid(candidateIndex))
 		relation->rd_replidindex = candidateIndex;
@@ -4957,7 +4965,8 @@ RelationGetStatExtList(Relation relation)
 /*
  * RelationGetPrimaryKeyIndex -- get OID of the relation's primary key index
  *
- * Returns InvalidOid if there is no such index.
+ * Returns InvalidOid if there is no such index, or if the primary key is
+ * DEFERRABLE.
  */
 Oid
 RelationGetPrimaryKeyIndex(Relation relation)
@@ -4972,7 +4981,7 @@ RelationGetPrimaryKeyIndex(Relation relation)
 		Assert(relation->rd_indexvalid);
 	}
 
-	return relation->rd_pkindex;
+	return relation->rd_ispkdeferred ? InvalidOid : relation->rd_pkindex;
 }
 
 /*
@@ -5198,6 +5207,9 @@ RelationGetIndexPredicate(Relation relation)
  * Attribute numbers are offset by FirstLowInvalidHeapAttributeNumber so that
  * we can include system attributes (e.g., OID) in the bitmap representation.
  *
+ * Deferred indexes are considered for the primary key, but not for replica
+ * identity.
+ *
  * Caller had better hold at least RowExclusiveLock on the target relation
  * to ensure it is safe (deadlock-free) for us to take locks on the relation's
  * indexes.  Note that since the introduction of CREATE INDEX CONCURRENTLY,
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index b3ea2b2042..51f876b5b5 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -63,6 +63,7 @@ typedef struct RelationData
 	bool		rd_isvalid;		/* relcache entry is valid */
 	bool		rd_indexvalid;	/* is rd_indexlist valid? (also rd_pkindex and
 								 * rd_replidindex) */
+	bool		rd_ispkdeferred;
 	bool		rd_statvalid;	/* is rd_statlist valid? */
 
 	/*----------
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index 16361a91f9..0c5521d2aa 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -732,6 +732,16 @@ ALTER PUBLICATION testpub_table_ins ADD TABLE testpub_tbl5 (a);		-- ok
 Tables:
     "public.testpub_tbl5" (a)
 
+-- error: cannot work with deferrable primary keys
+CREATE TABLE testpub_tbl5d (a int PRIMARY KEY DEFERRABLE);
+ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5d;
+UPDATE testpub_tbl5d SET a = 1;
+ERROR:  cannot update table "testpub_tbl5d" because it does not have a replica identity and publishes updates
+HINT:  To enable updating the table, set REPLICA IDENTITY using ALTER TABLE.
+/* but works fine with FULL replica identity */
+ALTER TABLE testpub_tbl5d REPLICA IDENTITY FULL;
+UPDATE testpub_tbl5d SET a = 1;
+DROP TABLE testpub_tbl5d;
 -- tests with REPLICA IDENTITY FULL
 CREATE TABLE testpub_tbl6 (a int, b text, c text);
 ALTER TABLE testpub_tbl6 REPLICA IDENTITY FULL;
diff --git a/src/test/regress/expected/replica_identity.out b/src/test/regress/expected/replica_identity.out
index 6038bf8e9f..cb3ef599d5 100644
--- a/src/test/regress/expected/replica_identity.out
+++ b/src/test/regress/expected/replica_identity.out
@@ -7,6 +7,7 @@ CREATE TABLE test_replica_identity (
        CONSTRAINT test_replica_identity_unique_nondefer UNIQUE (keya, keyb)
 ) ;
 CREATE TABLE test_replica_identity_othertable (id serial primary key);
+CREATE TABLE test_replica_identity_t3 (id serial constraint pk primary key deferrable);
 CREATE INDEX test_replica_identity_keyab ON test_replica_identity (keya, keyb);
 CREATE UNIQUE INDEX test_replica_identity_keyab_key ON test_replica_identity (keya, keyb);
 CREATE UNIQUE INDEX test_replica_identity_nonkey ON test_replica_identity (keya, nonkey);
@@ -57,6 +58,9 @@ ERROR:  "test_replica_identity_othertable_pkey" is not an index for table "test_
 -- fail, deferrable
 ALTER TABLE test_replica_identity REPLICA IDENTITY USING INDEX test_replica_identity_unique_defer;
 ERROR:  cannot use non-immediate index "test_replica_identity_unique_defer" as replica identity
+-- fail, deferrable
+ALTER TABLE test_replica_identity_t3 REPLICA IDENTITY USING INDEX pk;
+ERROR:  cannot use non-immediate index "pk" as replica identity
 SELECT relreplident FROM pg_class WHERE oid = 'test_replica_identity'::regclass;
  relreplident 
 --------------
@@ -292,3 +296,4 @@ DROP TABLE test_replica_identity3;
 DROP TABLE test_replica_identity4;
 DROP TABLE test_replica_identity5;
 DROP TABLE test_replica_identity_othertable;
+DROP TABLE test_replica_identity_t3;
diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql
index d5051a5e74..8ba8036bfb 100644
--- a/src/test/regress/sql/publication.sql
+++ b/src/test/regress/sql/publication.sql
@@ -444,6 +444,15 @@ RESET client_min_messages;
 ALTER PUBLICATION testpub_table_ins ADD TABLE testpub_tbl5 (a);		-- ok
 \dRp+ testpub_table_ins
 
+-- error: cannot work with deferrable primary keys
+CREATE TABLE testpub_tbl5d (a int PRIMARY KEY DEFERRABLE);
+ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5d;
+UPDATE testpub_tbl5d SET a = 1;
+/* but works fine with FULL replica identity */
+ALTER TABLE testpub_tbl5d REPLICA IDENTITY FULL;
+UPDATE testpub_tbl5d SET a = 1;
+DROP TABLE testpub_tbl5d;
+
 -- tests with REPLICA IDENTITY FULL
 CREATE TABLE testpub_tbl6 (a int, b text, c text);
 ALTER TABLE testpub_tbl6 REPLICA IDENTITY FULL;
diff --git a/src/test/regress/sql/replica_identity.sql b/src/test/regress/sql/replica_identity.sql
index dd43650586..30daec05b7 100644
--- a/src/test/regress/sql/replica_identity.sql
+++ b/src/test/regress/sql/replica_identity.sql
@@ -8,6 +8,7 @@ CREATE TABLE test_replica_identity (
 ) ;
 
 CREATE TABLE test_replica_identity_othertable (id serial primary key);
+CREATE TABLE test_replica_identity_t3 (id serial constraint pk primary key deferrable);
 
 CREATE INDEX test_replica_identity_keyab ON test_replica_identity (keya, keyb);
 CREATE UNIQUE INDEX test_replica_identity_keyab_key ON test_replica_identity (keya, keyb);
@@ -40,6 +41,8 @@ ALTER TABLE test_replica_identity REPLICA IDENTITY USING INDEX test_replica_iden
 ALTER TABLE test_replica_identity REPLICA IDENTITY USING INDEX test_replica_identity_othertable_pkey;
 -- fail, deferrable
 ALTER TABLE test_replica_identity REPLICA IDENTITY USING INDEX test_replica_identity_unique_defer;
+-- fail, deferrable
+ALTER TABLE test_replica_identity_t3 REPLICA IDENTITY USING INDEX pk;
 
 SELECT relreplident FROM pg_class WHERE oid = 'test_replica_identity'::regclass;
 
@@ -137,3 +140,4 @@ DROP TABLE test_replica_identity3;
 DROP TABLE test_replica_identity4;
 DROP TABLE test_replica_identity5;
 DROP TABLE test_replica_identity_othertable;
+DROP TABLE test_replica_identity_t3;
-- 
2.39.2


--3qs7zxxmfn4jzib6--