From c21c88114f9740d8f5db864e87a29d27bf436f3b Mon Sep 17 00:00:00 2001
From: Antonin Houska <ah@cybertec.at>
Date: Mon, 30 Jun 2025 19:41:43 +0200
Subject: [PATCH 3/7] 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 | 108 +++++++++++++++++++--------------
 1 file changed, 62 insertions(+), 46 deletions(-)

diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 5e94b570431..57ae5d561fd 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -69,6 +69,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,
+								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,
@@ -317,53 +319,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,
-											   CLUSTER_COMMAND_CLUSTER))
-		{
-			relation_close(OldHeap, AccessExclusiveLock);
+		if (!cluster_rel_recheck(OldHeap, indexOid, save_userid,
+								 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
@@ -465,6 +423,64 @@ 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,
+					int options)
+{
+	Oid			tableOid = RelationGetRelid(OldHeap);
+
+	/* Check that the user still has privileges for the relation */
+	if (!cluster_is_permitted_for_relation(tableOid, userid,
+										   CLUSTER_COMMAND_CLUSTER))
+	{
+		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.47.1