Horton
Menu

Thread

  1. renameatt() can rename attribute of index, sequence, ...

    Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-03-03T02:19:27Z

    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.

-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-03-03T02:22:50Z

    KaiGai Kohei <kaigai@ak.jp.nec.com> writes:
> Is it an expected behavior?

There's no particular reason to forbid it, is there?

			regards, tom lane

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

    Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-03-03T02:27:43Z

    (2010/03/03 11:22), Tom Lane wrote:
> KaiGai Kohei<kaigai@ak.jp.nec.com>  writes:
>> Is it an expected behavior?
> 
> There's no particular reason to forbid it, is there?

Perhaps, it is harmless.

It just seemed to me the renameatt() was not implemented correctly
according to the documentation.
-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>

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

    Robert Haas <robertmhaas@gmail.com> — 2010-03-03T05:26:29Z

    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.

...Robert

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

    Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-03-03T06:03:00Z

    (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().

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.

Thanks,
-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>

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

    Boszormenyi Zoltan <zb@cybertec.at> — 2010-03-03T08:42:14Z

    Hi,

KaiGai Kohei írta:
> (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().
>
> 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 just tried it on 8.3.7:

zozo=# create sequence seq2;
CREATE SEQUENCE

"is_called" is modified from false to true on the first call of nextval()
so I renamed it:

zozo=# alter table seq2 rename column is_called to bob;
ALTER TABLE

zozo=# create table seq2_tab (id integer primary key default
nextval('seq2'), t text);
NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index
"seq2_tab_pkey" for table "seq2_tab"
CREATE TABLE
zozo=# alter sequence seq2 owned by seq2_tab.id;
ALTER SEQUENCE

No error it seems:

zozo=# insert into seq2_tab (t) values ('a');
INSERT 0 1

zozo=# select * from seq2;
sequence_name | last_value | increment_by | max_value | min_value |
cache_value | log_cnt | is_cycled | bob
---------------+------------+--------------+---------------------+-----------+-------------+---------+-----------+-----
seq2 | 1 | 1 | 9223372036854775807 | 1 | 1 | 0 | f | t
(1 sor)

Let's try other fields:

zozo=# alter table seq2 rename column min_value to first;
ALTER TABLE
zozo=# alter table seq2 rename column max_value to last;
ALTER TABLE
zozo=# alter table seq2 rename column last_value to always;
ALTER TABLE

Still no error:

zozo=# insert into seq2_tab (t) values ('b');
INSERT 0 1

Let's try more fields:

zozo=# alter table seq2 rename column cache_value to keep;
ALTER TABLE
zozo=# alter table seq2 rename column increment_by to advance;
ALTER TABLE
zozo=# alter table seq2 rename column is_cycled to bobek;
ALTER TABLE

Still no error:

zozo=# insert into seq2_tab (t) values ('c');
INSERT 0 1
zozo=# select * from seq2;
sequence_name | always | advance | last | first | keep | log_cnt | bobek
| bob
---------------+--------+---------+---------------------+-------+------+---------+-------+-----
seq2 | 3 | 1 | 9223372036854775807 | 1 | 1 | 31 | f | t
(1 sor)

Still no error:

zozo=# alter table seq2 rename column log_cnt to pampalini;
ALTER TABLE
zozo=# insert into seq2_tab (t) values ('d');
INSERT 0 1
zozo=# select * from seq2;
sequence_name | always | advance | last | first | keep | pampalini |
bobek | bob
---------------+--------+---------+---------------------+-------+------+-----------+-------+-----
seq2 | 4 | 1 | 9223372036854775807 | 1 | 1 | 32 | f | t
(1 sor)

Change the last remaining field and still no error:

zozo=# alter table seq2 rename column sequence_name to pimpa;
ALTER TABLE
zozo=# insert into seq2_tab (t) values ('d');
INSERT 0 1
zozo=# select * from seq2;
pimpa | always | advance | last | first | keep | pampalini | bobek | bob
-------+--------+---------+---------------------+-------+------+-----------+-------+-----
seq2 | 5 | 1 | 9223372036854775807 | 1 | 1 | 31 | f | t
(1 sor)

zozo=# select * from seq2_tab;
id | t
----+---
1 | a
2 | b
3 | c
4 | d
5 | d
(5 rows)

Internally, the system refers these column by position instead of names.
But from the user perspective, the sequence fields are more
like system columns, renaming them leads to confusion.

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

Best regards,
Zoltán Böszörményi

-- 
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

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

    Robert Haas <robertmhaas@gmail.com> — 2010-03-03T13:42:15Z

    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.

...Robert

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

    Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-03-04T00:10:37Z

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

  postgres=# CREATE TABLE t (a serial primary key, b text);
  NOTICE:  CREATE TABLE will create implicit sequence "t_a_seq" for serial column "t.a"
  NOTICE:  CREATE TABLE / PRIMARY KEY will create implicit index "t_pkey" for table "t"
  CREATE TABLE
  postgres=# ALTER TABLE t_a_seq RENAME sequence_name TO foo;
  ERROR:  "t_a_seq" is not a table or view
  postgres=# ALTER TABLE t_pkey RENAME a TO var;
  ERROR:  "t_pkey" is not a table or view
  postgres=# ALTER TABLE t RENAME b TO baz;
  ALTER TABLE
  postgres=# SELECT * FROM t;
   a | baz
  ---+-----
  (0 rows)

Ideally, I think it is not necessary to call CheckRelationOwnership()
at ExecRenameStmt() with OBJECT_COLUMN, because ATSimplePermissions()
also apply same checks later.
However, it might be done in the context of access control reworks for
the ALTER TABLE statement.

Thanks,
-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>

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

    Robert Haas <robertmhaas@gmail.com> — 2010-03-04T00:16:53Z

    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.

...Robert

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

    Robert Haas <robertmhaas@gmail.com> — 2010-03-10T15:54:07Z

    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

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

    Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-03-10T23:58:35Z

    (2010/03/11 0:54), Robert Haas wrote:
> 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.

Indeed, it is useful to allow renaming attribute of composite types.

However, it is also useless to allow to rename attribute of sequences,
but harmless, like renames on indexes. It seems to me it is fair enough
to allow renaming attributes of tables, views and composite types...

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

Yes, when we rename attribute of the relations, it originally checks
ownership of the relation twice in ExecRenameStmt() and renameatt().
It should be reworked in the next.

Thanks,
-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>

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

    Robert Haas <robertmhaas@gmail.com> — 2010-03-11T14:55:57Z

    2010/3/10 KaiGai Kohei <kaigai@ak.jp.nec.com>:
> Indeed, it is useful to allow renaming attribute of composite types.
>
> However, it is also useless to allow to rename attribute of sequences,
> but harmless, like renames on indexes. It seems to me it is fair enough
> to allow renaming attributes of tables, views and composite types...

I don't agree.  I think users should be allowed to rename things they
had a hand in naming in the first place (and index columns fall into
that category, since the names are derived from table column names).
But changing system-assigned column names for sequences or toast
tables is just weird.

...Robert

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

    Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-03-12T00:19:48Z

    (2010/03/11 23:55), Robert Haas wrote:
> 2010/3/10 KaiGai Kohei<kaigai@ak.jp.nec.com>:
>> Indeed, it is useful to allow renaming attribute of composite types.
>>
>> However, it is also useless to allow to rename attribute of sequences,
>> but harmless, like renames on indexes. It seems to me it is fair enough
>> to allow renaming attributes of tables, views and composite types...
> 
> I don't agree.  I think users should be allowed to rename things they
> had a hand in naming in the first place (and index columns fall into
> that category, since the names are derived from table column names).
> But changing system-assigned column names for sequences or toast
> tables is just weird.

OK, the attached patch forbid to rename an attribute of relations except
for tables, views, composite types and indexes.

Thanks,
-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>

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

    Robert Haas <robertmhaas@gmail.com> — 2010-03-13T01:08:23Z

    2010/3/11 KaiGai Kohei <kaigai@ak.jp.nec.com>:
> (2010/03/11 23:55), Robert Haas wrote:
>> 2010/3/10 KaiGai Kohei<kaigai@ak.jp.nec.com>:
>>> Indeed, it is useful to allow renaming attribute of composite types.
>>>
>>> However, it is also useless to allow to rename attribute of sequences,
>>> but harmless, like renames on indexes. It seems to me it is fair enough
>>> to allow renaming attributes of tables, views and composite types...
>>
>> I don't agree.  I think users should be allowed to rename things they
>> had a hand in naming in the first place (and index columns fall into
>> that category, since the names are derived from table column names).
>> But changing system-assigned column names for sequences or toast
>> tables is just weird.
>
> OK, the attached patch forbid to rename an attribute of relations except
> for tables, views, composite types and indexes.

I would like to apply this for 9.0, with some adjustments to the
comments.  Objections?

...Robert