Re: Recovering from detoast-related catcache invalidations

On Thu, Jan 09, 2025 at 11:39:53AM +0200, Heikki Linnakangas wrote:
> 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.

That difference is reasonable. catcache will add PG_TRY overhead only in the
HEAP_HASEXTERNAL case, which is not a normal benchmark situation. If relcache
were to adopt the PG_TRY approach, it would be harder to rule out the
significance of the overhead. So a long-term state of using both designs is
reasonable. It's not a mere historical accident.

> > 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.

It's warning-free now.

> @@ -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

> + 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?

> - 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.