Re: ALTER command reworks

2012/11/19 Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>:
> Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>