Re: get_whatever_oid, part 2

(2010/06/15 2:25), Robert Haas wrote:
> Per previous discussion:
>
> http://archives.postgresql.org/pgsql-hackers/2010-05/msg01195.php
> http://archives.postgresql.org/pgsql-hackers/2010-05/msg01577.php
>
> Changes in this patch:
>
> - Rename TSParserGetPrsid to get_tsparser_oid, TSDictionaryGetDictid
> to get_tsdictionary_oid, TSTemplateGetTmplid to get_tstemplate_oid,
> TSConfigGetCfgid to get_tsconfiguration_oid, FindConversionByName to
> get_conversion_oid, and GetConstraintName to get_constraint_oid.
> - Add new functions get_opclass_oid, get_opfamily_oid, get_rule_oid,
> get_rule_oid_without_relid, get_trigger_oid, and get_cast_oid.
> - Refactor existing code to use these functions wherever possible.
>
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.

| --- a/src/backend/commands/functioncmds.c
| +++ b/src/backend/commands/functioncmds.c
| @@ -1759,31 +1759,23 @@ DropCast(DropCastStmt *stmt)
| {
| Oid sourcetypeid;
| Oid targettypeid;
| - HeapTuple tuple;
| ObjectAddress object;
|
| /* when dropping a cast, the types must exist even if you use IF EXISTS */
| sourcetypeid = typenameTypeId(NULL, stmt->sourcetype, NULL);
| targettypeid = typenameTypeId(NULL, stmt->targettype, NULL);
|
| - tuple = SearchSysCache2(CASTSOURCETARGET,
| - ObjectIdGetDatum(sourcetypeid),
| - ObjectIdGetDatum(targettypeid));
| - if (!HeapTupleIsValid(tuple))
| + object.classId = CastRelationId;
| + object.objectId = get_cast_oid(sourcetypeid, targettypeid,
| + stmt->missing_ok);
| + object.objectSubId = 0;
| +
| + if (!OidIsValid(object.objectId))
| {
| - if (!stmt->missing_ok)
| - ereport(ERROR,
| - (errcode(ERRCODE_UNDEFINED_OBJECT),
| - errmsg("cast from type %s to type %s does not exist",
| - format_type_be(sourcetypeid),
| - format_type_be(targettypeid))));
| - else
| - ereport(NOTICE,
| + ereport(NOTICE,
| (errmsg("cast from type %s to type %s does not exist, skipping",
| format_type_be(sourcetypeid),
| format_type_be(targettypeid))));
| -
| - return;
| }
|
| /* Permission check */

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.

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

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

4. Name of the arguments incorrect.

| --- a/src/include/catalog/namespace.h
| +++ b/src/include/catalog/namespace.h
| @@ -74,16 +74,16 @@ extern bool OpfamilyIsVisible(Oid opfid);
| extern Oid ConversionGetConid(const char *conname);
| extern bool ConversionIsVisible(Oid conid);
|
| -extern Oid TSParserGetPrsid(List *names, bool failOK);
| +extern Oid get_tsparser_oid(List *names, bool failOK);
| extern bool TSParserIsVisible(Oid prsId);
|
| -extern Oid TSDictionaryGetDictid(List *names, bool failOK);
| +extern Oid get_tsdictionary_oid(List *names, bool failOK);
| extern bool TSDictionaryIsVisible(Oid dictId);
|
| -extern Oid TSTemplateGetTmplid(List *names, bool failOK);
| +extern Oid get_tstemplate_oid(List *names, bool failOK);
| extern bool TSTemplateIsVisible(Oid tmplId);
|
| -extern Oid TSConfigGetCfgid(List *names, bool failOK);
| +extern Oid get_tsconfiguration_oid(List *names, bool failOK);
| extern bool TSConfigIsVisible(Oid cfgid);
|
| extern void DeconstructQualifiedName(List *names,

It seems to me 'missing_ok', instead of 'failOK'. :D

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