On Mon, Jun 28, 2010 at 10:53 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> 2010/6/28 KaiGai Kohei <kaigai@ak.jp.nec.com>:
>>> * 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