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
tryto add in this patch. Since this patch was submitted about one month ago, it might be necessary to rebase to the
latestmaster.
 

* 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,
thisrenaming does not affects column name of the typed tables in use. Is it necessary to prohibit renaming, or also
callsrenameatt() 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@ak.jp.nec.com>