v45-0001-refactor-ATExecMergePartitions-transformPartitionCmdF.no-cfbot

From 97f4372b6c1f4eb282b7a3e09e860abe9a27cae1 Mon Sep 17 00:00:00 2001
From: jian he <jian.universality@gmail.com>
Date: Sun, 15 Jun 2025 22:24:26 +0800
Subject: [PATCH v45 1/2] refactor ATExecMergePartitions,
 transformPartitionCmdForMerge
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

For the command:
ALTER TABLE pk MERGE PARTITIONS (pk_1, pk_2) INTO pk_1;

I believe we should acquire AccessExclusiveLock on the partitions to be merged
(pk_1, pk_2) during transformPartitionCmdForMerge. Here’s why:

* The merged partitions (pk_1, pk_2) will be dropped in the end, so acquiring
AccessExclusiveLock is unavoidable for ALTER TABLE MERGE PARTITIONS.
* Taking an AccessShareLock first, and then later acquiring AccessExclusiveLock in
ATExecMergePartitions unnecessarily waste resources.
* Acquiring AccessExclusiveLock first helps avoid potential anomalies caused by concurrent operations.

+ partOid = RangeVarGetRelid(name, AccessShareLock, false);
if current user don't owne the table (name), then he can not lock the table.
thus the above code in transformPartitionCmdForMerge need refactor
---
 src/backend/commands/tablecmds.c   | 74 +++++++++++++++++-------------
 src/backend/parser/parse_utilcmd.c | 18 ++++++--
 2 files changed, 56 insertions(+), 36 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7e7703c7997..e3bbfd9283b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -22543,13 +22543,13 @@ createPartitionTable(List **wqueue, RangeVar *newPartName,
 }
 
 /*
- * moveMergedTablesRows: scan partitions to be merged (mergingPartitionsList)
+ * moveMergedTablesRows: scan partitions to be merged (mergingPartitions)
  * of the partitioned table (rel) and move rows into the new partition
  * (newPartRel). We also reevaulate check constraints against these rows.
  */
 static void
 moveMergedTablesRows(List **wqueue, Relation rel,
-					 List *mergingPartitionsList, Relation newPartRel)
+					 List *mergingPartitions, Relation newPartRel)
 {
 	CommandId	mycid;
 	EState	   *estate;
@@ -22580,12 +22580,15 @@ moveMergedTablesRows(List **wqueue, Relation rel,
 	/* Create necessary tuple slot. */
 	dstslot = table_slot_create(newPartRel, NULL);
 
-	foreach_ptr(RelationData, mergingPartition, mergingPartitionsList)
+	foreach_oid(merging_oid, mergingPartitions)
 	{
 		TupleTableSlot *srcslot;
 		TupleConversionMap *tuple_map;
 		TableScanDesc scan;
 		Snapshot	snapshot;
+		Relation 	mergingPartition;
+
+		mergingPartition = table_open(merging_oid, NoLock);
 
 		/* Create tuple slot for new partition. */
 		srcslot = table_slot_create(mergingPartition, NULL);
@@ -22659,6 +22662,7 @@ moveMergedTablesRows(List **wqueue, Relation rel,
 			free_conversion_map(tuple_map);
 
 		ExecDropSingleTupleTableSlot(srcslot);
+		table_close(mergingPartition, NoLock);
 	}
 
 	FreeExecutorState(estate);
@@ -22709,7 +22713,7 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo *tab, Relation rel,
 					  PartitionCmd *cmd, AlterTableUtilityContext *context)
 {
 	Relation	newPartRel;
-	List	   *mergingPartitionsList = NIL;
+	List	   *mergingPartitions = NIL;
 	Oid			defaultPartOid;
 	Oid			existingRelid;
 	Oid			ownerId = InvalidOid;
@@ -22718,18 +22722,21 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo *tab, Relation rel,
 	int			save_nestlevel;
 
 	/*
-	 * Lock all merged partitions, check them and create list with partitions
-	 * contexts.
-	 */
+	 * Check ownership of merged partitions — partitions with different owners
+	 * cannot be merged. Also, collect the OIDs of these partitions during the
+	 * check.
+	*/
 	foreach_node(RangeVar, name, cmd->partlist)
 	{
 		Relation	mergingPartition;
 
 		/*
-		 * We are going to detach and remove this partition: need to use
-		 * exclusive lock for preventing DML-queries to the partition.
+		 * We are going to detach and remove this partition.  We already took
+		 * AccessExclusiveLock lock on transformPartitionCmdForMerge, so here,
+		 * NoLock is fine.
 		 */
-		mergingPartition = table_openrv(name, AccessExclusiveLock);
+		mergingPartition = table_openrv_extended(name, NoLock, false);
+		Assert(CheckRelationLockedByMe(mergingPartition, AccessExclusiveLock, false));
 
 		if (OidIsValid(ownerId))
 		{
@@ -22743,8 +22750,10 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo *tab, Relation rel,
 			ownerId = mergingPartition->rd_rel->relowner;
 
 		/* Store a next merging partition into the list. */
-		mergingPartitionsList = lappend(mergingPartitionsList,
-										mergingPartition);
+		mergingPartitions = lappend_oid(mergingPartitions,
+										RelationGetRelid(mergingPartition));
+
+		table_close(mergingPartition, NoLock);
 	}
 
 	/*
@@ -22764,18 +22773,18 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo *tab, Relation rel,
 	 */
 	if (OidIsValid(existingRelid))
 	{
-		Relation	sameNamePartition = NULL;
+		Oid		new_partition = InvalidOid;
 
-		foreach_ptr(RelationData, mergingPartition, mergingPartitionsList)
+		foreach_oid(mergingPartition, mergingPartitions)
 		{
-			if (RelationGetRelid(mergingPartition) == existingRelid)
+			if (mergingPartition == existingRelid)
 			{
-				sameNamePartition = mergingPartition;
+				new_partition = mergingPartition;
 				break;
 			}
 		}
 
-		if (sameNamePartition)
+		if (OidIsValid(new_partition))
 		{
 			/*
 			 * The new partition has the same name as one of merging
@@ -22792,8 +22801,7 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo *tab, Relation rel,
 			 * in the future because we're going to eventually drop the
 			 * existing partition anyway.
 			 */
-			RenameRelationInternal(RelationGetRelid(sameNamePartition),
-								   tmpRelName, true, false);
+			RenameRelationInternal(new_partition, tmpRelName, true, false);
 
 			/*
 			 * We must bump the command counter to make the new partition
@@ -22822,14 +22830,23 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo *tab, Relation rel,
 	 * merge process (moveMergedTablesRows) can be time-consuming, performing an
 	 * early check on the drop eligibility of old partitions is preferable.
 	 */
-	foreach_ptr(RelationData, mergingPartition, mergingPartitionsList)
+	foreach_oid(mergingPartition, mergingPartitions)
+	{
+		Relation	child_rel;
+
+		child_rel = table_open(mergingPartition, NoLock);
+
+		detachPartitionTable(rel, child_rel, defaultPartOid);
+
+		table_close(child_rel, NoLock);
+	}
+
+	foreach_oid(mergingPartition, mergingPartitions)
 	{
 		ObjectAddress object;
 
-		detachPartitionTable(rel, mergingPartition, defaultPartOid);
-
 		/* Get oid of the later to be dropped relation */
-		object.objectId = RelationGetRelid(mergingPartition);
+		object.objectId = mergingPartition;
 		object.classId = RelationRelationId;
 		object.objectSubId = 0;
 
@@ -22854,24 +22871,19 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo *tab, Relation rel,
 	RestrictSearchPath();
 
 	/* Copy data from merged partitions to new partition. */
-	moveMergedTablesRows(wqueue, rel, mergingPartitionsList, newPartRel);
+	moveMergedTablesRows(wqueue, rel, mergingPartitions, newPartRel);
 
 	/* Drop the current partitions before attaching the new one. */
-	foreach_ptr(RelationData, mergingPartition, mergingPartitionsList)
+	foreach_oid(mergingPartition, mergingPartitions)
 	{
 		ObjectAddress object;
 
-		/* Get relation id before table_close() call. */
-		object.objectId = RelationGetRelid(mergingPartition);
+		object.objectId = mergingPartition;
 		object.classId = RelationRelationId;
 		object.objectSubId = 0;
 
-		/* Keep the lock until commit. */
-		table_close(mergingPartition, NoLock);
-
 		performDeletion(&object, DROP_RESTRICT, 0);
 	}
-	list_free(mergingPartitionsList);
 
 	/*
 	 * Attach a new partition to the partitioned table. wqueue = NULL:
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index da291986446..eb8a937e1e8 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -3500,7 +3500,7 @@ checkPartition(Relation rel, Oid partRelOid)
 {
 	Relation	partRel;
 
-	partRel = relation_open(partRelOid, AccessShareLock);
+	partRel = table_open(partRelOid, NoLock);
 
 	if (partRel->rd_rel->relkind != RELKIND_RELATION)
 		ereport(ERROR,
@@ -3526,7 +3526,7 @@ checkPartition(Relation rel, Oid partRelOid)
 		aclcheck_error(ACLCHECK_NOT_OWNER, get_relkind_objtype(partRel->rd_rel->relkind),
 					   RelationGetRelationName(partRel));
 
-	relation_close(partRel, AccessShareLock);
+	table_close(partRel, NoLock);
 }
 
 /*
@@ -3577,10 +3577,18 @@ transformPartitionCmdForMerge(CreateStmtContext *cxt, PartitionCmd *partcmd)
 		}
 
 		/*
-		 * Search DEFAULT partition in the list. Lock partitions before
-		 * calculating the boundary for resulting partition.
+		 * Search DEFAULT partition in the list. Open and lock partitions before
+		 * calculating the boundary for resulting partition, we also check for
+		 * ownership along the way.  We need to use AccessExclusiveLock here,
+		 * because these merged partitions will be detached then dropped in
+		 * ATExecMergePartitions.
 		 */
-		partOid = RangeVarGetRelid(name, AccessShareLock, false);
+		partOid = RangeVarGetRelidExtended(name,
+										   AccessExclusiveLock,
+										   false,
+										   RangeVarCallbackOwnsRelation,
+										   NULL);
+
 		if (partOid == defaultPartOid)
 			isDefaultPart = true;
 
-- 
2.34.1