Re: type cache cleanup improvements

On Mon, Aug 26, 2024 at 11:26 AM Alexander Korotkov
<aekorotkov@gmail.com> wrote:
>
> On Mon, Aug 26, 2024 at 9:37 AM Andrei Lepikhov <lepihov@gmail.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.

I see fdd965d074 added a proper handling for concurrent invalidation
for relation cache. If a concurrent invalidation occurs, we retry
building a relation descriptor.  Thus, we end up with returning of a
valid relation descriptor to caller.  I wonder if we can take the same
approach to type cache.  That would make the whole type cache more
consistent and less fragile.  Also, this patch will be simpler.

------
Regards,
Alexander Korotkov
Supabase