Re: type cache cleanup improvements

On Sat, Aug 31, 2024 at 10:33 PM Andrei Lepikhov <lepihov(at)gmail(dot)com> wrote:
> On 29/8/2024 11:01, Alexander Korotkov wrote:
> > On Mon, Aug 26, 2024 at 11:26 AM Alexander Korotkov
> >> 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.
> I think I understand the solution from the commit fdd965d074.
> Just for the record, you mentioned invalidation inside the
> lookup_type_cache above. Passing through the code, I found the only
> place for such a case - the call of the GetDefaultOpClass, which
> triggers the opening of the relation pg_opclass, which can cause an
> AcceptInvalidationMessages call. Did you mean this case, or does a wider
> field of cases exist here?

I've tried to implement handling of concurrent invalidation similar to
commit fdd965d074. However that appears to be more difficult that I
thought, because for some datatypes like arrays, ranges etc we might
need fill the element type and reference it. So, I decided to
continue with the current approach but borrowing some ideas from
fdd965d074. The revised patchset attached.

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.

------
Regards,
Alexander Korotkov
Supabase