Re: BUG #18014: Releasing catcache entries makes schema_to_xmlschema() fail when parallel workers are used

Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> At Tue, 25 Jul 2023 13:00:00 +0300, Alexander Lakhin <exclusion@gmail.com> wrote in
>> 21.07.2023 22:21, Tom Lane wrote:
>>> There is no inval in the entry-already-present code path in syscache
>>> lookup.  So if we are seeing this failure, ISTM it must mean that an
>>> inval is happening during "close and unlock catalog", which seems like
>>> something that we don't want.  But I've not traced exactly how that
>>> happens.

>> Yes, but here we deal with -DCATCACHE_FORCE_RELEASE (added to config_env
>> on prion), so the cache entry, that was just found in
>> SearchSysCacheExists(), is removed immediately because of
>> SearchSysCacheExists() ->  ReleaseSysCache(tuple) ->
>> ReleaseCatCache(tuple).

Oh!  So are you saying that this case cannot happen in the wild
(that is, in a non-cache-clobbering build)?  If so, I think that
there's a good case to be made that the cache-clobbering behavior
is too strict, and we should change that (not sure just how) rather
than complicating callers that are perfectly safe in reality.

> Doesn't this imply that the function isn't parallel-safe? The issue is
> gone by marking it and all variants as parallel-restricted.

As I said earlier, I think that's a purely coincidental "fix" for
this specific manifestation.  Either SearchSysCacheExists followed
by a syscache lookup of the same tuple should be considered safe,
or it shouldn't.  If it should be considered safe, we need to fix the
cache-clobber test scaffolding to not give a false positive.  While if
it shouldn't, we need to get rid of that coding pattern, not apply
high-level band-aids that remove just one particular path to reaching
the problem.  I'm not dead set on either answer at this point, but
I think those are the plausible alternatives.

            regards, tom lane