Re: type cache cleanup improvements

Hi, Arthur!

Thank you so much for your review!

On Thu, Oct 10, 2024 at 6:54 PM Artur Zakirov <zaartur(at)gmail(dot)com> wrote:
> 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.

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