Re: [COMMITTERS] pgsql: Remove arbitrary ALTER TABLE .. ADD COLUMN restriction.

On Wed, Jan 26, 2011 at 11:47 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> It's certainly not obvious from the archives from around 2004-06-06
>> that this was discussed.  Perhaps you could be a bit more specific.
>> As for the spec, if it requires composite types to have defaults (or
>> constraints), then we're in violation of that all over the place
>> anyway.
>
> Here's the point: the spec requires that ADD COLUMN with a default cause
> the column to spring into existence with the default value inserted in
> all existing rows, like this:
>
> regression=# create table foo (f1 int);
> CREATE TABLE
> regression=# insert into foo values (1),(2);
> INSERT 0 2
> regression=# alter table foo add column f2 text default 'hello';
> ALTER TABLE
> regression=# select * from foo;
>  f1 |  f2
> ----+-------
>  1 | hello
>  2 | hello
> (2 rows)
>
> This is entirely different from what happens when you set the default
> afterwards:
>
> regression=# create table foo (f1 int);
> CREATE TABLE
> regression=# insert into foo values (1),(2);
> INSERT 0 2
> regression=# alter table foo add column f2 text ;
> ALTER TABLE
> regression=# select * from foo;
>  f1 | f2
> ----+----
>  1 |
>  2 |
> (2 rows)
>
> regression=# alter table foo alter column f2 set default 'hello';
> ALTER TABLE
> regression=# select * from foo;
>  f1 | f2
> ----+----
>  1 |
>  2 |
> (2 rows)
>
> In this case the column springs into existence as nulls, and the
> subsequent change of default doesn't change that.

I completely understand all of that, and I'm not debating any of it.

> What your patch does is accept the syntax with ensuing
> non-spec-compliant behavior.  That's not a step forward.  If it added
> any really useful functionality, then maybe there would be an excuse for
> violating the spec here --- but it doesn't.  You can just add the column
> without default and change the default afterwards, and get to the same
> place without using any non-spec-compliant operations.
>
> And yes, I know that we're not doing all that well with honoring
> defaults (or constraints) for rowtypes.  But that's something to be
> fixed.  Enlarging our non-compliance with the spec to gain no useful
> functionality isn't an improvement.

Saying "we're not doing all that well" with it is an understatement.
We're not doing it at all, except apparently for this one place. Your
claim that this is an intentional prohibition is without a shred of
corroborating evidence. To believe this is really a problem, you'd
have to believe that someone will someday want to make default values
- but not constraints - apply to row types (because, of course, if no
one ever makes default values apply to row types, then this isn't any
more of a problem than it is today, and if they make constraints -
even NOT NULL constraints - apply to row types, then they'll need
logic to recurse through to tables that use the row type anyway).
Considering that this code has been this way for almost seven years
and that you've not so far offered so much as a shred of evidence in
the code, the documentation, the commit logs, or the mailing list
archives that anyone ever put this arbitrary wart in there for that
reason, or any reason other than inadvertency, I find that altogether
insufficient reason to revert the patch.

I am also pretty dubious that the spec requires this behavior, even if
we did support defaults on row types. Please produce the evidence.
In essence, what you're arguing is that the spec requires us to treat
every instance of a row-type as if it were part of the table that gave
rise to that row-type. Is "UPDATE foo SET a = 1" required to update
every copy of that rowtype everywhere in the system? I bet it isn't.
I think you're conflating the table with its row type, and I'd like to
see some prior writing indicating otherwise.

For those following along at home who may wish to express an opinion,
perhaps a brief review of the behavior change we're arguing about will
be helpful. Prior to this patch, if foo was used as a type in some
other table, this would work:

ALTER TABLE foo ADD COLUMN bar integer;

And this would work:

ALTER TABLE foo ADD COLUMN bar integer DEFAULT null;

But this would fail:

ALTER TABLE foo ADD COLUMN bar integer DEFAULT 5;

...and specifically, it would give you this error message:

cannot alter table "%s" because column "%s"."%s" uses its rowtype

Now, at the very least, that error message sucks, because clearly you
*could* alter that table; you could even add that specific column, and
you could subsequently set a default on it. You just couldn't do both
at the same time. With this patch, the operation succeeds: the rows
in the table are updated with the new default, but instances of the
row type in other tables are not updated, so they effectively have a
NULL in that column. That doesn't seem like a problem to me, because
INSERT statements also ignore the column defaults, so the behavior
you're getting from ALTER TABLE / ADD COLUMN is consistent with what
INSERT does, but Tom disagrees. Anyone else want to weigh in?

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