> For the main fix, I'd make it narrower than v1, leave has_subclass() and > find_inheritance_children() as they are, and only let > find_all_inheritors() skip a missing relation when called with NoLock (and > never for the starting relation, which the caller must lock). > A static helper with a missing_ok flag would do it without changing any > exported function.
Thanks for the review, that makes sense. My logic for changing
find_inheritance_children_extended was that this error outside NoLock
is only a sanity check and shouldn't happen, but you are right that we
could keep the more robust code.
v2 attached does exactly this: find_inheritance_children_extended() is
now a wrapper around a static find_inheritance_children_internal()
with a missing_ok flag, and only find_all_inheritors() sets it with
NoLock and not for the starting relation.
> Without NOWAIT it can just pass the lock mode to find_all_inheritors().
> With NOWAIT it would need to go back to locking one level at a time.
> That could be a separate patch.
Agreed, I left LOCK TABLE alone in this patch.
> 1. This isn't limited to sub-partitioned tables. Dropping a partition
> only locks its direct parent, so VACUUM's lock on the
> root protects the first level and nothing below it. Plain leaves at the
> second level are enough.
You are right, my example was more complex than necessary, I updated
the commit message.
(But also see the next point)
> 2. If the injection point were in the find_all_inheritors() loop instead of
> after the relhassubclass check, the test wouldn't need
> the extra q/q1 tables. root -> p1 -> {a, b} would do, and it would cover
> the leaf case too.
I moved the injection point to the top of the find_all_inheritors()
loop, but I still needed one extra table to make it fail reliably.
To reliably trigger it we also need a pg_inherits scan after the drop,
since that has no syscache and takes a new catalog snapshot.
So the test now uses root -> p1 -> {a -> a1, b}, waits before a is
expanded, and drops the leaf b.
> 3. The test comment "q's pg_inherits scan doesn't see p2 anymore" is a bit
> misleading. q's scan never looks at p2; q just gets
> expanded first because it has the smaller OID.
Right, that comment was wrong, the new one explains what I wrote above.
v2-0001-Fix-find_all_inheritors-with-NoLock-against-concu.patch
Description: Binary data
