From 43a540cff157e2e6246d0b27565fc761f7d0e5da Mon Sep 17 00:00:00 2001
From: Naisila Puka <37271756+naisila@users.noreply.github.com>
Date: Thu, 1 Sep 2022 11:56:31 +0300
Subject: [PATCH] Use RelationGetPrimaryKeyIndex for citus catalog tables
 (#6262)

pg_dist_node and pg_dist_colocation have a primary key index, not a replica identity index.

Citus catalog tables are created in public schema, which has replica identity index by default
as primary key index. Later the citus catalog tables are moved to pg_catalog schema.

During pg_upgrade, all tables are recreated, and given that pg_dist_colocation is found in
pg_catalog schema, it is recreated in that schema, and when it is recreated it doesn't
have a replica identity index, because catalog tables have no replica identity.

Further action:
Do we even need to acquire this lock on the primary key index?
Postgres doesn't acquire such locks on indexes before deleting catalog tuples.
Also, catalog tuples don't have replica identities by definition.
---
 src/backend/distributed/metadata/node_metadata.c | 11 +++++++++--
 src/backend/distributed/utils/colocation_utils.c | 11 +++++++++--
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/src/backend/distributed/metadata/node_metadata.c b/src/backend/distributed/metadata/node_metadata.c
index 5abc62e43..cb1b711b5 100644
--- a/src/backend/distributed/metadata/node_metadata.c
+++ b/src/backend/distributed/metadata/node_metadata.c
@@ -1910,9 +1910,16 @@ DeleteNodeRow(char *nodeName, int32 nodePort)
 
 	/*
 	 * simple_heap_delete() expects that the caller has at least an
-	 * AccessShareLock on replica identity index.
+	 * AccessShareLock on primary key index.
+	 *
+	 * XXX: This does not seem required, do we really need to acquire this lock?
+	 * Postgres doesn't acquire such locks on indexes before deleting catalog tuples.
+	 * Linking here the reasons we added this lock acquirement:
+	 * https://github.com/citusdata/citus/pull/2851#discussion_r306569462
+	 * https://github.com/citusdata/citus/pull/2855#discussion_r313628554
+	 * https://github.com/citusdata/citus/issues/1890
 	 */
-	Relation replicaIndex = index_open(RelationGetReplicaIndex(pgDistNode),
+	Relation replicaIndex = index_open(RelationGetPrimaryKeyIndex(pgDistNode),
 									   AccessShareLock);
 
 	ScanKeyInit(&scanKey[0], Anum_pg_dist_node_nodename,
diff --git a/src/backend/distributed/utils/colocation_utils.c b/src/backend/distributed/utils/colocation_utils.c
index 979f24b8f..14ce64dae 100644
--- a/src/backend/distributed/utils/colocation_utils.c
+++ b/src/backend/distributed/utils/colocation_utils.c
@@ -1193,10 +1193,17 @@ DeleteColocationGroup(uint32 colocationId)
 	{
 		/*
 		 * simple_heap_delete() expects that the caller has at least an
-		 * AccessShareLock on replica identity index.
+		 * AccessShareLock on primary key index.
+		 *
+		 * XXX: This does not seem required, do we really need to acquire this lock?
+		 * Postgres doesn't acquire such locks on indexes before deleting catalog tuples.
+		 * Linking here the reasons we added this lock acquirement:
+		 * https://github.com/citusdata/citus/pull/2851#discussion_r306569462
+		 * https://github.com/citusdata/citus/pull/2855#discussion_r313628554
+		 * https://github.com/citusdata/citus/issues/1890
 		 */
 		Relation replicaIndex =
-			index_open(RelationGetReplicaIndex(pgDistColocation),
+			index_open(RelationGetPrimaryKeyIndex(pgDistColocation),
 					   AccessShareLock);
 		simple_heap_delete(pgDistColocation, &(heapTuple->t_self));