Re: tablecmds: Deadlock caused by Attach Partition

Hi Chao,

I looked at v1 and tried the problem on master. I have some comments below.
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.

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.

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.

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.)

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?
- "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."
- +1 to Rafia on pgindent; git show --check also flags trailing
whitespace on both "/* " lines.

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)

[1]
https://postgr.es/m/CAA4eK1LsFpjK5gL+0HEvoqB2DJVOi19vGAWbZBEx8fACOi5+_A@mail.gmail.com

Regards,
Surya Poondla