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(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.
>

--
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>