Horton

Re: type cache cleanup improvements

Alexander Korotkov <aekorotkov@gmail.com>

View thread
From: Alexander Korotkov <aekorotkov@gmail.com>
To: Artur Zakirov <zaartur@gmail.com>
Cc: 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-13T12:08:38Z
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.

Attachments

Hi, Arthur!

Thank you so much for your review!

On Thu, Oct 10, 2024 at 6:54 PM Artur Zakirov <zaartur@gmail.com> wrote:
> On Fri, 13 Sept 2024 at 01:38, Alexander Korotkov <aekorotkov@gmail.com> wrote:
> >
> > 0001 - adds comment about concurrent invalidation handling
> > 0002 - revised c14d4acb8.  Now we track type oids, whose
> > TypeCacheEntry's filing is in-progress.  Add entry to
> > RelIdToTypeIdCacheHash at the end of lookup_type_cache() or on the
> > transaction abort.  During invalidation don't assert
> > RelIdToTypeIdCacheHash to be here if TypeCacheEntry is in-progress.
>
> Thank you Alexander for the patch. I reviewed and tested it.
>
> I used Teodor's script to check the performance. On my laptop on
> master ROLLBACK runs 11496.219 ms. With patch ROLLBACK runs 378.990
> ms.
>
> It seems to me that there are couple of possible issues in the patch:
>
> In `lookup_type_cache()` `in_progress_list` is allocated using
> `CacheMemoryContext`, on the other hand it seems there might be a case
> when `CacheMemoryContext` is not created yet. It is created below in
> the code if it doesn't exist:
>
>     /* Also make sure CacheMemoryContext exists */
>     if (!CacheMemoryContext)
>         CreateCacheMemoryContext();
>
> It is probably a very rare case, but it might be better to allocate
> `in_progress_list` after that line, or move creation of
> `CacheMemoryContext` higher.

Yes, it makes sense to initialize `in_progress_list ` when
`CacheMemoryContext` is guaranteed to be initialized.  Fixed in the
attached patch.

> Within `insert_rel_type_cache_if_needed()` and
> `delete_rel_type_cache_if_needed()` there is an if condition:
>
>     if ((typentry->flags & TCFLAGS_HAVE_PG_TYPE_DATA) ||
>         (typentry->flags & TCFLAGS_OPERATOR_FLAGS) ||
>         typentry->tupDesc != NULL)
>
> Based on the logic of the rest of the code does it make sense to use
> TCFLAGS_DOMAIN_BASE_IS_COMPOSITE instead of TCFLAGS_OPERATOR_FLAGS?
> Otherwise the condition doesn't look logical.

I'm not sure I get the point.  This check ensures that type entry has
something to be cleared.  In this case we need to keep
RelIdToTypeIdCacheHash entry to find this item on invalidation
message.  I'm not sure how TCFLAGS_DOMAIN_BASE_IS_COMPOSITE is
relevant here, because it's valid only for TYPTYPE_DOMAIN while this
patch deals with TYPTYPE_COMPOSITE.

------
Regards,
Alexander Korotkov
Supabase