Re: Recovering from detoast-related catcache invalidations

On 07/01/2025 23:56, Noah Misch wrote:
> On Tue, Dec 24, 2024 at 12:18:09AM +0200, Heikki Linnakangas wrote:
>> 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.
>
> That's similar to how relcache has been doing it (in_progress_list). I see no
> problem applying that technique here.

Oh thanks, I didn't notice that in relcache.c. I adjusted the naming and
comments in the new catcache.c code to be a little closer to the
relcache.c version, just to make it a bit more consistent.

The main difference is that relcache.c uses an array and an end-of-xact
callback to reset the array on error, while my implementation uses a
linked list of entries allocated on stack and PG_TRY-CATCH for error
cleanup. I considered adopting relcache.c's approach for the sake of
consistency, but decided to keep my original approach in the end. Some
of the code in catcache.c already had a suitable PG_TRY-CATCH block and
I didn't want to add a new end-of-xact callback just for this.

>> not sure I want to
>> commit the test with the injection point, but it's useful now to demonstrate
>> the bug.)
>
> I'd err on the side of including it. Apart from some copied comments, the
> test looks ready.

Ok, I cleaned up the comments.

> On Wed, Dec 25, 2024 at 11:27:38AM +0200, Heikki Linnakangas wrote:
>> +++ b/src/test/modules/test_misc/t/007_bugs.pl
>
> test_misc/t/007_bugs.pl could be home to almost anything. How about naming it
> 007_catcache_inval.pl?

Renamed

>> @@ -744,6 +770,13 @@ ResetCatalogCache(CatCache *cache)
>> #endif
>> }
>> }
>> +
>> + /* Also invalidate any entries that are being built */
>> + for (CatCCreating *e = catcache_creating_stack; e != NULL; e = e->next)
>> + {
>> + if (e->cache == cache)
>> + e->dead = true;
>> + }
>> }
>
> With debug_discard_caches=1, "make check" hangs early with some INSERT using
> 100% CPU. The new test file does likewise. I bet this needs a special case
> to short-circuit debug_discard_caches, like RelationCacheInvalidate() has.

Added

>> @@ -1665,6 +1698,8 @@ SearchCatCacheList(CatCache *cache,
>> HeapTuple ntp;
>> MemoryContext oldcxt;
>> int i;
>> + volatile CatCCreating creating_list;
>
> You could drop the volatile by copying catcache_creating_stack to an
> additional var that you reference after longjmp, instead of referencing
> creating_list.next after longjmp. Likewise for the instance of volatile in
> CatalogCacheCreateEntry().
> https://pubs.opengroup.org/onlinepubs/9799919799/functions/longjmp.html has
> the rules. Using volatile is fine, though.

Thanks for the tips. I used the additional var, it seems a little nicer.

>> + bool first_iter = true;
>
> This is okay as non-volatile, but it could just as easily move inside the
> PG_TRY.

Done

>> @@ -2076,38 +2118,33 @@ CatalogCacheCreateEntry(CatCache *cache, HeapTuple ntp, SysScanDesc scandesc,
>
>> + PG_TRY();
>> {
>> - matches = equalTuple(before, ntp);
>> - heap_freetuple(before);
>> + dtp = toast_flatten_tuple(ntp, cache->cc_tupdesc);
>
> gcc 4.8.5 warns:
>
> catcache.c: In function ‘CatalogCacheCreateEntry’:
> catcache.c:2159:29: warning: ‘dtp’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> ct->tuple.t_tableOid = dtp->t_tableOid;
>
> I remember some commit of a gcc 4.x warning fix in a recent year, and we do
> have buildfarm representation. Consider silencing it.

Hmm, I guess the compiler doesn't see that it's initialized with all the
PG_TRY()/CATCH() stuff. I initialized it to NULL to silence the warning.

Review of this new version is much appreciated, but if I don't hear
anything I'll commit and backpatch this.

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