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

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