Re: ALTER command reworks

On Mon, Jan 7, 2013 at 3:43 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
> Kohei KaiGai escribió:
>
>> The attached patch is a revised version.
>>
>> It follows the manner in ExecAlterObjectSchemaStmt(); that tries to check
>> name duplication for object classes that support catcache with name-key.
>> Elsewhere, it calls individual routines for each object class to solve object
>> name and check namespace conflicts.
>> For example, RenameFunction() is still called from ExecRenameStmt(),
>> however, it looks up the given function name and checks namespace
>> conflict only, then it just invokes AlterObjectRename_internal() in spite
>> of system catalog updates by itself.
>> It allows to use this consolidated routine by object classes with special
>> rule for namespace conflicts, such as collation.
>> In addition, this design allowed to eliminate get_object_type() to pull
>> OBJECT_* enum from class_id/object_id pair.
>
> I checked this patch. It needed a rebase for the changes to return
> OIDs. Attached patch applies to current HEAD. In general this looks
> good, with one exception: it's using getObjectDescriptionOids() to build
> the messages to complain in case the object already exists in the
> current schema, which results in diffs like this:
>
> -ERROR: event trigger "regress_event_trigger2" already exists
> +ERROR: event trigger regress_event_trigger2 already exists
>
> I don't know how tense we are about keeping the quotes, but I fear there
> would be complaints because it took us lots of sweat, blood and tears to
> get where we are now.
>
> If this is considered a problem, I think the way to fix it is to have a
> getObjectDescriptionOids() variant that quotes the object name in the
> output. This would be pretty intrusive: we'd have to change things
> so that, for instance,
>
> appendStringInfo(&buffer, _("collation %s"),
> NameStr(coll->collname));
>
> would become
>
> if (quotes)
> appendStringInfo(&buffer, _("collation \"%s\""),
> NameStr(coll->collname));
> else
> appendStringInfo(&buffer, _("collation %s"),
> NameStr(coll->collname));
>
> Not really thrilled with this idea. Of course,
> getObjectDescription() itself should keep the same API as now, to avoid
> useless churn all over the place.

This sort of thing has been rejected repeatedly in the past on
translation grounds:

+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("%s already exists in schema \"%s\"",
+ getObjectDescriptionOids(classId, exists),
+ get_namespace_name(namespaceId))));

If we're going to start allowing that, there's plenty of other code
that can be hit with the same hammer, but I'm pretty sure that all
previous proposals in this area have been slapped down.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company