mapping object names to role IDs

Suppose you have an object name as a CString and you want to convert
it to an OID. The method of doing this varies widely depending on the
object type:

oid = get_database_oid(name);
oid = get_tablespace_oid(name);
oid = GetForeignDataWrapperOidByName(name, true);
oid = GetForeignServerOidByName(name, true);
oid = LookupFuncNameTypeNames(name, args, true);
oid = LookupAggNameTypeNames(name, args, true);
oid = LookupFuncNameTypeNames(name, args, true);
oid = LookupOperNameTypeNames(name, args, true);
oid = GetSysCacheOid1(LANGNAME, CStringGetDatum(name));
oid = GetSysCacheOid1(NAMESPACENAME, CStringGetDatum(name));
oid = GetSysCacheOid1(AMNAME, CStringGetDatum(name));
oid = get_roleid(name);
oid = GetConstraintByName(reloid, name);
oid = FindConversionByName(list_make1(name));
oid = TSDictionaryGetDictid(list_make1(name), true);
oid = TSTemplateGetTmplid(list_make1(name), true);
oid = TSConfigGetCfgid(list_make1(name), true);

If you'd like to throw an error if the object doesn't exist, then you
can change the "true" parameter in the above examples to false, where
it exists, or you can use get_roleid_checked for roles. For the types
where a direct syscache lookup is used, you get to write the check
yourself. For constraints, GetConstraintByName throws an error
anyway; there's no equivalent that doesn't. Some other object types -
like rules and casts - have even more complex logic that is sometimes
duplicated in multiple places; and others (like tablespaces) that have
convenience functions still manage to duplicate the logic anyway.
Long story short, this is kind of a mess.

Looking at the different cases, there seem to be three main cases:

1. Objects that just have one name, like databases and procedural languages.
2. Objects that have possibly-qualified names, like conversions and
text search dictionaries.
3. Objects that have both names and argument, like functions and operators.

There's enough complexity about the stuff in category #3 that I'm
inclined to leave it alone, but try to make the other stuff more
standardized. What I would propose is that we create a new source
file somewhere (maybe utils/cache), move all of the other functions of
this type there, give them standardized names, and provide them all
with an argument that specifies whether an error is to be thrown if
the object doesn't exist. Begin bikeshedding here. I suggest that
the names be get_<object-type>_oid and that they take two parameters,
either a List * for possibly-qualified names (a little wonky, but it's
what we do now) or a char * for unqualified names, and a boolean
indicating whether to throw an error if the object isn't found. Thus:

Oid get_<object-type>_oid(List *qualname, bool missingok);
-or-
Oid get_<object-type>_oid(char *name, bool missingok);

Thus get_database_oid and get_tablespace_oid would remain unchanged
except for taking a second argument, get_roleid and get_roleid_checked
would merge, and all of the others would change to match that style.

Thoughts?

P.S. Purple.

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

And of course I meant for the subject line to be "mapping object names
to OIDs", not "role IDs".

Sigh.

...Robert

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> Long story short, this is kind of a mess.

I agree that it's a bit of a mess.

> What I would propose is that we create a new source
> file somewhere (maybe utils/cache), move all of the other functions of
> this type there, give them standardized names, and provide them all
> with an argument that specifies whether an error is to be thrown if
> the object doesn't exist.

Something which has come up in the past is that putting all the
functions that do the same kind of thing, but operate on different
types of objects, into the same backend file/area ends up meaning that
such an area has an untold amount of knowledge about everything.
Additionally, what *does* go into said area has tons of things that are
only related by the kind of operation- not because they actually have
anything to do with each other.

This was one of the complaints levied at the approach for moving all the
ACL checking into one place. I think it would be good to have a
consistant naming/calling scheme for these various functions, but I'm
not sure that moving them all to the same place makes sense.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
>> Long story short, this is kind of a mess.

> ... I think it would be good to have a
> consistant naming/calling scheme for these various functions, but I'm
> not sure that moving them all to the same place makes sense.

I'm with Stephen on this one. I agree that standardizing the function
names and behavior would be a good idea, but don't try to put them all
in one place.

BTW, the plain-name cases should be "const char *", else some callers
will have to cast away const. You could possibly make an argument for
"const List *" in the qualified-name cases, but we don't do that
anywhere else so I think it'd just look funny here (and would require
internally casting away const, too).

regards, tom lane

On Sun, May 23, 2010 at 11:10 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
>> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
>>> Long story short, this is kind of a mess.
>
>> ... I think it would be good to have a
>> consistant naming/calling scheme for these various functions, but I'm
>> not sure that moving them all to the same place makes sense.
>
> I'm with Stephen on this one.  I agree that standardizing the function
> names and behavior would be a good idea, but don't try to put them all
> in one place.

Some of the existing functions are in lsyscache.c, some are in files
in the commands directory, and some are in files in the parser
directory; also, even between commands and parser, not every object
type has its own file. It would be nice to bring some consistency to
where the functions are located as well as what they do. Any thoughts
on how to achieve that?

> BTW, the plain-name cases should be "const char *", else some callers
> will have to cast away const.  You could possibly make an argument for
> "const List *" in the qualified-name cases, but we don't do that
> anywhere else so I think it'd just look funny here (and would require
> internally casting away const, too).

Makes sense.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Sun, May 23, 2010 at 11:10 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I'm with Stephen on this one. I agree that standardizing the function
>> names and behavior would be a good idea, but don't try to put them all
>> in one place.

> Some of the existing functions are in lsyscache.c, some are in files
> in the commands directory, and some are in files in the parser
> directory; also, even between commands and parser, not every object
> type has its own file. It would be nice to bring some consistency to
> where the functions are located as well as what they do. Any thoughts
> on how to achieve that?

I think both Stephen and I are saying we don't see merit in that.
Moving around pre-existing functions won't accomplish much except
causing include-list churn. Let's just standardize the names/APIs
and be done.

regards, tom lane

On Sun, May 23, 2010 at 11:30 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Sun, May 23, 2010 at 11:10 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> I'm with Stephen on this one.  I agree that standardizing the function
>>> names and behavior would be a good idea, but don't try to put them all
>>> in one place.
>
>> Some of the existing functions are in lsyscache.c, some are in files
>> in the commands directory, and some are in files in the parser
>> directory; also, even between commands and parser, not every object
>> type has its own file.  It would be nice to bring some consistency to
>> where the functions are located as well as what they do.  Any thoughts
>> on how to achieve that?
>
> I think both Stephen and I are saying we don't see merit in that.
> Moving around pre-existing functions won't accomplish much except
> causing include-list churn.  Let's just standardize the names/APIs
> and be done.

Where do we put the new functions?

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Sun, May 23, 2010 at 11:30 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I think both Stephen and I are saying we don't see merit in that.
>> Moving around pre-existing functions won't accomplish much except
>> causing include-list churn. Let's just standardize the names/APIs
>> and be done.

> Where do we put the new functions?

Probably in files that are already concerned with each object type.

regards, tom lane

On Sun, May 23, 2010 at 1:39 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Sun, May 23, 2010 at 11:30 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> I think both Stephen and I are saying we don't see merit in that.
>>> Moving around pre-existing functions won't accomplish much except
>>> causing include-list churn.  Let's just standardize the names/APIs
>>> and be done.
>
>> Where do we put the new functions?
>
> Probably in files that are already concerned with each object type.

Not every object type has a file, and the existing functions are split
across three different directories, sometimes in files that don't
really pertain to the object type being dealt with. I think this is
going to be difficult to maintain if we intentionally spread out the
parallel code across essentially the entire backend. But I guess I
can code it up and we can argue about it then.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Not every object type has a file, and the existing functions are split
> across three different directories, sometimes in files that don't
> really pertain to the object type being dealt with. I think this is
> going to be difficult to maintain if we intentionally spread out the
> parallel code across essentially the entire backend. But I guess I
> can code it up and we can argue about it then.

The only thing that seems really significantly parallel is the error
message to be issued for object-not-found. I would suggest maybe
putting the code in lsyscache.c, except that lsyscache functions
generally are not expected to throw error on object-not-found.

As for "not every object type has a file", there is certainly code
someplace that would be calling these functions. Whereever (the
preponderance of) such calls are would be an appropriate place for the
function, IMO.

regards, tom lane

On sön, 2010-05-23 at 00:50 -0400, Robert Haas wrote:
> Oid get_<object-type>_oid(List *qualname, bool missingok);
> -or-
> Oid get_<object-type>_oid(char *name, bool missingok);
>
> Thus get_database_oid and get_tablespace_oid would remain unchanged
> except for taking a second argument, get_roleid and get_roleid_checked
> would merge, and all of the others would change to match that style.

If you are doing some refactoring work in that area, maybe you can also
take care of the issue I talked about there:
http://archives.postgresql.org/pgsql-hackers/2008-12/msg00725.php

"""
Our code contains about 200 copies of the following code:

tuple = SearchSysCache[Copy](FOOOID, ObjectIdGetDatum(fooid), 0, 0, 0);
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup failed for foo %u", fooid);
"""

It looks like your proposal would reduce that number significantly.

On Wed, May 26, 2010 at 5:27 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On sön, 2010-05-23 at 00:50 -0400, Robert Haas wrote:
>> Oid get_<object-type>_oid(List *qualname, bool missingok);
>> -or-
>> Oid get_<object-type>_oid(char *name, bool missingok);
>>
>> Thus get_database_oid and get_tablespace_oid would remain unchanged
>> except for taking a second argument, get_roleid and get_roleid_checked
>> would merge, and all of the others would change to match that style.
>
> If you are doing some refactoring work in that area, maybe you can also
> take care of the issue I talked about there:
> http://archives.postgresql.org/pgsql-hackers/2008-12/msg00725.php
>
> """
> Our code contains about 200 copies of the following code:
>
> tuple = SearchSysCache[Copy](FOOOID, ObjectIdGetDatum(fooid), 0, 0, 0);
> if (!HeapTupleIsValid(tuple))
>    elog(ERROR, "cache lookup failed for foo %u", fooid);
> """
>
> It looks like your proposal would reduce that number significantly.

Well, not directly, because I was proposing to do something about
wanting to turn an object identified by name into an OID, rather than
wanting to look up an OID and find a syscache tuple. But the same
approach could be applied to the problem you mention.

I still feel that we'd be better off putting all the functions that
use the same design pattern in a single file, rather than spreading
them out all over the backend. It's true that that one file will then
depend on all the catalog stuff, but it actually can limit
dependencies a little bit on the other end, because if someone wants
to call a bunch of these functions from the same file, they only need
to include the one header where they are all declared, rather than all
the individual files that contain the individual functions. And more
to the point, it's way easier from a maintenance standpoint: there is
much less chance that someone will change one function without
changing all the others if they are all in the same place.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I still feel that we'd be better off putting all the functions that
> use the same design pattern in a single file, rather than spreading
> them out all over the backend. It's true that that one file will then
> depend on all the catalog stuff, but it actually can limit
> dependencies a little bit on the other end, because if someone wants
> to call a bunch of these functions from the same file, they only need
> to include the one header where they are all declared,

This is nonsense, because the call sites are going to be places that are
actually *doing* something with that catalog, and so will need not only
the catalog .h file but any other support functions associated with
doing work on that catalog. Centralizing the lookups will do nothing
whatsoever to reduce dependencies; it'll just create a central file
dependent on everything in addition to every dependency we have now.

The closest equivalent we have now is lsyscache.c, which is not exactly
a sterling example of how to design a module: it's got no conceptual
consistency whatsoever.

I'm for standardizing the API of lookup functions, but not for
relocating them.

regards, tom lane

On Wed, May 26, 2010 at 9:45 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> This is nonsense

You can assert that, but I don't agree. We certainly have places
(comment.c being the obvious example) where we need to look up a name
and map it to an OID without doing anything else, and actually I
believe there are useful ways to refactor the code that might lead to
more of this. Anyway, I think the code maintenance argument ought to
carry a lot more weight than whether one or two small files get
rebuilt for dependencies slightly more often. lsyscache.c might have
no conceptual consistency but it's extremely useful, and there are
plenty of other examples of where we've put code for different object
types into a single file to simplify maintenance and reduce code
complexity (e.g. copyfuncs, equalfuncs, outfuncs, etc.).

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

Excerpts from Robert Haas's message of mié may 26 07:20:30 -0400 2010:

> I still feel that we'd be better off putting all the functions that
> use the same design pattern in a single file, rather than spreading
> them out all over the backend. It's true that that one file will then
> depend on all the catalog stuff, but it actually can limit
> dependencies a little bit on the other end, because if someone wants
> to call a bunch of these functions from the same file, they only need
> to include the one header where they are all declared, rather than all
> the individual files that contain the individual functions.

This doesn't buy you anything, because that one header will likely have
to #include all the other headers anyway. And if this is so, then all
those headers will now be included in all files that require even a
single one of these functions.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Wed, May 26, 2010 at 11:01 AM, alvherre <alvherre(at)commandprompt(dot)com> wrote:
> Excerpts from Robert Haas's message of mié may 26 07:20:30 -0400 2010:
>
>> I still feel that we'd be better off putting all the functions that
>> use the same design pattern in a single file, rather than spreading
>> them out all over the backend.  It's true that that one file will then
>> depend on all the catalog stuff, but it actually can limit
>> dependencies a little bit on the other end, because if someone wants
>> to call a bunch of these functions from the same file, they only need
>> to include the one header where they are all declared, rather than all
>> the individual files that contain the individual functions.
>
> This doesn't buy you anything, because that one header will likely have
> to #include all the other headers anyway.  And if this is so, then all
> those headers will now be included in all files that require even a
> single one of these functions.

Well, at any rate, I'm giving up on the argument.

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

alvherre <alvherre(at)commandprompt(dot)com> writes:
> Excerpts from Robert Haas's message of mi may 26 07:20:30 -0400 2010:
>> I still feel that we'd be better off putting all the functions that
>> use the same design pattern in a single file, rather than spreading
>> them out all over the backend.

> This doesn't buy you anything, because that one header will likely have
> to #include all the other headers anyway. And if this is so, then all
> those headers will now be included in all files that require even a
> single one of these functions.

For the particular case Robert is proposing, the *header* isn't a
problem, because the only types it would deal in are Oid, bool,
const char *, and List *. But you're right that in general this design
pattern carries a risk of having to include the world in a commonly-used
header file, which is certainly not a good idea.

regards, tom lane

On Wed, May 26, 2010 at 1:27 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> alvherre <alvherre(at)commandprompt(dot)com> writes:
>> Excerpts from Robert Haas's message of mié may 26 07:20:30 -0400 2010:
>>> I still feel that we'd be better off putting all the functions that
>>> use the same design pattern in a single file, rather than spreading
>>> them out all over the backend.
>
>> This doesn't buy you anything, because that one header will likely have
>> to #include all the other headers anyway.  And if this is so, then all
>> those headers will now be included in all files that require even a
>> single one of these functions.
>
> For the particular case Robert is proposing, the *header* isn't a
> problem, because the only types it would deal in are Oid, bool,
> const char *, and List *.  But you're right that in general this design
> pattern carries a risk of having to include the world in a commonly-used
> header file, which is certainly not a good idea.

Right. I am very cognizant of the problem, but it isn't really an
issue in this case.

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

Excerpts from Robert Haas's message of mié may 26 10:34:00 -0400 2010:

> lsyscache.c might have no conceptual consistency but it's extremely
> useful,

I know I've been annoyed by lsyscache: looking for accessors to catalog
stuff, not finding them and so creating my own by using syscache
directly, only to find out later that they already existed there.
I think we should be moving in the direction of *removing* lsyscache,
not replicating it.

BTW I quite agree with both the suggestion you give in this thread
(modulo this issue), and Peter's idea of getting rid of the repetitive
syscache coding pattern.

> and there are
> plenty of other examples of where we've put code for different object
> types into a single file to simplify maintenance and reduce code
> complexity (e.g. copyfuncs, equalfuncs, outfuncs, etc.).

Well, that's all related to node manipulation, so I'm not so sure it's
exactly the same.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support