Re: renameatt() can rename attribute of index, sequence, ...

On Wed, Mar 3, 2010 at 7:16 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> 2010/3/3 KaiGai Kohei <kaigai@ak.jp.nec.com>:
>> (2010/03/03 22:42), Robert Haas wrote:
>>> 2010/3/3 KaiGai Kohei<kaigai@ak.jp.nec.com>:
>>>> (2010/03/03 14:26), Robert Haas wrote:
>>>>> 2010/3/2 KaiGai Kohei<kaigai@ak.jp.nec.com>:
>>>>>> Is it an expected behavior?
>>>>>>
>>>>>>    postgres=>    CREATE SEQUENCE s;
>>>>>>    CREATE SEQUENCE
>>>>>>    postgres=>    ALTER TABLE s RENAME sequence_name TO abcd;
>>>>>>    ALTER TABLE
>>>>>>
>>>>>>    postgres=>    CREATE TABLE t (a int primary key, b text);
>>>>>>    NOTICE:  CREATE TABLE / PRIMARY KEY will create implicit index "t_pkey" for table "t"
>>>>>>    CREATE TABLE
>>>>>>    postgres=>    ALTER TABLE t_pkey RENAME a TO xyz;
>>>>>>    ALTER TABLE
>>>>>>
>>>>>> The documentation says:
>>>>>>    http://developer.postgresql.org/pgdocs/postgres/sql-altertable.html
>>>>>>
>>>>>>      :
>>>>>>    RENAME
>>>>>>      The RENAME forms change the name of a table (or an index, sequence, or view) or
>>>>>>      the name of an individual column in a table. There is no effect on the stored data.
>>>>>>
>>>>>> It seems to me the renameatt() should check relkind of the specified relation, and
>>>>>> raise an error if relkind != RELKIND_RELATION.
>>>>>
>>>>> Are we talking about renameatt() or RenameRelation()?  Letting
>>>>> RenameRelation() rename whatever seems fairly harmless; renameatt(),
>>>>> on the other hand, should probably refuse to allow this:
>>>>>
>>>>> CREATE SEQUENCE foo;
>>>>> ALTER TABLE foo RENAME COLUMN is_cycled TO bob;
>>>>>
>>>>> ...because that's just weird.  Tables, indexes, and views make sense,
>>>>> but the attributes of a sequence should be nailed down I think;
>>>>> they're basically system properties.
>>>>
>>>> I'm talking about renameatt(), not RenameRelation().
>>>
>>> OK.  Your original example was misleading because you had renameatt()
>>> in the subject line but the actual SQL commands were renaming a whole
>>> relation (which is a reasonable thing to do).
>>>
>>>> If our perspective is these are a type of system properties, we should
>>>> be able to reference these attributes with same name, so it is not harmless
>>>> to allow renaming these attributes.
>>>>
>>>> I also agree that it makes sense to allow renaming attributes of tables
>>>> and views. But I don't know whether it makes sense to allow it on indexs,
>>>> like sequence and toast relations.
>>>
>>> I would think not.
>>
>> OK, the attached patch forbid renameatt() on relations expect for tables
>> and views.
>
> OK, I will review it.

I don't think we should apply this as-is.  After some reflection, I
don't believe we should reject attribute renames on indices or
composite types.  The former is maybe useless but seems harmless, and
the latter seems affirmatively useful.

Also, I think that the comment about "this would normally be done in
utility.c" is no longer true and should be removed while we're
cleaning house.

...Robert