Re: type cache cleanup improvements

On Mon, Aug 26, 2024 at 9:37 AM Andrei Lepikhov <lepihov(at)gmail(dot)com> wrote:
> On 25/8/2024 23:22, Alexander Korotkov wrote:
> > On Sun, Aug 25, 2024 at 10:21 PM Alexander Korotkov
> >>> (This Assert is introduced by c14d4acb8.)
> >>
> >> Thank you for noticing. I'm checking this.
> >
> > I didn't take into account that TypeCacheEntry could be invalidated
> > while lookup_type_cache() does syscache lookups. When I realized that
> > I was curious on how does it currently work. It appears that type
> > cache invalidation mostly only clears the flags while values are
> > remaining in place and still available for lookup_type_cache() caller.
> > TypeCacheEntry.tupDesc is invalidated directly, and it has guarantee
> > to survive only because we don't do any syscache lookups for composite
> > data types later in lookup_type_cache(). I'm becoming less fan of how
> > this works... I think these aspects needs to be at least documented
> > in details.
> >
> > Regarding c14d4acb8, it appears to require redesign. I'm going to revert it.
> Sorry, but I don't understand your point.
> Let's refocus on the problem at hand. The issue arose when the
> TypeCacheTypCallback and the TypeCacheRelCallback were executed in
> sequence within InvalidateSystemCachesExtended.
> The first callback cleaned the flags TCFLAGS_HAVE_PG_TYPE_DATA and
> TCFLAGS_CHECKED_DOMAIN_CONSTRAINTS. But the call of the second callback
> checks the typentry->tupDesc and, because it wasn't NULL, attempted to
> remove this record a second time.
> I think there is no case for redesign, but we have a mess in
> insertion/deletion conditions.

Yes, it's possible to repair the current approach. But we need to do
this correct, not just "not failing with current usages". Then we
need to call insert_rel_type_cache_if_needed() not just when we set
TCFLAGS_HAVE_PG_TYPE_DATA flag, but every time we set any of
TCFLAGS_OPERATOR_FLAGS or tupDesc. That's a lot of places, not as
simple and elegant as it was planned. This is why I wonder if there
is a better approach.

Secondly, I'm not terribly happy with current state of type cache.
The caller of lookup_type_cache() might get already invalidated data.
This probably OK, because caller probably hold locks on dependent
objects to guarantee that relevant properties of type actually
persists. At very least this should be documented, but it doesn't
seem so. Setting of tupdesc is sensitive to its order of execution.
That feels quite fragile to me, and not documented either. I think
this area needs improvements before we push additional functionality
there.

------
Regards,
Alexander Korotkov
Supabase