Horton

Re: type cache cleanup improvements

Alexander Korotkov <aekorotkov@gmail.com>

View thread
From: Alexander Korotkov <aekorotkov@gmail.com>
To: Andrei Lepikhov <lepihov@gmail.com>
Cc: 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-08-26T08:26:26Z
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 Mon, Aug 26, 2024 at 9:37 AM Andrei Lepikhov <lepihov@gmail.com> wrote:
> On 25/8/2024 23:22, Alexander Korotkov wrote:
> > On Sun, Aug 25, 2024 at 10:21 PM Alexander Korotkov
> >>> (This Assert is introduced by c14d4acb8.)
> >>
> >> Thank you for noticing.  I'm checking this.
> >
> > I didn't take into account that TypeCacheEntry could be invalidated
> > while lookup_type_cache() does syscache lookups.  When I realized that
> > I was curious on how does it currently work.  It appears that type
> > cache invalidation mostly only clears the flags while values are
> > remaining in place and still available for lookup_type_cache() caller.
> > TypeCacheEntry.tupDesc is invalidated directly, and it has guarantee
> > to survive only because we don't do any syscache lookups for composite
> > data types later in lookup_type_cache().  I'm becoming less fan of how
> > this works...  I think these aspects needs to be at least documented
> > in details.
> >
> > Regarding c14d4acb8, it appears to require redesign.  I'm going to revert it.
> Sorry, but I don't understand your point.
> Let's refocus on the problem at hand. The issue arose when the
> TypeCacheTypCallback and the TypeCacheRelCallback were executed in
> sequence within InvalidateSystemCachesExtended.
> The first callback cleaned the flags TCFLAGS_HAVE_PG_TYPE_DATA and
> TCFLAGS_CHECKED_DOMAIN_CONSTRAINTS. But the call of the second callback
> checks the typentry->tupDesc and, because it wasn't NULL, attempted to
> remove this record a second time.
> I think there is no case for redesign, but we have a mess in
> insertion/deletion conditions.

Yes, it's possible to repair the current approach.  But we need to do
this correct, not just "not failing with current usages".  Then we
need to call insert_rel_type_cache_if_needed() not just when we set
TCFLAGS_HAVE_PG_TYPE_DATA flag, but every time we set any of
TCFLAGS_OPERATOR_FLAGS or tupDesc.  That's a lot of places, not as
simple and elegant as it was planned.  This is why I wonder if there
is a better approach.

Secondly, I'm not terribly happy with current state of type cache.
The caller of lookup_type_cache() might get already invalidated data.
This probably OK, because caller probably hold locks on dependent
objects to guarantee that relevant properties of type actually
persists.  At very least this should be documented, but it doesn't
seem so.  Setting of tupdesc is sensitive to its order of execution.
That feels quite fragile to me, and not documented either.  I think
this area needs improvements before we push additional functionality
there.

------
Regards,
Alexander Korotkov
Supabase