Re: get_whatever_oid, part 1: object types with unqualifed names

Link to previous discussion:

http://archives.postgresql.org/pgsql-hackers/2010-05/msg01195.php

Attached, please find a patch which standardizes the following
interface for object types that use unqualifed names.

Oid get_whatever_oid(char *name, bool missing_ok);

It also refactors the existing code to use these functions whenever
possible. I'm going to work up a similar path for the object types
that use qualified names, but I thought it would be a good idea to
send this first before I invest too much time in it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

(2010/05/27 11:51), Robert Haas wrote:
> Link to previous discussion:
>
> http://archives.postgresql.org/pgsql-hackers/2010-05/msg01195.php
>
> Attached, please find a patch which standardizes the following
> interface for object types that use unqualifed names.
>
> Oid get_whatever_oid(char *name, bool missing_ok);
>
> It also refactors the existing code to use these functions whenever
> possible. I'm going to work up a similar path for the object types
> that use qualified names, but I thought it would be a good idea to
> send this first before I invest too much time in it.
>

This patch is not obviously small, but most part of the changeset are
mechanical. So, I could not find any other matters in this patch except
for the following three items.

* Patch format.

This patch uses the unified format, instead of the context format. :D

* ExecAlterOwnerStmt()
The original code uses get_roleid_checked() which does not allow invalid
username, but the new code gives missing_ok = true on the get_role_oid().
It should be fixed.

| --- a/src/backend/commands/alter.c
| +++ b/src/backend/commands/alter.c
| @@ -210,7 +210,7 @@ ExecAlterObjectSchemaStmt(AlterObjectSchemaStmt *stmt)
| void
| ExecAlterOwnerStmt(AlterOwnerStmt *stmt)
| {
| - Oid newowner = get_roleid_checked(stmt->newowner);
| + Oid newowner = get_role_oid(stmt->newowner, true);
|
| switch (stmt->objectType)
| {

* assign_temp_tablespaces()?
| @@ -1116,21 +1116,13 @@ assign_temp_tablespaces(const char *newval, bool doit, GucSource source)
| continue;
| }
|
| - /* Else verify that name is a valid tablespace name */
| - curoid = get_tablespace_oid(curname);
| + /*
| + * In an interactive SET command, we ereport for bad info.
| + * Otherwise, silently ignore any bad list elements.
| + */
| + curoid = get_tablespace_oid(curname, source < PGC_S_INTERACTIVE);
| if (curoid == InvalidOid)

It seems to me "if (!OidIsValid(curoid))" should be here, instead of comparison
with InvalidOid here. However, it may be cleaned up in any other patch instead
of get_whatever_oid() efforts.

| - {
| - /*
| - * In an interactive SET command, we ereport for bad info.
| - * Otherwise, silently ignore any bad list elements.
| - */
| - if (source >= PGC_S_INTERACTIVE)
| - ereport(ERROR,
| - (errcode(ERRCODE_UNDEFINED_OBJECT),
| - errmsg("tablespace \"%s\" does not exist",
| - curname)));
| continue;
| - }
|
| /*
| * Allow explicit specification of database's default tablespace

In addition, I could find out the following candidates to be replaced with
the new get_xxx_oid() APIs. Apart from whether these items should be cleaned
up in this patch at once, or not, it seems to me this patch can refactor the
following redundant codes.

* at the CreateRole(CreateRoleStmt *stmt)

| tuple = SearchSysCache1(AUTHNAME, PointerGetDatum(stmt->role));
| if (HeapTupleIsValid(tuple))
| ereport(ERROR,
| (errcode(ERRCODE_DUPLICATE_OBJECT),
| errmsg("role \"%s\" already exists",
| stmt->role)));

I saw similar code which was replaced with the new APIs in this patch.
It seems to me "if (OidIsValid(get_role_oid(stmt->role, true)))" can be used,
and it enables to write the code more clean.

* at the DefineOpFamily()

| /* Get necessary info about access method */
| tup = SearchSysCache1(AMNAME, CStringGetDatum(stmt->amname));
| if (!HeapTupleIsValid(tup))
| ereport(ERROR,
| (errcode(ERRCODE_UNDEFINED_OBJECT),
| errmsg("access method \"%s\" does not exist",
| stmt->amname)));
|
| amoid = HeapTupleGetOid(tup);
|
| /* XXX Should we make any privilege check against the AM? */
|
| ReleaseSysCache(tup);

It can be replaced by get_am_oid(stmt->amname, false).

* at the RenameSchema()

| /* make sure the new name doesn't exist */
| if (HeapTupleIsValid(
| SearchSysCache1(NAMESPACENAME,
| CStringGetDatum(newname))))
| ereport(ERROR,
| (errcode(ERRCODE_DUPLICATE_SCHEMA),
| errmsg("schema \"%s\" already exists", newname)));

It is similar to the case of CreateRole(). Does get_namespace_oid()
enables to write the code more clean?

Thanks,
--
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

2010/6/28 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> * ExecAlterOwnerStmt()
> The original code uses get_roleid_checked() which does not allow invalid
> username, but the new code gives missing_ok = true on the get_role_oid().
> It should be fixed.

Woops, good catch. Fixed.

> * assign_temp_tablespaces()?
> It seems to me "if (!OidIsValid(curoid))" should be here, instead of comparison
> with InvalidOid here. However, it may be cleaned up in any other patch instead
> of get_whatever_oid() efforts.

Yeah, we have a lot of places that check foo == InvalidOid or foo !=
InvalidOid rather than using OidIsValid(). That might or might not be
worth changing, but I don't see a strong need for this patch to change
this particular instance.

> * at the CreateRole(CreateRoleStmt *stmt)
>
> I saw similar code which was replaced with the new APIs in this patch.
> It seems to me "if (OidIsValid(get_role_oid(stmt->role, true)))" can be used,
> and it enables to write the code more clean.

I agree. Changed.

> * at the DefineOpFamily()
>
> |     /* Get necessary info about access method */
> |     tup = SearchSysCache1(AMNAME, CStringGetDatum(stmt->amname));
> |     if (!HeapTupleIsValid(tup))
> |         ereport(ERROR,
> |                 (errcode(ERRCODE_UNDEFINED_OBJECT),
> |                  errmsg("access method \"%s\" does not exist",
> |                         stmt->amname)));
> |
> |     amoid = HeapTupleGetOid(tup);
> |
> |     /* XXX Should we make any privilege check against the AM? */
> |
> |     ReleaseSysCache(tup);
>
> It can be replaced by get_am_oid(stmt->amname, false).

I agree, done. In fact, aren't we leaking a syscache reference here?
And if so, should we fix it in HEAD and the backbranches also? I'm
tempted not to bother because, after all, how often do you invoke
DefineOpFamily(), but maybe someone else has a different opinion?

> * at the RenameSchema()
>
> |     /* make sure the new name doesn't exist */
> |     if (HeapTupleIsValid(
> |                          SearchSysCache1(NAMESPACENAME,
> |                                          CStringGetDatum(newname))))
> |         ereport(ERROR,
> |                 (errcode(ERRCODE_DUPLICATE_SCHEMA),
> |                  errmsg("schema \"%s\" already exists", newname)));
>
> It is similar to the case of CreateRole(). Does get_namespace_oid()
> enables to write the code more clean?

This looks like another syscache reference leak. I have changed it to
use get_namespace_oid() in the patch, but we need to decide whether to
fix this in pre-9.1 releases.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I agree, done. In fact, aren't we leaking a syscache reference here?

How so? There's a ReleaseSysCache call.

regards, tom lane

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> 2010/6/28 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)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.

regards, tom lane

On Mon, Jun 28, 2010 at 10:48 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> I agree, done.  In fact, aren't we leaking a syscache reference here?
>
> How so?  There's a ReleaseSysCache call.

Oops, you're right.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Mon, Jun 28, 2010 at 10:53 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> 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().

True. Is it worth providing whatever_exists() macros that wrap
get_whatever_oid() like this, just so that callers are a bit clearer as
to what they're doing? I'm not certain though how many places it could
be used, since many callers probably actually do need the OID, and would
have to write separate lines anyway:

objoid = get_whatever_oid(...);
if (!OidIsValid(objoid))
ereport(...);
... code using objoid here ...

But it's been useful to have the SearchSysCacheExists functions, even
though they're just wrappers, so maybe whatever_exists() would be worth
its keep.

regards, tom lane

On Mon, Jun 28, 2010 at 12:17 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Mon, Jun 28, 2010 at 10:53 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> 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().
>
> True.  Is it worth providing whatever_exists() macros that wrap
> get_whatever_oid() like this, just so that callers are a bit clearer as
> to what they're doing?  I'm not certain though how many places it could
> be used, since many callers probably actually do need the OID, and would
> have to write separate lines anyway:
>
>        objoid = get_whatever_oid(...);
>        if (!OidIsValid(objoid))
>                ereport(...);
>        ... code using objoid here ...
>
> But it's been useful to have the SearchSysCacheExists functions, even
> though they're just wrappers, so maybe whatever_exists() would be worth
> its keep.

I haven't made a detailed study of this issue, so I'm not 100% sure.
My gut feeling however is that nearly all of the callers need the OID,
and that some of the whatever_exists() functions wouldn't have any
callers at all. Which makes me pretty hesitant to add them,
especially given our decision not to centralize all the
get_whatever_oid() functions in one place.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Mon, Jun 28, 2010 at 12:17 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> True. Is it worth providing whatever_exists() macros that wrap
>> get_whatever_oid() like this, just so that callers are a bit clearer as
>> to what they're doing?

> I haven't made a detailed study of this issue, so I'm not 100% sure.
> My gut feeling however is that nearly all of the callers need the OID,
> and that some of the whatever_exists() functions wouldn't have any
> callers at all. Which makes me pretty hesitant to add them,
> especially given our decision not to centralize all the
> get_whatever_oid() functions in one place.

Well, the whatever_exists() things would just be one-liner macros
declared in the same headers that declare the underlying
get_whatever_oid() functions. So the cost of carrying ones that happen
to not be used would be nil.

regards, tom lane

On Mon, Jun 28, 2010 at 12:31 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Mon, Jun 28, 2010 at 12:17 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> True.  Is it worth providing whatever_exists() macros that wrap
>>> get_whatever_oid() like this, just so that callers are a bit clearer as
>>> to what they're doing?
>
>> I haven't made a detailed study of this issue, so I'm not 100% sure.
>> My gut feeling however is that nearly all of the callers need the OID,
>> and that some of the whatever_exists() functions wouldn't have any
>> callers at all.  Which makes me pretty hesitant to add them,
>> especially given our decision not to centralize all the
>> get_whatever_oid() functions in one place.
>
> Well, the whatever_exists() things would just be one-liner macros
> declared in the same headers that declare the underlying
> get_whatever_oid() functions.  So the cost of carrying ones that happen
> to not be used would be nil.

True, but I think there's a pretty high chance that they woudn't get
used even in places where they ought to, for lack of existing
examples, or that new code would fail to add matching macros for new
object types. Even so, it's not a terrible idea; I just don't think
it should be a requirement for this particular patch. And to be
honest, I'd sort of like to see how this shakes out before going too
much further with it.

Another, and related idea that I had while looking at this is that a
lot of object types could benefit from a get_whatever_heaptuple()
function with the same calling syntax. get_whatever_oid() could be
restructured to use it, and most object types would have other
callers, also. But that too seems like opening a larger can of worms
than I really want to get into at this point.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Mon, Jun 28, 2010 at 12:31 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Well, the whatever_exists() things would just be one-liner macros
>> declared in the same headers that declare the underlying
>> get_whatever_oid() functions. So the cost of carrying ones that happen
>> to not be used would be nil.

> True, but I think there's a pretty high chance that they woudn't get
> used even in places where they ought to, for lack of existing
> examples, or that new code would fail to add matching macros for new
> object types. Even so, it's not a terrible idea; I just don't think
> it should be a requirement for this particular patch. And to be
> honest, I'd sort of like to see how this shakes out before going too
> much further with it.

Well, once you've finished the get_whatever_oid() patch it won't be
hard to count how many instances of OidIsValid(get_whatever_oid()) there
are. If there's more than a few then I think the macros would be
appropriate to provide.

> Another, and related idea that I had while looking at this is that a
> lot of object types could benefit from a get_whatever_heaptuple()
> function with the same calling syntax. get_whatever_oid() could be
> restructured to use it, and most object types would have other
> callers, also. But that too seems like opening a larger can of worms
> than I really want to get into at this point.

This is the sort of thing that I think we should get right the first
time, rather than have multiple waves of large-scale changes.

I'm actually inclined to think we should try to land this stuff in 9.0
if we're going to do it at all. As a new committer, I suspect you do
not realize exactly how much pain this sort of thing inflicts on
back-patchers. The SearchSysCache call convention changes have already
ensured that the 8.4 to 9.0 crossover is going to be a major, major PITA
for back-patching, probably nearly as bad as the 8.1 changes in
pgindent's comment wrapping rules. (If I had it to do over I'd have
vetoed those changes --- I don't even want to think about how many
man-days I've lost to that completely useless cosmetic change.) This
proposed change will touch many of the same places that were already
modified for that. It'd be nice to have only one version boundary
where we're manually adjusting back-patched fixes, and not two.

regards, tom lane

On Mon, Jun 28, 2010 at 1:00 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Another, and related idea that I had while looking at this is that a
>> lot of object types could benefit from a get_whatever_heaptuple()
>> function with the same calling syntax.  get_whatever_oid() could be
>> restructured to use it, and most object types would have other
>> callers, also.  But that too seems like opening a larger can of worms
>> than I really want to get into at this point.
>
> This is the sort of thing that I think we should get right the first
> time, rather than have multiple waves of large-scale changes.

After taking a walk around the block, I have two further thoughts about this:

1. I wouldn't have submitted these patches (this one and the part two
patch) unless it reflected my best judgment about how far it makes
sense to proceed with refactoring in a certain direction. I'm willing
to be second-guessed, if you or someone else wants to move the
goal-posts. But I don't personally feel that there's enough bang for
the buck in going further in this direction, or I would have done it
already. I'm not planning to commit these patches and then start
immediately working on a second set of patches that touches all of
these same spots in the code over again. In the particular area that
this patch touches (mapping names as gathered by the parser to OIDs),
there are many different permutations: the input can be a cstring, or
a text datum, or a list of cstrings, etc.; and the output can be a
boolean (is it there?), an OID, a heap tuple, etc. If you try to
cover every combination, especially for the more obscure object types,
you'll drive yourself nuts; on the other hand, trying to regularize
the more common cases is, I think, helpful and worthwhile. So it's a
trade-off; I took my best crack at it.

2. It might be too optimistic to think that we're going to avoid
having large-scale code changes in 9.1 by committing these to 9.0. I
think refactoring is a fact of life as we try to move the project
forward, and while we want to be careful about how we do it for the
reasons you mention, it's also important if we want to have a clean
base to build on for future features (which, in fact, is why I
proposed these patches in the first place - I discovered that this
code wasn't too clean right now while thinking about SE-PostgreSQL
security labels at PGCon). I have at least one other patch that's
basically just refactoring in the queue for 9.1 already, which is
fairly wide-ranging but in an area completely unrelated to these
patches, and a couple other less ambitious ones that I plan to work on
as time permits; so I think that the need for these kinds of changes
is not going to go away.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

(2010/06/29 3:52), Robert Haas wrote:
> On Mon, Jun 28, 2010 at 1:00 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Another, and related idea that I had while looking at this is that a
>>> lot of object types could benefit from a get_whatever_heaptuple()
>>> function with the same calling syntax. get_whatever_oid() could be
>>> restructured to use it, and most object types would have other
>>> callers, also. But that too seems like opening a larger can of worms
>>> than I really want to get into at this point.
>>
>> This is the sort of thing that I think we should get right the first
>> time, rather than have multiple waves of large-scale changes.
>
> After taking a walk around the block, I have two further thoughts about this:
>
> 1. I wouldn't have submitted these patches (this one and the part two
> patch) unless it reflected my best judgment about how far it makes
> sense to proceed with refactoring in a certain direction. I'm willing
> to be second-guessed, if you or someone else wants to move the
> goal-posts. But I don't personally feel that there's enough bang for
> the buck in going further in this direction, or I would have done it
> already. I'm not planning to commit these patches and then start
> immediately working on a second set of patches that touches all of
> these same spots in the code over again. In the particular area that
> this patch touches (mapping names as gathered by the parser to OIDs),
> there are many different permutations: the input can be a cstring, or
> a text datum, or a list of cstrings, etc.; and the output can be a
> boolean (is it there?), an OID, a heap tuple, etc. If you try to
> cover every combination, especially for the more obscure object types,
> you'll drive yourself nuts; on the other hand, trying to regularize
> the more common cases is, I think, helpful and worthwhile. So it's a
> trade-off; I took my best crack at it.
>
Sorry, I couldn't understand some of idiomatic expressions, but you are
saying that we have many variations of syscaches, so it is nonsense to
wrap up all the usecases of them, but being worthful to focus on major
usecases, such as name to oid translation, aren't you?

If so, it also seems to me fair enough. It seems to me translations from
name into oid, tuple or verifying object existence are major use cases,
but we have no reason why replace all the SysCacheSearch().

> 2. It might be too optimistic to think that we're going to avoid
> having large-scale code changes in 9.1 by committing these to 9.0. I
> think refactoring is a fact of life as we try to move the project
> forward, and while we want to be careful about how we do it for the
> reasons you mention, it's also important if we want to have a clean
> base to build on for future features (which, in fact, is why I
> proposed these patches in the first place - I discovered that this
> code wasn't too clean right now while thinking about SE-PostgreSQL
> security labels at PGCon). I have at least one other patch that's
> basically just refactoring in the queue for 9.1 already, which is
> fairly wide-ranging but in an area completely unrelated to these
> patches, and a couple other less ambitious ones that I plan to work on
> as time permits; so I think that the need for these kinds of changes
> is not going to go away.
>

--
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

2010/6/28 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> Sorry, I couldn't understand some of idiomatic expressions, but you are
> saying that we have many variations of syscaches, so it is nonsense to
> wrap up all the usecases of them, but being worthful to focus on major
> usecases, such as name to oid translation, aren't you?

Yes.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Robert,

I checked the v2 patch, but I could not find anything to comment
except for its patch format.
The -v2 patch still uses unified format, instead of context format.
I believe it is just a careless miss. Please fix it later, if necessary.

When I submit a patch, I makes it as follows :D
git diff master my_branch | filterdiff --format=context > my_feature.patch

The transitions from object name to its oid are widely used in
the routines of the backend, so I also think this reworks will
enough worthwhile to keep the code clean.

So, I marked it as "ready for committer".

Thanks,

(2010/06/28 22:55), Robert Haas wrote:
> 2010/6/28 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> * ExecAlterOwnerStmt()
>> The original code uses get_roleid_checked() which does not allow invalid
>> username, but the new code gives missing_ok = true on the get_role_oid().
>> It should be fixed.
>
> Woops, good catch. Fixed.
>
>> * assign_temp_tablespaces()?
>> It seems to me "if (!OidIsValid(curoid))" should be here, instead of comparison
>> with InvalidOid here. However, it may be cleaned up in any other patch instead
>> of get_whatever_oid() efforts.
>
> Yeah, we have a lot of places that check foo == InvalidOid or foo !=
> InvalidOid rather than using OidIsValid(). That might or might not be
> worth changing, but I don't see a strong need for this patch to change
> this particular instance.
>
>> * at the CreateRole(CreateRoleStmt *stmt)
>>
>> I saw similar code which was replaced with the new APIs in this patch.
>> It seems to me "if (OidIsValid(get_role_oid(stmt->role, true)))" can be used,
>> and it enables to write the code more clean.
>
> I agree. Changed.
>
>> * at the DefineOpFamily()
>>
>> | /* Get necessary info about access method */
>> | tup = SearchSysCache1(AMNAME, CStringGetDatum(stmt->amname));
>> | if (!HeapTupleIsValid(tup))
>> | ereport(ERROR,
>> | (errcode(ERRCODE_UNDEFINED_OBJECT),
>> | errmsg("access method \"%s\" does not exist",
>> | stmt->amname)));
>> |
>> | amoid = HeapTupleGetOid(tup);
>> |
>> | /* XXX Should we make any privilege check against the AM? */
>> |
>> | ReleaseSysCache(tup);
>>
>> It can be replaced by get_am_oid(stmt->amname, false).
>
> I agree, done. In fact, aren't we leaking a syscache reference here?
> And if so, should we fix it in HEAD and the backbranches also? I'm
> tempted not to bother because, after all, how often do you invoke
> DefineOpFamily(), but maybe someone else has a different opinion?
>
>> * at the RenameSchema()
>>
>> | /* make sure the new name doesn't exist */
>> | if (HeapTupleIsValid(
>> | SearchSysCache1(NAMESPACENAME,
>> | CStringGetDatum(newname))))
>> | ereport(ERROR,
>> | (errcode(ERRCODE_DUPLICATE_SCHEMA),
>> | errmsg("schema \"%s\" already exists", newname)));
>>
>> It is similar to the case of CreateRole(). Does get_namespace_oid()
>> enables to write the code more clean?
>
> This looks like another syscache reference leak. I have changed it to
> use get_namespace_oid() in the patch, but we need to decide whether to
> fix this in pre-9.1 releases.
>

--
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

2010/7/2 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> The transitions from object name to its oid are widely used in
> the routines of the backend, so I also think this reworks will
> enough worthwhile to keep the code clean.
>
> So, I marked it as "ready for committer".

Thanks. Are you planning to review the part 2 patch also?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

2010/7/2 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> So, I marked it as "ready for committer".

There is a minor merge conflict with this patch as a result of a patch
I committed last week. New version attached.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company