Re: type cache cleanup improvements

On Tue, Oct 15, 2024 at 12:50 PM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
> On Tue, Oct 15, 2024 at 4:09 PM Alexander Korotkov <aekorotkov(at)gmail(dot)com> wrote:
> >
> > Hi, Jian!
> >
> > Thank you for your review.
> >
> > On Tue, Oct 15, 2024 at 10:34 AM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
> > > I don't fully understand all of it. but I did some tests anyway.
> > >
> > > static void
> > > cleanup_in_progress_typentries(void)
> > > {
> > > int i;
> > > if (in_progress_list_len > 1)
> > > elog(INFO, "%s:%d in_progress_list_len > 1", __FILE_NAME__, __LINE__);
> > > for (i = 0; i < in_progress_list_len; i++)
> > > {
> > > TypeCacheEntry *typentry;
> > > typentry = (TypeCacheEntry *) hash_search(TypeCacheHash,
> > > &in_progress_list[i],
> > > HASH_FIND, NULL);
> > > insert_rel_type_cache_if_needed(typentry);
> > > }
> > > in_progress_list_len = 0;
> > > }
> > >
> > > the regress still passed.
> > > I assume "elog(INFO, " won't interfere in cleanup_in_progress_typentries.
> > > So we lack tests for larger in_progress_list_len values or i missed something?
> >
> > Try to run test suite with -DCLOBBER_CACHE_ALWAYS.
> >
>
> build from source, DCLOBBER_CACHE_ALWAYS takes a very long time.
> So I gave up.
>
>
> in lookup_type_cache, we unconditionally do
> in_progress_list_len++;
> in_progress_list_len--;

Yes, this should work OK when no errors. On error or interruption,
finalize_in_progress_typentries() will clean the things up.

> "static int in_progress_list_len;"
> means in_progress_list_len value change is confined in
> src/backend/utils/cache/typcache.c.

Yep.

> based on above information, i am still confused with
> cleanup_in_progress_typentries, in_progress_list_len
> is there any simple sql example to demo
> cleanup_in_progress_typentries, in_progress_list_len> 0.

I don't think there is simple sql to reliably reproduce that. In
order to hit that, we must process invalidation messages in some
(short) moment of time during lookup_type_cache(). You can reproduce
that by setting a breakpoint in lookup_type_cache() and in parallel do
something to invalidate the type cache entry (for instance, ALTER
TABLE ... ADD COLUMN ... would invalidate the composite type). In
principle, we can reproduce that using injection points. However, I'm
not intended to do that as long as we have buildfarm members with
-DCLOBBER_CACHE_ALWAYS. FWIW, I will for sure run tests with
-DCLOBBER_CACHE_ALWAYS before committing this.

------
Regards,
Alexander Korotkov
Supabase