Re: ALTER command reworks - Mailing list pgsql-hackers

2012/11/19 Kohei KaiGai <kaigai@kaigai.gr.jp>:
> 2012/11/19 Dimitri Fontaine <dimitri@2ndquadrant.fr>:
>> Kohei KaiGai <kaigai@kaigai.gr.jp> writes:
>>> OK, Are you suggesting to add a "generic" comments such as "Generic
>>> function to change the name of a given object, for simple cases ...",
>>> not a list of OBJECT_* at the head of this function, aren't you?
>>
>> Just something like
>>
>>  * Most simple objects are covered by a generic function, the other
>>  * still need special processing.
>>
>> Mainly I was surprised to see collation not supported here, but I didn't
>> take enough time to understand why that is the case. I will do that
>> later in the review process.
>>
> The reason why collation is not supported is that takes special name-
> duplication checks. The new collation name must have no collision on
> both of current database encoding and "any" encoding.
> It might be an idea to have a simple rule similar to
> AlterObjectNamespace_internal(); that requires caller to check
> namespace-duplication, if the given object type has no catcache-id
> with name-key.
>
>>> The pain point is AlterObjectNamespace_internal is not invoked by
>>> only ExecAlterObjectSchemaStmt(), but AlterObjectNamespace_oid()
>>> also.
>>
>> I should remember better about that as the use case is extensions…
>>
>>> It is the reason why I had to put get_object_type() to find out OBJECT_*
>>> from the supplied ObjectAddress, because this code path does not
>>> available to pass down its ObjectType from the caller.
>>
>> If we really want to do that, I think I would only do that in
>> AlterObjectNamespace_oid and add an argument to
>> AlterObjectNamespace_internal, that you already have in
>> ExecAlterObjectSchemaStmt().
>>
>> But really, what about just removing that part of your patch instead:
>>
>> @@ -396,14 +614,23 @@ AlterObjectNamespace_internal(Relation rel, Oid objid, Oid nspOid)
>>          * Check for duplicate name (more friendly than unique-index failure).
>>          * Since this is just a friendliness check, we can just skip it in cases
>>          * where there isn't a suitable syscache available.
>> +        *
>> +        * XXX - the caller should check object-name duplication, if the supplied
>> +        * object type need to take object arguments for identification, such as
>> +        * functions.
>>          */
>> -       if (nameCacheId >= 0 &&
>> -               SearchSysCacheExists2(nameCacheId, name, ObjectIdGetDatum(nspOid)))
>> -               ereport(ERROR,
>> -                               (errcode(ERRCODE_DUPLICATE_OBJECT),
>> -                                errmsg("%s already exists in schema \"%s\"",
>> -                                               getObjectDescriptionOids(classId, objid),
>> -                                               get_namespace_name(nspOid))));
>> +       if (get_object_catcache_name(classId) >= 0)
>> +       {
>> +               ObjectAddress   address;
>> +
>> +               address.classId = classId;
>> +               address.objectId = objid;
>> +               address.objectSubId = 0;
>> +
>> +               objtype = get_object_type(&address);
>> +               check_duplicate_objectname(objtype, nspOid,
>> +                                                                  NameStr(*DatumGetName(name)), NIL);
>> +       }
>>
>> It would be much simpler to retain the old-style duplicate object check,
>> and compared to doing extra cache lookups, it'd still be much cleaner in
>> my view.
>>
> Now, I get inclined to follow the manner of AlterObjectNamespace_internal
> at AlterObjectRename also; that requires caller to check name duplication
> in case when no catcache entry is supported.
>
> That simplifies the logic to check name duplication, and allows to eliminate
> get_object_type() here, even though RenameAggregate and
> RenameFunction have to be remained.
>
The attached patch is a revised version.

It follows the manner in ExecAlterObjectSchemaStmt(); that tries to check
name duplication for object classes that support catcache with name-key.
Elsewhere, it calls individual routines for each object class to solve object
name and check namespace conflicts.
For example, RenameFunction() is still called from ExecRenameStmt(),
however, it looks up the given function name and checks namespace
conflict only, then it just invokes AlterObjectRename_internal() in spite
of system catalog updates by itself.
It allows to use this consolidated routine by object classes with special
rule for namespace conflicts, such as collation.
In addition, this design allowed to eliminate get_object_type() to pull
OBJECT_* enum from class_id/object_id pair.

Please check this patch.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>