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(at)ak(dot)jp(dot)nec(dot)com>