> On Oct 20, 2026, at 03:06, surya poondla <[email protected]> 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...@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/
v2-0001-Prevent-deadlock-between-ALTER-TABLE-recursion-an.patch
Description: Binary data
v2-0002-Add-isolation-test-for-ATTACH-PARTITION-lock-orde.patch
Description: Binary data
