Re: ALTER command reworks

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