From f2a3f4e65e39a7d8fd454e0bdcf43a9d710315f4 Mon Sep 17 00:00:00 2001 From: "Chao Li (Evan)" Date: Fri, 30 Jan 2026 11:10:32 +0800 Subject: [PATCH v2 1/2] Prevent deadlock between ALTER TABLE recursion and ATTACH PARTITION Within a declarative partition tree, recursive ALTER TABLE commands (e.g. ALTER COLUMN SET DEFAULT) lock ancestors before descendants. In contrast, ALTER TABLE ATTACH PARTITION may lock the target table and then its ancestors while collecting partition constraints. When these operations run concurrently, this lock order inversion can lead to a deadlock. Pre-lock all ancestor partitioned tables from root to parent in RangeVarCallbackForAttachPartition(), before the target relation is locked. This matches the top-down locking of the partition tree during recursive ALTER TABLE. The ordering depends on the partition hierarchy, not on the relative OIDs of ancestors and descendants. Track the ancestor locks across relation lookup retries, avoiding repeated acquisition for the same target and releasing obsolete locks if the name resolves to a different relation. Include pending-detach parent links so that pre-locking covers the ancestors visited by generate_partition_qual(). This fix is limited to ATTACH PARTITION. The known deadlock involving DML directly on a partition remains outside its scope: the partition is already locked during parse analysis before its ancestors are locked to check partition constraints. Author: Chao Li Reviewed-by: Rafia Sabih Reviewed-by: Kirill Reshke Reviewed-by: Surya Poondla Discussion: https://postgr.es/m/CFACA0EB-7E6F-4FAA-9ACE-1FC2226D7482@gmail.com --- src/backend/catalog/partition.c | 46 ++++++++++++++++++--- src/backend/commands/tablecmds.c | 70 ++++++++++++++++++++++++++++++++ src/include/catalog/partition.h | 3 ++ 3 files changed, 113 insertions(+), 6 deletions(-) diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c index 28f3cade6ff..2f460c0d223 100644 --- a/src/backend/catalog/partition.c +++ b/src/backend/catalog/partition.c @@ -26,6 +26,7 @@ #include "nodes/makefuncs.h" #include "optimizer/optimizer.h" #include "rewrite/rewriteManip.h" +#include "storage/lmgr.h" #include "utils/fmgroids.h" #include "utils/partcache.h" #include "utils/rel.h" @@ -34,7 +35,7 @@ static Oid get_partition_parent_worker(Relation inhRel, Oid relid, bool *detach_pending); static void get_partition_ancestors_worker(Relation inhRel, Oid relid, - List **ancestors); + List **ancestors, bool even_if_detached); /* * get_partition_parent @@ -126,6 +127,9 @@ get_partition_parent_worker(Relation inhRel, Oid relid, bool *detach_pending) * The first element is the immediate parent and the last one is the topmost * parent in the partition hierarchy. * + * If a partition is being detached, stop before adding its parent, excluding + * that parent and any ancestors above it. + * * Note: Because this function assumes that the relation whose OID is passed * as an argument and each ancestor will have precisely one parent, it should * only be called when it is known that the relation is a partition. @@ -138,33 +142,63 @@ get_partition_ancestors(Oid relid) inhRel = table_open(InheritsRelationId, AccessShareLock); - get_partition_ancestors_worker(inhRel, relid, &result); + get_partition_ancestors_worker(inhRel, relid, &result, false); table_close(inhRel, AccessShareLock); return result; } +/* + * lock_partition_ancestors + * Lock all ancestors of given relation from root to parent. + * + * Include detach-pending links if even_if_detached is true; otherwise stop + * at such a link, as get_partition_ancestors() does. Return the locked + * ancestors in parent-to-root order so callers can release the locks if + * relation lookup must be retried for a different relation. + */ +List * +lock_partition_ancestors(Oid relid, LOCKMODE lockmode, bool even_if_detached) +{ + List *ancestors = NIL; + Relation inhRel; + + inhRel = table_open(InheritsRelationId, AccessShareLock); + get_partition_ancestors_worker(inhRel, relid, &ancestors, even_if_detached); + table_close(inhRel, AccessShareLock); + + for (int i = list_length(ancestors) - 1; i >= 0; i--) + { + Oid parentoid = list_nth_oid(ancestors, i); + + LockRelationOid(parentoid, lockmode); + } + return ancestors; +} + /* * get_partition_ancestors_worker * recursive worker for get_partition_ancestors */ static void -get_partition_ancestors_worker(Relation inhRel, Oid relid, List **ancestors) +get_partition_ancestors_worker(Relation inhRel, Oid relid, List **ancestors, + bool even_if_detached) { Oid parentOid; bool detach_pending; /* * Recursion ends at the topmost level, ie., when there's no parent; also - * when the partition is being detached. + * when the partition is being detached, unless requested otherwise. */ parentOid = get_partition_parent_worker(inhRel, relid, &detach_pending); - if (parentOid == InvalidOid || detach_pending) + if (parentOid == InvalidOid || (detach_pending && !even_if_detached)) return; *ancestors = lappend_oid(*ancestors, parentOid); - get_partition_ancestors_worker(inhRel, parentOid, ancestors); + get_partition_ancestors_worker(inhRel, parentOid, ancestors, + even_if_detached); } /* diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 93ad49e59fd..e6bb52aa6b5 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -327,6 +327,16 @@ struct DropRelationCallbackState char actual_relpersistence; }; +/* + * communication between AlterTableLookupRelation and + * RangeVarCallbackForAttachPartition + */ +struct AttachPartitionCallbackState +{ + Node *stmt; + List *lockedAncestors; +}; + /* Alter table target-type flags for ATSimplePermissions */ #define ATT_TABLE 0x0001 #define ATT_VIEW 0x0002 @@ -724,6 +734,8 @@ static void RangeVarCallbackForDropRelation(const RangeVar *rel, Oid relOid, Oid oldRelOid, void *arg); static void RangeVarCallbackForAlterRelation(const RangeVar *rv, Oid relid, Oid oldrelid, void *arg); +static void RangeVarCallbackForAttachPartition(const RangeVar *rv, Oid relid, + Oid oldrelid, void *arg); static PartitionSpec *transformPartitionSpec(Relation rel, PartitionSpec *partspec); static void ComputePartitionAttrs(ParseState *pstate, Relation rel, List *partParams, AttrNumber *partattrs, List **partexprs, Oid *partopclass, Oid *partcollation, @@ -4532,6 +4544,26 @@ CheckAlterTableIsSafe(Relation rel) Oid AlterTableLookupRelation(AlterTableStmt *stmt, LOCKMODE lockmode) { + ListCell *lc; + + foreach(lc, stmt->cmds) + { + AlterTableCmd *cmd = lfirst_node(AlterTableCmd, lc); + + if (cmd->subtype == AT_AttachPartition) + { + struct AttachPartitionCallbackState state = {(Node *) stmt, NIL}; + Oid relid; + + relid = RangeVarGetRelidExtended(stmt->relation, lockmode, + stmt->missing_ok ? RVR_MISSING_OK : 0, + RangeVarCallbackForAttachPartition, + &state); + list_free(state.lockedAncestors); + return relid; + } + } + return RangeVarGetRelidExtended(stmt->relation, lockmode, stmt->missing_ok ? RVR_MISSING_OK : 0, RangeVarCallbackForAlterRelation, @@ -20309,6 +20341,7 @@ RangeVarCallbackForAlterRelation(const RangeVar *rv, Oid relid, Oid oldrelid, AclResult aclresult; char relkind; + tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relid)); if (!HeapTupleIsValid(tuple)) return; /* concurrently dropped */ @@ -20446,6 +20479,43 @@ RangeVarCallbackForAlterRelation(const RangeVar *rv, Oid relid, Oid oldrelid, ReleaseSysCache(tuple); } +/* + * Pre-lock ATTACH PARTITION's ancestors before locking the target relation. + */ +static void +RangeVarCallbackForAttachPartition(const RangeVar *rv, Oid relid, Oid oldrelid, + void *arg) +{ + struct AttachPartitionCallbackState *state = arg; + + /* Release locks belonging to a previous lookup of a different relation. */ + if (relid != oldrelid && state->lockedAncestors != NIL) + { + ListCell *lc; + + foreach(lc, state->lockedAncestors) + UnlockRelationOid(lfirst_oid(lc), AccessShareLock); + list_free(state->lockedAncestors); + state->lockedAncestors = NIL; + } + + RangeVarCallbackForAlterRelation(rv, relid, oldrelid, state->stmt); + + if (relid == oldrelid || !OidIsValid(relid)) + return; + + /* + * Lock ancestors from root to parent before ATTACH locks the target and + * walks up the tree to collect partition constraints. Include + * detach-pending links, as generate_partition_qual() does. If the target + * is not a partitioned table, let normal ALTER TABLE processing report + * that. + */ + if (get_rel_relkind(relid) == RELKIND_PARTITIONED_TABLE) + state->lockedAncestors = + lock_partition_ancestors(relid, AccessShareLock, true); +} + /* * Transform any expressions present in the partition key * diff --git a/src/include/catalog/partition.h b/src/include/catalog/partition.h index a93cf081dd2..e8ce1045b8d 100644 --- a/src/include/catalog/partition.h +++ b/src/include/catalog/partition.h @@ -14,6 +14,7 @@ #define PARTITION_H #include "partitioning/partdefs.h" +#include "storage/lockdefs.h" #include "utils/relcache.h" /* Seed for the extended hash function */ @@ -21,6 +22,8 @@ extern Oid get_partition_parent(Oid relid, bool even_if_detached); extern List *get_partition_ancestors(Oid relid); +extern List *lock_partition_ancestors(Oid relid, LOCKMODE lockmode, + bool even_if_detached); extern Oid index_get_partition(Relation partition, Oid indexId); extern List *map_partition_varattnos(List *expr, int fromrel_varno, Relation to_rel, Relation from_rel); -- 2.50.1 (Apple Git-155)