Re: tablecmds: Deadlock caused by Attach Partition

> On Oct 20, 2026, at 03:06, surya poondla <suryapoondla4(at)gmail(dot)com> wrote:
>
> Hi Chao,
>
> I looked at v1 and tried the problem on master. I have some comments below.

Thanks a lot for your review.

> I'm also attaching an isolation test, to be applied on top of your v1
> (in reply to Rafia's request). It needs no injection point, so it could replace the one Kirill posted.
>
> 1. The INSERT-into-a-partition variant of this deadlock (ExecPartitionCheck() -> generate_partition_qual() locking the parents
> while the partition is already locked) was discussed in 2021 [1], and the conclusion there was that child-first, then parent, locking is inherent
> for DML on a partition and accepted as rare. I reproduced that variant on master too, and v1 doesn't (and can't) cover it, since the partition is
> locked during parse analysis. ATTACH is different in that it can lock the ancestors before the target, so I think the narrow fix is reasonable, but
> it might be worth saying in the commit message that the DML case is known and intentionally out of scope.

Let’s keep the scope of this patch narrowed. I have updated the commit message.

>
> 2. RangeVarCallbackForDropRelation() already does the same thing for DROP of a partition, it locks the parent before the partition
> "because queries lock the parent before its partitions, so we risk deadlock if we do it the other way around." That supports doing this in
> the RangeVar callback. Two things it does that v1 doesn't:
>
> i) It only takes the lock when relOid != oldRelOid, and releases the lock taken for a previous lookup if the name now resolves to a
> different relation. v1 redoes get_partition_ancestors() on every callback invocation and keeps locks on the ancestors of a relation
> the name no longer refers to.
>

This is an old patch. When I created it, TBH, I wasn’t aware the callback retry mechanism. In v2, I switched to a similar way of RangeVarCallbackForDropRelation to remember locked ancestors and release them upon retry.

> ii) It uses get_partition_parent(relOid, true), i.e. it follows a parent link that is pending detach. v1 uses get_partition_ancestors(), which
> stops at a detach-pending link, while generate_partition_qual() calls get_partition_parent(..., true) and keeps going. So if the
> target (or an ancestor) is in the middle of DETACH CONCURRENTLY, the pre-lock covers fewer ancestors than the later walk. I haven't
> looked at whether a recursing ALTER TABLE can still deadlock with it in that state, since find_all_inheritors() may skip the detached
> partition, but either way it would be good to say whether the difference is intended.

Good catch. Fixed in v2.

>
> 3. The commit message says find_all_inheritors() locks partitions "in ascending OID order" and that the patch pre-locks
> ancestors "in OID order". The code locks ancestors root-first using pg_inherits, which is the right thing. find_all_inheritors() goes one
> level at a time and sorts only siblings by OID (find_inheritance_children_extended(), pg_inherits.c), so parents are
> always locked before children whatever their OIDs. So the "potential issue remaining" in your first mail doesn't apply to this patch, and I think the
> commit message should say "root to parent" rather than OID order. (Kirill, small correction on your note, the sorting happens per level, in
> find_inheritance_children_extended(), not across the whole tree.)

Updated the commit message.

>
> 4. Small things in the code:
> - lock_partition_ancestors() returns the ancestor list, but the only caller ignores it, so the list is just allocated and dropped. Could it
> return void, or be a short loop in tablecmds.c instead of a new function in partition.c?

V2 now uses the returned list.

> - "The target table must be a partitioned table, but if, we don't need to fail here;" is not very clear. Maybe "If the target is not a
> partitioned table, don't fail here; ALTER TABLE will report that later."

Updated the comment.

> - +1 to Rafia on pgindent; git show --check also flags trailing whitespace on both "/* " lines.

Ran pgindent on v2.

>
> 5. The attached test is (0002-Add-isolation-test-for-ATTACH-PARTITION-lock-orderin.patch)
> src/test/isolation/specs/partition-attach-lock-order.spec. It doesn't need an injection point: session 1 runs LOCK TABLE ONLY root IN ACCESS
> EXCLUSIVE MODE (only the root, the way the recursing ALTER TABLE has it before it recurses), session 2 runs ATTACH PARTITION on a mid-level
> table, then session 1 runs the recursing ALTER TABLE. It uses three levels (root > mid1 > mid2, attaching to mid2), so pre-locking only the direct parent would still deadlock on mid1.
>
> With v1 applied the test passes. Without v1 (unpatched master), s2attach has locked mid2 and then mid1 while walking up for the partition
> constraint, and waits for the root, which s1 holds. s1alter, recursing from the root, waits for mid1, which s2attach holds. The deadlock detector
> aborts the ATTACH:
>
> step s1alter: ALTER TABLE attach_lock_root ALTER COLUMN id SET DEFAULT 0; <waiting ...>
> step s1alter: <... completed>
> step s2attach: <... completed>
> ERROR: deadlock detected
>
> Note that the deadlock only happens when session 2 hasn't cached the target's partition constraint yet (generate_partition_qual() returns the
> cached copy without locking anything). The isolation sessions connect before setup creates the tables, so the cache is always cold there. This
> is also why a fresh session for every ATTACH reproduces it so easily.
>
> Feel free to fold the test into your next version.
> (Note: v1 is unchanged and is same as Chao's v1)

Your test looks good to me. I’d keep it in 0002 and let the committer decide whether or not to accept it.

>
> [1] https://postgr.es/m/CAA4eK1LsFpjK5gL+0HEvoqB2DJVOi19vGAWbZBEx8fACOi5+_A@mail.gmail.com
>
> Regards,
> Surya Poondla
>
>
> <0002-Add-isolation-test-for-ATTACH-PARTITION-lock-orderin.patch><0001-Prevent-deadlock-between-ALTER-TABLE-recursion-and-A.patch>

PFA v2:

* 0001 - Addressed all review comments
* 0002 - The same Surya as posted, only added Author and Reviewed-by annotations

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/