Horton

Re: Recovering from detoast-related catcache invalidations

Heikki Linnakangas <hlinnaka@iki.fi>

View thread
From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Noah Misch <noah@leadboat.com>, Xiaoran Wang <fanfuxiaoran@gmail.com>, pgsql-hackers@lists.postgresql.org, Andres Freund <andres@anarazel.de>
Date: 2024-12-23T22:18:09Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix catcache invalidation of a list entry that's being built

  2. Cope with inplace update making catcache stale during TOAST fetch.

  3. Add previous commit to .git-blame-ignore-revs.

  4. Re-pgindent catcache.c after previous commit.

  5. Cope with catcache entries becoming stale during detoasting.

Attachments

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)