From: Tom Lane <tgl@sss.pgh.pa.us>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
cc: exclusion@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18014: Releasing catcache entries makes
 schema_to_xmlschema() fail when parallel workers are used
In-reply-to: <20230726.112923.27361680552823861.horikyota.ntt@gmail.com>
References: <00e24aa6-5d03-99b6-ab41-495a35fc93f6@gmail.com>
 <982076.1689967306@sss.pgh.pa.us>
 <5bd4cbfe-fc71-c576-851d-3bb21ef5c080@gmail.com>
 <20230726.112923.27361680552823861.horikyota.ntt@gmail.com>
Comments: In-reply-to Kyotaro Horiguchi <horikyota.ntt@gmail.com>
	message dated "Wed, 26 Jul 2023 11:29:23 +0900"
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-ID: <1821880.1690339317.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Tue, 25 Jul 2023 22:41:57 -0400
Message-ID: <1821881.1690339317@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1821881.1690339317%40sss.pgh.pa.us>
Precedence: bulk

Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> At Tue, 25 Jul 2023 13:00:00 +0300, Alexander Lakhin <exclusion@gmail.co=
m> 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_en=
v
>> on prion), so the cache entry, that was just found in
>> SearchSysCacheExists(), is removed immediately because of
>> SearchSysCacheExists() ->=C2=A0 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