Horton

Re: Recovering from detoast-related catcache invalidations

Heikki Linnakangas <hlinnaka@iki.fi>

View thread
From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Noah Misch <noah@leadboat.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Xiaoran Wang <fanfuxiaoran@gmail.com>, pgsql-hackers@lists.postgresql.org, Andres Freund <andres@anarazel.de>
Date: 2025-01-14T13:13:21Z
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.

On 12/01/2025 03:26, Noah Misch wrote:
> On Thu, Jan 09, 2025 at 11:39:53AM +0200, Heikki Linnakangas wrote:
>> On 07/01/2025 23:56, Noah Misch wrote:
>> @@ -697,9 +725,14 @@ CreateCacheMemoryContext(void)
>>    *
>>    * This is not very efficient if the target cache is nearly empty.
>>    * However, it shouldn't need to be efficient; we don't invoke it often.
>> + *
>> + * If 'debug_discard' is true, we are being called as part of
>> + * debug_discard_caches.  In that case, the cache not reset for correctness,
> 
> s/cache not/cache is not/ or similar

fixed

>> +			PG_TRY();
>>   			{
>> -				matches = equalTuple(before, ntp);
>> -				heap_freetuple(before);
>> +				dtp = toast_flatten_tuple(ntp, cache->cc_tupdesc);
>>   			}
>> -			if (!matches || !systable_recheck_tuple(scandesc, ntp))
>> +			PG_FINALLY();
>>   			{
>> -				heap_freetuple(dtp);
> 
> Is this an intentional removal of the heap_freetuple(dtp) before return NULL?

No, that was a mistake. Fixed.

>> -				return NULL;
>> +				Assert(catcache_in_progress_stack == &in_progress_ent);
>> +				catcache_in_progress_stack = save_in_progress;
>>   			}
>> +			PG_END_TRY();
>> +
>> +			if (in_progress_ent.dead)
>> +				return NULL;
> ...
>> +$node->append_conf(
>> +	'postgresql.conf', qq{
>> +debug_discard_caches=1
>> +});
> 
> I'd drop this debug_discard_caches.  debug_discard_caches buildfarm runs will
> still test it, so let's have normal runs test the normal case.

Ah yes, I didn't mean to include that; removed.

Committed with those fixes. Thanks for the review!

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