From 89bbc474d96b980c215483e88f50f2365c460580 Mon Sep 17 00:00:00 2001 From: "Chao Li (Evan)" Date: Mon, 26 Oct 2026 15:03:59 +0800 Subject: [PATCH v5 2/3] Prevent deadlocks during partitioned index attachment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ALTER INDEX ... ATTACH PARTITION can validate its target index and then walk up the index hierarchy to validate its ancestors. Locking ancestors after descendants can deadlock with recursive ALTER TABLE or another index ATTACH that locks the hierarchy from the top down. Extend RangeVarCallbackForAttachPartition() to pre-lock ancestor indexes and their owning tables from root to parent before locking the target index. At each level, lock the table before its index, and also lock the target index's table before the target index. Follow the index hierarchy, which need not extend to the root of the table hierarchy. Use AccessExclusiveLock for ancestors, matching validatePartitionedIndex(), to avoid deadlocks from later lock upgrades. Track the lock mode and the target's owning table so obsolete locks can be released when relation lookup resolves to a different target. Reported-by: Álvaro Herrera Author: Chao Li Discussion: https://postgr.es/m/CFACA0EB-7E6F-4FAA-9ACE-1FC2226D7482@gmail.com --- src/backend/commands/tablecmds.c | 57 +++++++++++++++++++++++++++++--- 1 file changed, 52 insertions(+), 5 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index adcda53aff8..11df4451cee 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -335,6 +335,9 @@ struct AttachPartitionCallbackState { Node *stmt; List *lockedAncestors; + LOCKMODE ancestorLockmode; + /* Owning table of the target index; used only for index ATTACH */ + Oid lockedTable; }; /* Alter table target-type flags for ATSimplePermissions */ @@ -20488,16 +20491,22 @@ RangeVarCallbackForAttachPartition(const RangeVar *rv, Oid relid, Oid oldrelid, void *arg) { struct AttachPartitionCallbackState *state = arg; + char relkind; /* Release locks belonging to a previous lookup of a different relation. */ - if (relid != oldrelid && state->lockedAncestors != NIL) + if (relid != oldrelid) { ListCell *lc; foreach(lc, state->lockedAncestors) - UnlockRelationOid(lfirst_oid(lc), AccessShareLock); + UnlockRelationOid(lfirst_oid(lc), state->ancestorLockmode); list_free(state->lockedAncestors); state->lockedAncestors = NIL; + if (OidIsValid(state->lockedTable)) + { + UnlockRelationOid(state->lockedTable, AccessShareLock); + state->lockedTable = InvalidOid; + } } RangeVarCallbackForAlterRelation(rv, relid, oldrelid, state->stmt); @@ -20505,10 +20514,11 @@ RangeVarCallbackForAttachPartition(const RangeVar *rv, Oid relid, Oid oldrelid, if (relid == oldrelid || !OidIsValid(relid)) return; + relkind = get_rel_relkind(relid); + /* * Lock ancestors from root to parent before ATTACH locks the target and - * walks up the tree to collect partition constraints. If the target is - * not a partitioned table, let normal ALTER TABLE processing report that. + * walks up the tree to collect partition constraints or validate indexes. * * The target is not locked yet, so its ancestry could change before we * lock it, leaving some ancestors not pre-locked. This does not @@ -20518,13 +20528,50 @@ RangeVarCallbackForAttachPartition(const RangeVar *rv, Oid relid, Oid oldrelid, * deadlock detector will abort one of the transactions, which can then be * retried. */ - if (get_rel_relkind(relid) == RELKIND_PARTITIONED_TABLE && + if (relkind == RELKIND_PARTITIONED_TABLE && get_rel_relispartition(relid)) { + state->ancestorLockmode = AccessShareLock; /* Include detach-pending links, as generate_partition_qual() does. */ state->lockedAncestors = lock_partition_ancestors(relid, AccessShareLock, true); } + else if (relkind == RELKIND_PARTITIONED_INDEX) + { + List *ancestors = NIL; + + /* + * Follow the index hierarchy, which need not extend to the root of + * the table hierarchy. Lock each ancestor's table before its index, + * using the same lock mode as validatePartitionedIndex(). Taking + * weaker locks here could let concurrent ATTACH commands acquire + * compatible locks and then deadlock while upgrading them during + * validation. + */ + if (get_rel_relispartition(relid)) + ancestors = get_partition_ancestors(relid); + state->ancestorLockmode = AccessExclusiveLock; + for (int i = list_length(ancestors) - 1; i >= 0; i--) + { + Oid indexoid = list_nth_oid(ancestors, i); + Oid tableoid = IndexGetRelation(indexoid, true); + + /* An ancestor index might have been concurrently dropped. */ + if (!OidIsValid(tableoid)) + continue; + + LockRelationOid(tableoid, state->ancestorLockmode); + LockRelationOid(indexoid, state->ancestorLockmode); + state->lockedAncestors = lappend_oid(state->lockedAncestors, tableoid); + state->lockedAncestors = lappend_oid(state->lockedAncestors, indexoid); + } + list_free(ancestors); + + /* Also lock the target index's table before locking the index. */ + state->lockedTable = IndexGetRelation(relid, true); + if (OidIsValid(state->lockedTable)) + LockRelationOid(state->lockedTable, AccessShareLock); + } } /* -- 2.50.1 (Apple Git-155)