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