Re: Recovering from detoast-related catcache invalidations

On 14/12/2024 02:06, Heikki Linnakangas wrote:
> Ok, I missed that. It does not handle the 2nd scenario though: If a new
> catalog tuple is concurrently inserted that should be part of the list,
> it is missed.
>
> I was able to reproduce that, by pausing a process with gdb while it's
> building the list in SearchCatCacheList():
>
> 1. Create a function called foofunc(integer). It must be large so that
> its pg_proc tuple is toasted.
>
> 2. In one backend, run "SELECT foofunc(1)". It calls
> FuncnameGetCandidates() which calls
> "SearchSysCacheList1(PROCNAMEARGSNSP, CStringGetDatum(funcname));". Put
> a break point in SearchCatCacheList() just after the systable_beginscan().
>
> 3. In another backend, create function foofunc() with no args.
>
> 4. continue execution from the breakpoint.
>
> 5. Run "SELECT foofunc()" in the first session. It fails to find the
> function. The error persists, it will fail to find that function if you
> try again, until the syscache is invalidated again for some reason.
>
> Attached is an injection point test case to reproduce that. If you
> change the test so that the function's body is shorter, so that it's not
> toasted, the test passes.

I'm thinking of the attached to fix this. It changes the strategy for
detecting concurrent cache invalidations. Instead of the "recheck"
mechanism that was introduced in commit ad98fb1422, keep a stack of
"build in-progress" entries, and CatCacheInvalidate() invalidate those
"in-progress" entries in addition to the actual CatCTup and CatCList
entries.

My first attempt was to insert the CatCTup or CatCList entry to the
catcache before starting to build it, marked with a flag to indicate
that the entry isn't fully built yet. But when I started to write that
it got pretty invasive, and it felt easier to add another structure to
hold the in-progress entries instead.

(I'm not sure I got the 'volatile' markers on the local variable right
in this patch; before committing this I'll need to freshen my memory on
the rules on PG_TRY() and local variables again. Also, I'm not sure I
want to commit the test with the injection point, but it's useful now to
demonstrate the bug.)

--
Heikki Linnakangas
Neon (https://neon.tech)