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