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

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