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)