From 8584b47b926b8ba4f917db9af5337486228c81a1 Mon Sep 17 00:00:00 2001
From: Antonin Houska <ah@cybertec.at>
Date: Tue, 1 Apr 2025 13:48:57 +0200
Subject: [PATCH 3/9] Move the "recheck" branch to a separate function.

At some point I thought that the relation must be unlocked during the call of
setup_logical_decoding(), to avoid a deadlock. In that case we'd need to
recheck afterwards if the table still meets the requirements of cluster_rel().

Eventually I concluded that the risk of that deadlock is not that high, so the
table stays locked during the call of setup_logical_decoding(). Therefore the
rechecking code is only executed once per table. Anyway, this patch might be useful in terms of code readability.
---
 src/backend/commands/cluster.c | 106 +++++++++++++++++++--------------
 1 file changed, 61 insertions(+), 45 deletions(-)

diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 9ae3d87e412..67625d52f12 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -78,6 +78,8 @@ typedef struct
 
 static void cluster_multiple_rels(List *rtcs, ClusterParams *params,
 								  ClusterCommand cmd);
+static bool cluster_rel_recheck(Relation OldHeap, Oid indexOid, Oid userid,
+								ClusterCommand cmd, int options);
 static void rebuild_relation(Relation OldHeap, Relation index, bool verbose,
 							 ClusterCommand cmd);
 static void copy_table_data(Relation NewHeap, Relation OldHeap, Relation OldIndex,
@@ -329,52 +331,9 @@ cluster_rel(Relation OldHeap, Oid indexOid, ClusterParams *params,
 	 * to cluster a not-previously-clustered index.
 	 */
 	if (recheck)
-	{
-		/* Check that the user still has privileges for the relation */
-		if (!cluster_is_permitted_for_relation(tableOid, save_userid, cmd))
-		{
-			relation_close(OldHeap, AccessExclusiveLock);
+		if (!cluster_rel_recheck(OldHeap, indexOid, save_userid, cmd,
+								 params->options))
 			goto out;
-		}
-
-		/*
-		 * Silently skip a temp table for a remote session.  Only doing this
-		 * check in the "recheck" case is appropriate (which currently means
-		 * somebody is executing a database-wide CLUSTER or on a partitioned
-		 * table), because there is another check in cluster() which will stop
-		 * any attempt to cluster remote temp tables by name.  There is
-		 * another check in cluster_rel which is redundant, but we leave it
-		 * for extra safety.
-		 */
-		if (RELATION_IS_OTHER_TEMP(OldHeap))
-		{
-			relation_close(OldHeap, AccessExclusiveLock);
-			goto out;
-		}
-
-		if (OidIsValid(indexOid))
-		{
-			/*
-			 * Check that the index still exists
-			 */
-			if (!SearchSysCacheExists1(RELOID, ObjectIdGetDatum(indexOid)))
-			{
-				relation_close(OldHeap, AccessExclusiveLock);
-				goto out;
-			}
-
-			/*
-			 * Check that the index is still the one with indisclustered set,
-			 * if needed.
-			 */
-			if ((params->options & CLUOPT_RECHECK_ISCLUSTERED) != 0 &&
-				!get_index_isclustered(indexOid))
-			{
-				relation_close(OldHeap, AccessExclusiveLock);
-				goto out;
-			}
-		}
-	}
 
 	/*
 	 * We allow VACUUM FULL, but not CLUSTER, on shared catalogs.  CLUSTER
@@ -459,6 +418,63 @@ out:
 	pgstat_progress_end_command();
 }
 
+/*
+ * Check if the table (and its index) still meets the requirements of
+ * cluster_rel().
+ */
+static bool
+cluster_rel_recheck(Relation OldHeap, Oid indexOid, Oid userid,
+					ClusterCommand cmd, int options)
+{
+	Oid			tableOid = RelationGetRelid(OldHeap);
+
+	/* Check that the user still has privileges for the relation */
+	if (!cluster_is_permitted_for_relation(tableOid, userid, cmd))
+	{
+		relation_close(OldHeap, AccessExclusiveLock);
+		return false;
+	}
+
+	/*
+	 * Silently skip a temp table for a remote session.  Only doing this check
+	 * in the "recheck" case is appropriate (which currently means somebody is
+	 * executing a database-wide CLUSTER or on a partitioned table), because
+	 * there is another check in cluster() which will stop any attempt to
+	 * cluster remote temp tables by name.  There is another check in
+	 * cluster_rel which is redundant, but we leave it for extra safety.
+	 */
+	if (RELATION_IS_OTHER_TEMP(OldHeap))
+	{
+		relation_close(OldHeap, AccessExclusiveLock);
+		return false;
+	}
+
+	if (OidIsValid(indexOid))
+	{
+		/*
+		 * Check that the index still exists
+		 */
+		if (!SearchSysCacheExists1(RELOID, ObjectIdGetDatum(indexOid)))
+		{
+			relation_close(OldHeap, AccessExclusiveLock);
+			return false;
+		}
+
+		/*
+		 * Check that the index is still the one with indisclustered set, if
+		 * needed.
+		 */
+		if ((options & CLUOPT_RECHECK_ISCLUSTERED) != 0 &&
+			!get_index_isclustered(indexOid))
+		{
+			relation_close(OldHeap, AccessExclusiveLock);
+			return false;
+		}
+	}
+
+	return true;
+}
+
 /*
  * Verify that the specified heap and index are valid to cluster on
  *
-- 
2.43.5