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

(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@ak.jp.nec.com>