Re: ALTER TYPE extensions

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.

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

On fre, 2010-09-17 at 18:15 +0900, KaiGai Kohei wrote:
> * 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.

Good catch. Redundant checks removed.

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

Yeah, just to keep the patch small. ;-)

> * RENAME ATTRIBUTE ... TO ...
>
> Even if the composite type to be altered is in use, we can alter
> the name of attribute. Is it intended?

No. Added a check for it now.

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

The prohibition on altering types that are used in typed tables is
actually from the SQL standard. But for now it's just because it's not
implemented; I plan to work on extending that later.

The restriction by find_composite_type_dependencies() was already there
for altering tables, and I just kept it the same for now.

New patch attached.

Sorry, I missed a bug when we create a typed table using composite
type which has been altered.

postgres=# CREATE TYPE comp_1 AS (x int, y int, z int);
CREATE TYPE
postgres=# ALTER TYPE comp_1 DROP ATTRIBUTE y;
ALTER TYPE
postgres=# CREATE TABLE t1 OF comp_1;
ERROR: cache lookup failed for type 0
postgres=# SELECT attname, attnum, attisdropped FROM pg_attribute
WHERE attrelid = 'comp_1'::regclass;
attname | attnum | attisdropped
------------------------------+--------+--------------
x | 1 | f
........pg.dropped.2........ | 2 | t
z | 3 | f
(3 rows)

Perhaps, we also need to patch at transformOfType() to
skip attributes with attisdropped.

An additional question. It seems me we can remove all the attributes
from the composite type, although CREATE TYPE prohibits to create
a composite type without any attribute.
What does it mean a composite type with no attribute?
Or, do we need a restriction to prevent the last one attribute?

Rest of comments are below.

(2010/09/18 5:44), Peter Eisentraut wrote:
> On fre, 2010-09-17 at 18:15 +0900, KaiGai Kohei wrote:
>> * 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.
>
> Good catch. Redundant checks removed.
>
OK,

>> * At the ATPrepAlterColumnType(), you enclosed an existing code
>> block by "if (tab->relkind == RELKIND_RELATION) { ... }", but
>> it is not indented to appropriate level.
>
> Yeah, just to keep the patch small. ;-)
>
Hmm...
Although I expect the patched routine also should follow the common
coding style in spite of patch size, but it may not be a thing that
I should decide here.
So, I'd like to entrust this decision to committer. OK?

>> * RENAME ATTRIBUTE ... TO ...
>>
>> Even if the composite type to be altered is in use, we can alter
>> the name of attribute. Is it intended?
>
> No. Added a check for it now.
>
OK,

>> 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.
>
> The prohibition on altering types that are used in typed tables is
> actually from the SQL standard. But for now it's just because it's not
> implemented; I plan to work on extending that later.
>
> The restriction by find_composite_type_dependencies() was already there
> for altering tables, and I just kept it the same for now.
>
Thanks for your explanation. It made me clear.

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

On tis, 2010-09-21 at 17:53 +0900, KaiGai Kohei wrote:
> Sorry, I missed a bug when we create a typed table using composite
> type which has been altered.

> Perhaps, we also need to patch at transformOfType() to
> skip attributes with attisdropped.

Fixed.

> An additional question. It seems me we can remove all the attributes
> from the composite type, although CREATE TYPE prohibits to create
> a composite type without any attribute.
> What does it mean a composite type with no attribute?
> Or, do we need a restriction to prevent the last one attribute?

We need to allow the creation of zero-attribute types then; same as with
CREATE TABLE. I have fixed that now.

(2010/09/22 5:17), Peter Eisentraut wrote:
> On tis, 2010-09-21 at 17:53 +0900, KaiGai Kohei wrote:
>> Sorry, I missed a bug when we create a typed table using composite
>> type which has been altered.
>
>> Perhaps, we also need to patch at transformOfType() to
>> skip attributes with attisdropped.
>
> Fixed.
>
OK,

>> An additional question. It seems me we can remove all the attributes
>> from the composite type, although CREATE TYPE prohibits to create
>> a composite type without any attribute.
>> What does it mean a composite type with no attribute?
>> Or, do we need a restriction to prevent the last one attribute?
>
> We need to allow the creation of zero-attribute types then; same as with
> CREATE TABLE. I have fixed that now.
>
OK,

This version of the patch seems to me OK.
I marked it as 'ready for committer'.

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