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.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company