Re: tablecmds: Deadlock caused by Attach Partition

Hi Chao,

Thanks for v5. I applied the three patches on top of b936425002c, which
has Álvaro's 2ebd6849743. Everything applies without conflicts
and builds without warnings. With v5, partition-attach-lock-order and the
new partition-index-attach-lock-order both pass, along with
partition-concurrent-attach and partition-drop-index-locking. On an
unpatched build of the same commit, both lock-order specs fail with
"deadlock detected" in the ATTACH step, including the permutation with
two concurrent index ATTACHes, and the other two pass.

Nothing new from me on 0001. Apart from the moved comment and the change
to how the callback state is initialized, it's the same as v4.

0002 is where I spent most of my time. I wanted to see what locks ALTER
INDEX ... ATTACH PARTITION actually takes with and without the patch, so
I checked pg_locks in a single-user backend. I used a three-level tree,
root > mid1 > mid2 > {leaf1, leaf2}, created the partitioned indexes with
ON ONLY so they start out invalid, and then ran ALTER INDEX mid2_idx
ATTACH PARTITION leaf1_idx.

When leaf2_idx isn't attached yet, mid2_idx stays invalid and nothing
above it gets validated. In that case master doesn't lock any of the
ancestors, but v5 takes AccessExclusiveLock on root, mid1, root_idx and
mid1_idx.

When leaf2_idx is already attached, this ATTACH completes mid2_idx and
validation cascades all the way up to root_idx. Here master and v5 take
the same locks (AccessExclusiveLock on those same four), and the indexes
end up valid either way.

So the ordering is right for the cascading case, which is the one that
matters for the deadlock. The catch is that every ATTACH on a partition
index now takes AccessExclusiveLock on all of its ancestor indexes and
their tables, even when it isn't going to validate anything. In a full
index hierarchy that means the root table, so nobody can read or write the
root until the ATTACH commits, and concurrent index ATTACHes in the
same tree would end up running one at a time.

I can see why a weaker pre-lock doesn't work i.e validation would then have
to upgrade it to AccessExclusiveLock, and two ATTACHes doing that at the
same time could deadlock on the upgrade. Working out in the callback
whether this particular ATTACH will complete the parent doesn't look
reliable either, because it depends on other ATTACHes that haven't
committed yet, which is exactly the race 2ebd6849743 fixed. I don't have a
better idea, but I think the extra locking is worth a conscious
decision. Álvaro, what do you think, is that trade-off ok?

Regards,
Surya Poondla