Re: ALTER TYPE extensions

(2010/08/09 5:54), Peter Eisentraut wrote:
> For the next review cycle, here is a patch that adds some ALTER TYPE
> subcommands for composite types:
>
> ALTER TYPE ... ADD ATTRIBUTE
> ALTER TYPE ... DROP ATTRIBUTE
> ALTER TYPE ... ALTER ATTRIBUTE ... SET DATA TYPE
> ALTER TYPE ... RENAME ATTRIBUTE
>
> These work similarly to the analogous ALTER TABLE / $ACTION COLUMN
> commands. The first two above are from the SQL standard.
>

I checked this patch, then noticed some points:

* At the ATPrepAddColumn(), it seems to me someone added a check
to prevent adding a new column to typed table, as you try to
add in this patch.
Since this patch was submitted about one month ago, it might be
necessary to rebase to the latest master.

* At the ATPrepAlterColumnType(), you enclosed an existing code
block by "if (tab->relkind == RELKIND_RELATION) { ... }", but
it is not indented to appropriate level.

| if (tab->relkind == RELKIND_RELATION)
| {
| /*
| * Set up an expression to transform the old data value to the new type.
| * If a USING option was given, transform and use that expression, else
| * just take the old value and try to coerce it. We do this first so that
| * type incompatibility can be detected before we waste effort, and
| * because we need the expression to be parsed against the original table
| * rowtype.
| */
| if (cmd->transform)
| {
| RangeTblEntry *rte;
|
| /* Expression must be able to access vars of old table */
| rte = addRangeTableEntryForRelation(pstate,
| :

Perhaps, it is violated to the common coding style.

* RENAME ATTRIBUTE ... TO ...

Even if the composite type to be altered is in use, we can alter
the name of attribute. Is it intended?
In this case, this renaming does not affects column name of the
typed tables in use.
Is it necessary to prohibit renaming, or also calls renameatt()
for the depending typed tables.

postgres=# CREATE TYPE comp as (a int, b text);
CREATE TYPE
postgres=# CREATE TABLE t OF comp;
CREATE TABLE
postgres=# SELECT * FROM t;
a | b
---+---
(0 rows)

postgres=# ALTER TYPE comp RENAME ATTRIBUTE b TO bbb;
ALTER TYPE
postgres=# CREATE TABLE s OF comp;
CREATE TABLE
postgres=# SELECT * FROM t;
a | b
---+---
(0 rows)

postgres=# SELECT * FROM s;
a | bbb
---+-----
(0 rows)

BTW, is there any requirement from SQL standard about behavior
when we try to add/drop an attribute of composite type in use?
This patch always prohibit it, using find_typed_table_dependencies()
and find_composite_type_dependencies().
However, it seems to me not difficult to alter columns of typed
tables subsequent with this ALTER TYPE, although it might be
not easy to alter definitions of embedded composite type already
in use.
Of course, it may be our future works. If so, it's good.

Thanks,
--
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>