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

Zsolt Parragi <zsolt.parragi@percona.com> writes:

find_all_inheritors() with NoLock doesn't lock anything below the
given relation, but it still calls find_inheritance_children() on
every relation it found, which starts with has_subclass(), and that
errors out if the pg_class row is gone:

I think the assumption is that NoLock should only be used if the
caller already got a lock on the whole inheritance tree. So
I would blame the callers that are not doing that, rather than
squishy-fying find_all_inheritors' semantics. This seems quite
horrid:

The attached patch makes find_inheritance_children_extended() return
an empty list if the relation is gone, instead of erroring out.

regards, tom lane

From the 4 affected callers:
* For LOCK TABLE, it somewhat seems to be a regression caused by b7e51b350c4
* I would have to investigate GetPubPartitionOptionRelations, but I assume there's some reason why it wants to avoid locking. There's no comment explaining it, and it's not mentioned in the commit message.
* REPACK/CLUSTER says that it is the same as VACUUM
* and VACUUM explicitly says that it shouldn't lock because locking could cause deadlocks, it has a long comment about it from 19de0ab23cc:

* Make relation list entries for this rel and its partitions, if any.
* Note that the list returned by find_all_inheritors() includes the
* passed-in OID at its head. There's no point in taking locks on the
* individual partitions yet, and doing so would just add unnecessary
* deadlock risk.

the same commit message also says this about the issue it fixes:

A concurrent DROP TABLE could now produce "cache lookup
failed", which we don't want to have happen in normal operation.

Which is also the leftover issue caused by the use of find_all_inheritors with NoLock in the above scenario.

Assuming that the deadlock comment is accurate, that means we can't use locking find_all_inheritors, and that leaves either accepting the possible cache lookup failed error or making the NoLock variant resistent to it. (or inventing a similar different function that does the same without the cache error)

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

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.

Hi Zsolt,

Thanks for v2, it looks good to me.

I applied it on master and ran both test cases by hand, and also plain-leaf
versions with VACUUM, REPACK and LOCK TABLE. Everything
passes, and LOCK TABLE skips the dropped partition as expected. With the
fix disabled, both of your test cases fail with "cache lookup
failed" in has_subclass().

You're right about needing "a1". In my runs, with plain leaves the
pg_publication_tables case didn't fail even without the fix (the
dropped table was just left out), and the VACUUM case failed only when the
backend hadn't already cached the dropped table. So your test is the more
reliable one.

One note for back-patching, the test uses string conditions for injection
points, which only exist on master (0fd73cdffc1), so it
can't go to the back branches. The fix itself should back-patch with small
adjustments (an #include conflict on 18 and older, and 16 and
older have no utils/injection_point.h).

Otherwise I think this is ready for a committer to look at it.

Regards,
Surya Poondla

One note for back-patching, the test uses string conditions for injection
points, which only exist on master

Yes, I have a version of the testcase without it, if this patch gets accepted I can share the version of the patch for 19 and earlier. It uses the new form for reliability: making the test reliably fail on the unpatched code without it is much harder, the test is simpler with it.