Re: type cache cleanup improvements

Artur Zakirov <zaartur@gmail.com>

From: Artur Zakirov <zaartur@gmail.com>

To: Alexander Korotkov <aekorotkov@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-10T15:54:01Z

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 →

Maintain RelIdToTypeIdCacheHash in TypeCacheOpcCallback()
- bb78e4267817 18.0 landed
Fix concurrrently in typcache_rel_type_cache.sql
- aa1e898dea66 18.0 landed
Avoid looping over all type cache entries in TypeCacheRelCallback()
- b85a9d046efd 18.0 landed
- c14d4acb8134 18.0 landed
Update header comment for lookup_type_cache()
- c1500a1ba7e1 18.0 landed
Revert: Avoid looping over all type cache entries in TypeCacheRelCallback()
- 8daa62a10c91 18.0 landed
Introduce hash_search_with_hash_value() function
- d0f020037e19 18.0 landed
Optimize InvalidateAttoptCacheCallback() and TypeCacheTypCallback()
- 40064a8ee1b3 18.0 landed
Refactor initial hash lookup in dynahash.c
- cc5ef90edd80 17.0 landed
Rationalize and improve error messages for some jsonpath items
- 92d2ab7554f9 17.0 cited
Avoid race in RelationBuildDesc() affecting CREATE INDEX CONCURRENTLY.
- fdd965d074d4 15.0 cited
Represent Lists as expansible arrays, not chains of cons-cells.
- 1cff1b95ab6d 13.0 cited

Hi all,

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.

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.

-- 
Kind regards,
Artur