Re: type cache cleanup improvements

Hi all,

On Fri, 13 Sept 2024 at 01:38, Alexander Korotkov <aekorotkov(at)gmail(dot)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