From 317dda6af1e8b6c7bfa1f02f22436a146cd72df3 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 3f729eccf..f3c61b64c 100644
--- a/src/backend/distributed/metadata/node_metadata.c
+++ b/src/backend/distributed/metadata/node_metadata.c
@@ -2677,9 +2677,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 bb9488af1..a5a32db0e 100644
--- a/src/backend/distributed/utils/colocation_utils.c
+++ b/src/backend/distributed/utils/colocation_utils.c
@@ -1303,10 +1303,17 @@ DeleteColocationGroupLocally(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));