Re: type cache cleanup improvements

On Mon, Aug 5, 2024 at 4:16 AM Alexander Korotkov <aekorotkov(at)gmail(dot)com> wrote:
> I've revised the patchset. First of all, I've re-ordered the patches.
>
> 0001-0002 (former 0002-0003)
> Comprises hash_search_with_hash_value() function and its application
> to avoid full hash iteration in InvalidateAttoptCacheCallback() and
> TypeCacheTypCallback(). I think this is quite straightforward
> optimization without negative side effects. I've revised comments,
> commit message and did some code beautification. I'm going to push
> this if no objections.
>
> 0003 (former 0001)
> I've revised this patch. I think main concerns expressed in the
> thread about this path is that we don't have invalidation mechanism
> for relid => typid map. Finally due to oid wraparound same relids
> could get reused. That could lead to invalid entries in the map about
> existing relids and typeids. This is rather messy, but I don't think
> this could cause a material bug. The maps items are used only for
> cache invalidation. Extra invalidation doesn't cause a bug. If type
> with same relid will be cached, then correspoding map item will be
> overridden, so no missing invalidation. However, I see the following
> reasons for keeping consistent state of relid => typid map.
>
> 1) As the main use-case for this optimization is flood of temporary
> tables, it would be nice not let relid => typid map bloat in this
> case. I see that TypeCacheHash would get bloated, because its entries
> are never deleted. However, I would prefer to not get this situation
> even worse.
> 2) In future we may find some more use-cases for relid => typid map
> besides cache invalidation. Keeping that in consistent state could be
> advantage then.
>
> In the attached patch, I'm keeping relid => typid map when
> corresponding typentry have either TCFLAGS_HAVE_PG_TYPE_DATA, or
> TCFLAGS_OPERATOR_FLAGS, or tupdesc. Thus, when temporary table gets
> deleted, we would invalidate the map item.
>
> It will be also nice to get rid of iteration over all the cached
> domain types in TypeCacheRelCallback(). However, this typically
> shouldn't be a problem since domain types are less tended to bloat.
> Domain types are created manually, unlike composite types which are
> automatically created for every temporary table. We will probably
> need to optimize this in future, but I don't feel this to be necessary
> in present patch.
>
> I think the revised 0003 requires review.

The rebased remaining patch is attached.

------
Regards,
Alexander Korotkov
Supabase