Fix recursion in cache processing

[cevian]

Previously, cache invalidation could cause recursion in cache processing.
PR #1493 fixed this for the

• cache_invalidate_callback() -> ts_extension_invalidate()

call path. But the call path

• cache_invalidate_callback() -> ts_extension_is_loaded()

could still go into recursion.

So, this PR moves the recursion-prevention logic into
extension_update_state(), which is common to both call paths.

Fixes #2200.

[codecov]

Codecov Report

Merging #2259 into master will increase coverage by 0.12%.
The diff coverage is 90.90%.

@@            Coverage Diff             @@
##           master    #2259      +/-   ##
==========================================
+ Coverage   90.10%   90.22%   +0.12%     
==========================================
  Files         211      212       +1     
  Lines       34290    34297       +7     
==========================================
+ Hits        30897    30946      +49     
+ Misses       3393     3351      -42     

Impacted Files	Coverage Δ
src/compat.h	100.00% <ø> (ø)
src/hypertable.c	88.61% <0.00%> (-0.10%)	⬇️
src/dimension_slice.c	95.25% <50.00%> (+0.81%)	⬆️
src/debug_wait.c	88.88% <88.88%> (ø)
src/chunk.c	95.38% <100.00%> (+0.06%)	⬆️
src/extension.c	88.29% <100.00%> (ø)
src/hypercube.c	92.85% <100.00%> (+0.14%)	⬆️
src/scanner.c	96.26% <100.00%> (+0.02%)	⬆️
src/loader/bgw_message_queue.c	84.51% <0.00%> (-2.59%)	⬇️
src/bgw/scheduler.c	82.78% <0.00%> (-1.79%)	⬇️
... and 4 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5300b68...7b65ce7. Read the comment docs.

[erimatnor]

@cevian would be good to have a test case that reproduces the bug and shows that this is issue is fixed.

[cevian]

@erimatnor In theory I agree, but I haven't been able to come up with a reproducible case. This works most of the time as-is. It's some internal Postgres state that causes this behavior to be triggered.

[cevian]

@erimatnor I spent a lot more time trying to reproduce and could not. It seems like the loop is within ReceiveSharedInvalidMessages being execute at sinval.c:90 and sinval:120 (the latter every 32 = MAXINVALMSGS calls).

I think this only happens when there are a lot of msgs or something similar. I put my attempts at testing this in the fix_recursion_tests branch in my fork in case people want to get back to it at some point, but it feels like there is a benefit to merging this in the meantime.

[pmwkaa]

Looks good, but agree with @erimatnor that it would be great to have a test case for it

[mkindahl]

I think something is wrong with the cache code if we have to rely on these hacks to solve issues and I wonder how many more issues we have with the cache code. However, this solves the current problem and I do not see that this approach should cause any issues, so approving.

This seems to be a rare occurrence, so not sure how we can test this.

[cevian]

Leaving some more notes: I suspect this happens in the UNKNOWN state and in background workers. The backend causing the invalidation needs to commit or abort to cause invalidations to be sent to other backends, including bg workers. The bg workers has to also already be in a transaction when the messages are received, otherwise it will just happily stay in the unknown state.

Another note: scanning for the namespace in the heap instead of through cache will not help. The backtrace shows that the recursion happens through LockRelationOid, which would be called for a pure-heap scan too. Notice that the backtrace is for a cache miss.