Re: ALTER TYPE recursion to typed tables

I'm working on propagating ALTER TYPE commands to typed tables. This is
currently prohibited. For example, take these regression test cases:

CREATE TYPE test_type2 AS (a int, b text);
CREATE TABLE test_tbl2 OF test_type2;
ALTER TYPE test_type2 ADD ATTRIBUTE c text; -- fails
ALTER TYPE test_type2 ALTER ATTRIBUTE b TYPE varchar; -- fails
ALTER TYPE test_type2 DROP ATTRIBUTE b; -- fails
ALTER TYPE test_type2 RENAME ATTRIBUTE b TO bb; -- fails

The actual implementation isn't very difficult, because the ALTER TABLE
code already knows everything about recursion.

Now I'm wondering what kind of syntax should be used to control this. I
think you don't want to automatically propagate such innocent looking
operations to tables in a potentially data-destroying manner. The
natural idea would be RESTRICT/CASCADE. This is currently only
associated with DROP operations, but I suppose ADD/ALTER/RENAME
ATTRIBUTE x ... CASCADE doesn't sound too odd.

Comments, other ideas?

On Tue, Nov 2, 2010 at 9:15 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> I'm working on propagating ALTER TYPE commands to typed tables.  This is
> currently prohibited.  For example, take these regression test cases:
>
> CREATE TYPE test_type2 AS (a int, b text);
> CREATE TABLE test_tbl2 OF test_type2;
> ALTER TYPE test_type2 ADD ATTRIBUTE c text; -- fails
> ALTER TYPE test_type2 ALTER ATTRIBUTE b TYPE varchar; -- fails
> ALTER TYPE test_type2 DROP ATTRIBUTE b; -- fails
> ALTER TYPE test_type2 RENAME ATTRIBUTE b TO bb; -- fails
>
> The actual implementation isn't very difficult, because the ALTER TABLE
> code already knows everything about recursion.
>
> Now I'm wondering what kind of syntax should be used to control this.  I
> think you don't want to automatically propagate such innocent looking
> operations to tables in a potentially data-destroying manner.  The
> natural idea would be RESTRICT/CASCADE.  This is currently only
> associated with DROP operations, but I suppose ADD/ALTER/RENAME
> ATTRIBUTE x ... CASCADE doesn't sound too odd.
>
> Comments, other ideas?

That seems reasonable. What do you plan to do about this case?

CREATE TYPE test_type AS (a int, b text);
CREATE TABLE test_tbl (x test_type);
ALTER TYPE test_type ADD ATTRIBUTE c text;

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On tis, 2010-11-02 at 10:54 -0700, Robert Haas wrote:
> What do you plan to do about this case?
>
> CREATE TYPE test_type AS (a int, b text);
> CREATE TABLE test_tbl (x test_type);
> ALTER TYPE test_type ADD ATTRIBUTE c text;

This is currently prohibited, and I'm not planning to do anything about
that.

On Nov 2, 2010, at 11:17 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On tis, 2010-11-02 at 10:54 -0700, Robert Haas wrote:
>> What do you plan to do about this case?
>>
>> CREATE TYPE test_type AS (a int, b text);
>> CREATE TABLE test_tbl (x test_type);
>> ALTER TYPE test_type ADD ATTRIBUTE c text;
>
> This is currently prohibited, and I'm not planning to do anything about
> that.

OK.

...Robert

Here is the patch that adds [RESTRICT|CASCADE] to ALTER TYPE ...
ADD/ALTER/DROP/RENAME ATTRIBUTE, so that recurses to typed tables.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> Here is the patch that adds [RESTRICT|CASCADE] to ALTER TYPE ...
> ADD/ALTER/DROP/RENAME ATTRIBUTE, so that recurses to typed tables.

And here's my commitfest review of it:

- patch applies cleanly
- adds regression tests
- passes them
- is useful and needed, and something we don't already have
- don't generate warnings (or I missed them) :)

Code wise, though, I wonder about the name of the "recursing" parameter
of the renameatt_internal function is src/backend/commands/tablecmds.c,
which seems to only get used to detect erroneous attempt at renaming the
table column directly. Maybe it's only me not used enough to PostgreSQL
code yet, but here it distract the code reader. Having another parameter
called "recurse" is not helping, too, but I don't see this one needs to
be changed.

I'm not sure what a good name would be here, alter_type_cascade is an
example that comes to mind, on the verbose side.

As I think the issue is to be decided by a commiter, I will go and mark
this patch as ready for commiter!

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On ons, 2010-11-17 at 21:05 +0100, Dimitri Fontaine wrote:
> Code wise, though, I wonder about the name of the "recursing"
> parameter of the renameatt_internal function is
> src/backend/commands/tablecmds.c,
> which seems to only get used to detect erroneous attempt at renaming
> the table column directly. Maybe it's only me not used enough to
> PostgreSQL code yet, but here it distract the code reader. Having
> another parameter called "recurse" is not helping, too, but I don't
> see this one needs to be changed.

This parameter has only minimal use in the renameatt case, but the same
terminology is used throughout the ALTER TABLE code, so I think it's
wise to keep it.