From 97f4372b6c1f4eb282b7a3e09e860abe9a27cae1 Mon Sep 17 00:00:00 2001 From: jian he 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