Re: find_all_inheritors() with NoLock fails on concurrently dropped sub-partitions

Hi Zsolt, Tom,

Thank you for reporting the issue and the patch.
I agree v1 is too broad. It changes find_inheritance_children() for every
caller, including ones that hold a lock on the parent, where a
missing pg_class row really is a bug and should still error out.

But I'm not sure the callers are the ones at fault. The comment on
find_all_inheritors() explicitly allows NoLock:
* If lockmode is NoLock then no locks are acquired, but caller must
* beware of race conditions against possible DROPs of child relations.

The callers already do that. VACUUM, ANALYZE, REPACK and
pg_get_publication_tables() all skip relations that have disappeared.
The problem is that find_all_inheritors() errors out before they get the
chance, because it looks up relations it never locked.

Locking the whole tree isn't really an option for those callers. VACUUM
only takes a short lock on the table it was given and avoids
locking partitions to avoid deadlocks (19de0ab23cc). The publication code
doesn't lock even the root.

Yes, before b7e51b350c4 commit, LOCK TABLE locked each child before looking
at its children, so it couldn't hit this.
The fix below would stop the error for LOCK TABLE too, but LOCK TABLE is
the one caller where I think locking the tree is right, since that's its
whole job.
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.

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. Treating a dropped relation as having no
children is correct, since dropping a partitioned table drops its
partitions too.

A few comments on v1:
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.
With root -> p1 -> {a, b}, I get

ERROR: cache lookup failed for relation 16420

from has_subclass() on master if b is dropped while VACUUM root is walking
the tree. So dropping old partitions of an ordinary
two-level setup during a VACUUM can hit it.

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.

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.

4. There are two more NoLock callers you didn't list, indexcmds.c and
sepgsql. Both already hold locks on the tree, so they're fine.

5. A test for pg_get_publication_tables() would be nice, since that path
doesn't lock anything.

Regards,
Surya Poondla