Re: get_whatever_oid, part 2 - Mailing list pgsql-hackers

Sorry for the late responding.

It looks good to me that the point (1) and (4) are fixed.

The (2) and (3) are depending on individual subjective sense,
so it will be no problem, if we decide not to clean them up at
the same time.

Elsewhere, I noticed one other stuff related to naming convention.

The new internal APIs are basically named as: get_(object class)_oid(<args...>);

At the part 1 patch, the object class was entirely matched with
name of the system catalog.
E.g, get_namespace_oid(), get_langeage_oid().        ^^^^^^^^^            ^^^^^^^^          |                      |
     +--> pg_namespace      +--> pg_language
 

But some of APIs in the part 2 have different object class name
from their corresponding system catalog.

How about the following renamings?- get_tsparser_oid() -> get_ts_parser_oid()- get_tsdictionary_oid() ->
get_ts_dict_oid()-get_tstemplate_oid() -> get_ts_template_oid()- get_tsconfiguration_oid() -> get_ts_config_oid()-
get_rule_oid()-> get_rewrite_oid()- get_rule_oid_without_relid() -> get_rewrite_oid_without_relid()
 

But, it is also depending on my subjective sense.

Even if you disagree with the proposition, I'll mark it 'ready for
committer' on the next.

Thanks,

(2010/07/06 22:23), Robert Haas wrote:
> 2010/7/5 KaiGai Kohei<kaigai@ak.jp.nec.com>:
>> I checked this patch. Then, I was surprised at we have such so many
>> code duplications. I believe this patch can effectively eliminate
>> these code duplications and it is worthful to apply.
>> However, here are some points to be fixed/revised.
>>
>> 1. [BUG] DropCast() does not handle missing_ok correctly.
>>
>> You removed the 'return' on the path when get_cast_oid() with missing_ok = true
>> returned InvalidOid. It seems to me a bug to be fixed.
> 
> Good catch.  Fixed.
> 
>> 2. we can reduce more code duplications using get_opfamily_oid() and
>>    get_opclass_oid().
>>
>> I could find translations from a qualified name to schema/object names
>> at GetIndexOpClass(), RenameOpFamily() and AlterOpFamilyOwner().
>> The new APIs enable to eliminate code duplications here.
>>
>> GetIndexOpClass() needs input type of the operator class, in addition
>> to its OID, but it can be obtained using get_opclass_input_type().
>> RenameOpFamily() and AlterOpFamilyOwner() need a copy of the operator
>> family tuple, in addition to its OID, but it can be obtained using
>> GetSysCacheCopy1(OPFAMILYOID).
> 
> I don't believe this is a good idea.  We don't want to do extra
> syscache lookups just so that we can make the code look nicer.
> 
>> 3. Are OpClassCacheLookup() and OpFamilyCacheLookup() still needed?
>>
>> The OpFamilyCacheLookup() is only called from RemoveOpFamily()
>> except for the get_opfamily_oid(), because it needs namespace OID
>> in addition to the OID of operator family.
>> If we have get_opfamily_namespace() in lsyscache.c, we can merge
>> get_opfamily_oid() and OpFamilyCacheLookup() completely?
>>
>> The OpClassCacheLookup() is only called from RemoveOpClass(),
>> RenameOpClass() and AlterOpClassOwner(), because RemoveOpClass()
>> needs namespace OID in addition to the OID of operator class,
>> and rest of them want to copy the HeapTuple to update it.
>> If we have get_opclass_namespace() in lsyscache.c, RemoveOpClass()
>> can use get_opclass_oid() instead of OpClassCacheLookup().
>> And, we can use a pair of get_opclass_oid() and GetSysCacheCopy1()
>> instead of OpClassCacheLookup() and heap_copytuple() in the
>> RenameOpClass() and AlterOpClassOwner().
> 
> Same thing here.  I left that stuff alone on purpose.
> 
>> 4. Name of the arguments incorrect.
>>
>> It seems to me 'missing_ok', instead of 'failOK'. :D
> 
> Yeah, that's an oversight on my part.  Fixed.
> 


-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>