Skip site navigation (1) Skip section navigation (2)

Re: get_whatever_oid, part 2

From: Robert Haas <robertmhaas(at)gmail(dot)com>
To: KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: get_whatever_oid, part 2
Date: 2010-07-06 13:23:40
Message-ID: AANLkTimYyruDqicdbKjWVUmehzE0x8eqzINLl71WXjRq@mail.gmail.com (view raw or flat)
Thread:
Lists: pgsql-hackers
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
>   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

Attachment: get_whatever_oid_part2-v2.patch
Description: application/octet-stream (52.3 KB)

In response to

Responses

pgsql-hackers by date

Next:From: Robert HaasDate: 2010-07-06 13:25:03
Subject: Re: get_whatever_oid, part 1: object types with unqualifed names
Previous:From: Leonardo FDate: 2010-07-06 09:31:39
Subject: Re: I: About "Our CLUSTER implementation is pessimal" patch

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group