Skip site navigation (1) Skip section navigation (2)

Re: ALTER command reworks

From: Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr>
To: Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>
Cc: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, PgHacker <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: ALTER command reworks
Date: 2012-11-19 15:39:27
Message-ID: m24nklvbyo.fsf@2ndQuadrant.fr (view raw or flat)
Thread:
Lists: pgsql-hackers
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


In response to

Responses

pgsql-hackers by date

Next:From: Tom LaneDate: 2012-11-19 16:04:31
Subject: Re: [RFC] Fix div/mul crash and more undefined behavior
Previous:From: Amit KapilaDate: 2012-11-19 15:36:58
Subject: Re: Proposal for Allow postgresql.conf values to be changed via SQL

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group