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

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