On Mon, Jun 28, 2010 at 10:53 AM, Tom Lane <[email protected]> wrote: > Robert Haas <[email protected]> writes: >> 2010/6/28 KaiGai Kohei <[email protected]>: >>> * at the RenameSchema() > >> This looks like another syscache reference leak. > > Actually that one *is* a leak, although it doesn't matter much because > the leak occurs only in an error path, so transaction abort will clean > up the leaked reference. Still, it's sucky coding style: > SearchSysCacheExists should have been used. > > I'm not sure I agree that replacing SearchSysCacheExists calls (or > things that should have been SearchSysCacheExists calls) with > OidIsValid(get_whatever_oid()) is an improvement. The Exists call > tells what you're actually trying to accomplish. The other way is > an overspecification of the required result.
It is, but on the other hand it's only good fortune that testing whether a schema exists is a one-liner even without using get_namespace_oid(). Take a look at get_tablespace_oid() in CVS HEAD [which BTW is already used in this style], or at get_trigger_oid() in the "get_whatever_oid, part 2" patch, for example. I think the assumption that system objects (with the exception of attributes) are identified by OIDs is embedded pretty deeply in the code, so I don't think it costs us much to rely on it. We're more likely to want to do things like add or remove a syscache, in which case a style that minimizes direct syscache references is apt to make life easier. It's also slightly less wordy, which I like, too. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company -- Sent via pgsql-hackers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
