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)