Re: ALTER command reworks - Mailing list pgsql-hackers

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.

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