From: Tom Lane <tgl@sss.pgh.pa.us>
To: Michael Paquier <michael@paquier.xyz>
cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>, 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: <ZMCT1rszfKIiNLpk@paquier.xyz>
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>
 <1821881.1690339317@sss.pgh.pa.us> <ZMCT1rszfKIiNLpk@paquier.xyz>
Comments: In-reply-to Michael Paquier <michael@paquier.xyz>
	message dated "Wed, 26 Jul 2023 12:32:38 +0900"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1830959.1690343673.1@sss.pgh.pa.us>
Date: Tue, 25 Jul 2023 23:54:33 -0400
Message-ID: <1830960.1690343673@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1830960.1690343673%40sss.pgh.pa.us>
Precedence: bulk

Michael Paquier <michael@paquier.xyz> writes:
> FWIW, I'm having a hard time thinking about a reason that we should
> not support SearchSysCacheExists()+lookup to be a valid pattern, even
> if the cache is clobbered.  I am pretty sure that there are other code
> paths in the tree, not mentioned on this thread, that do exactly that
> (haven't checked, but indexcmds.c is one coming in mind).

Yeah.  As I see it, there are several questions here.

First, is the SearchSysCacheExists()+lookup pattern actually unsafe,
that is is there a way to break it without any cache-clobbering debug
options enabled?  If so, there's no question we have to get rid of it.

If it isn't really unsafe, then we have a choice whether to get rid of it
or adjust the cache-clobbering options to not fail.

I think the main argument for keeping it is exactly that we've probably
depended on the idea in multiple places, and finding them all might be
hard (and not re-introducing more later, even harder).

On the other hand, I have to concede that such coding patterns are
inherently fragile: you can't introduce any new opportunities for a
cache inval between the SearchSysCacheExists() and the lookup, or
there's definitely a real hazard --- which we might not find for
awhile, if this example is anything to go by.

These has_foo_privilege() functions are arguably a good example of the
least safe way to go about it, in that the SearchSysCacheExists and
the subsequent lookup aren't textually close or even in the same
module.  Nor do we have any comments warning against introducing more
logic into the critical code paths.

So another conclusion we could arrive at is that the pattern isn't
inherently unsafe, but we should only allow it when there's not much
code between the two calls, which would probably lead to wanting to
rewrite the has_foo_privilege() family after all.

I don't yet have an opinion about which way to go.

			regards, tom lane