2010/7/5 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)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
> 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
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.
The Enterprise Postgres Company
In response to
pgsql-hackers by date
|Next:||From: Robert Haas||Date: 2010-07-06 13:25:03|
|Subject: Re: get_whatever_oid, part 1: object types with unqualifed names|
|Previous:||From: Leonardo F||Date: 2010-07-06 09:31:39|
|Subject: Re: I: About "Our CLUSTER implementation is pessimal" patch|