Horton

Re: type cache cleanup improvements

Alexander Korotkov <aekorotkov@gmail.com>

View thread
From: Alexander Korotkov <aekorotkov@gmail.com>
To: jian he <jian.universality@gmail.com>
Cc: Artur Zakirov <zaartur@gmail.com>, Andrei Lepikhov <lepihov@gmail.com>, Alexander Lakhin <exclusion@gmail.com>, Pavel Borisov <pashkin.elfe@gmail.com>, Teodor Sigaev <teodor@sigaev.ru>, Pgsql Hackers <pgsql-hackers@postgresql.org>, Aleksander Alekseev <aleksander@timescale.com>, Tom Lane <tgl@sss.pgh.pa.us>, Michael Paquier <michael@paquier.xyz>
Date: 2024-10-20T17:47:28Z
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. Maintain RelIdToTypeIdCacheHash in TypeCacheOpcCallback()

  2. Fix concurrrently in typcache_rel_type_cache.sql

  3. Avoid looping over all type cache entries in TypeCacheRelCallback()

  4. Update header comment for lookup_type_cache()

  5. Revert: Avoid looping over all type cache entries in TypeCacheRelCallback()

  6. Introduce hash_search_with_hash_value() function

  7. Optimize InvalidateAttoptCacheCallback() and TypeCacheTypCallback()

  8. Refactor initial hash lookup in dynahash.c

  9. Rationalize and improve error messages for some jsonpath items

  10. Avoid race in RelationBuildDesc() affecting CREATE INDEX CONCURRENTLY.

  11. Represent Lists as expansible arrays, not chains of cons-cells.

On Tue, Oct 15, 2024 at 12:50 PM jian he <jian.universality@gmail.com> wrote:
>
> On Tue, Oct 15, 2024 at 4:09 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> >
> > Hi, Jian!
> >
> > Thank you for your review.
> >
> > On Tue, Oct 15, 2024 at 10:34 AM jian he <jian.universality@gmail.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