Re: get_whatever_oid, part 2

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.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

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

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

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

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

2010/7/8 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> 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()

I like that idea. Done, attached.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

(2010/07/09 22:36), Robert Haas wrote:
> 2010/7/8 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> 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()
>
> I like that idea. Done, attached.
>

I checked it, but here is nothing to comment any more.
So, I marked it as "ready for committer".

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