Re: ALTER command reworks

2012/8/31 Kohei KaiGai <kaigai@kaigai.gr.jp>:
> 2012/8/30 Robert Haas <robertmhaas@gmail.com>:
>> On Mon, Aug 13, 2012 at 3:35 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>>> The attached patch is a refreshed version of ALTER command
>>> reworks towards the latest tree. Here is few big changes except
>>> for code integration of the code to rename event triggers.
>>
>> This seems to have bit-rotted a bit.  Please rebase.
>>
>>> BTW, I had to adjust between oid of pg_largeobject_metadata
>>> and pg_largeobject on three points of this patch, like:
>>>
>>>         if (classId == LargeObjectRelationId)
>>>             classId = LargeObjectMetadataRelationId;
>>>
>>> When we supported largeobject permission features, we put
>>> special handling to track dependency of its ownership.
>>> The pg_depend records oid of pg_largeobject, instead of
>>> pg_largeobject_metadata. Thus, we cannot use classId of
>>> ObjectAddress being returned from get_object_address()
>>> as an argument of heap_open() as is, if it indicates oid of
>>> pg_largeobject.
>>>
>>> Was it a right decision to track dependency of large object using
>>> oid of pg_largeobject, instead of pg_largeobject_metadata?
>>> IIRC, the reason why we used oid of pg_largeobject is backward
>>> compatibility for applications that tries to reference pg_depend
>>> with built-in oids.
>>
>> I think it was a terrible decision and I'm pretty sure I said as much
>> at the time, but I lost the argument, so I'm inclined to think we're
>> stuck with continuing to support that kludge.
>>
> OK, we will keep to implement carefully...
>
>> Some other preliminary comments:
>>
>> - Surely you need to take AccessExclusiveLock on the object being
>> renamed, not just ShareUpdateExclusiveLock.
>> - I don't think it's acceptable to assemble the object-type
>> "object-name" already exists message using getObjectDescription();
>> it's not good for translators.  Use an array of messages, one per
>> object-type, as we have done in other cases.
>> - I would like to handle either the RENAME case or the ALTER OWNER
>> case in one patch, and the other in a follow-on patch.  Can you pick
>> one to do first and remove everything related to the other one?
>>
> OK, I'll split the patch into three (isn't it?) portions; RENAME, SET OWNER
> and SET SCHEMA, with all above your suggestions.
>
As attached, I split off the original one into three portions; for set-schema,
set-owner and rename-to. Please apply them in order of patch filename.
Regarding to the error message, RenameErrorMsgAlreadyExists() was added
to handle per object type messages in case when aclcheck_error() is not
available to utilize.
All the regression test is contained with the 1st patch to make sure the
series of reworks does not change existing behaviors.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>