Re: Recovering from detoast-related catcache invalidations

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)