Re: ALTER command reworks

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

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support