Thread: Re: [BUGS] BUG #1290: Default value and ALTER...TYPE

Re: [BUGS] BUG #1290: Default value and ALTER...TYPE

From
Tom Lane
Date:
"PostgreSQL Bugs List" <pgsql-bugs@postgresql.org> writes:
> troels=# create table lookat_feature(
> troels(#   feature_id char(4),
> troels(#   status varchar(2) default 'TODO'
> troels(# );
> CREATE TABLE
> troels=# alter table lookat_feature
> troels-#   alter column status type varchar(4);
> ALTER TABLE
> troels=# insert into lookat_feature (feature_id) values('B034');
> ERROR:  value too long for type character varying(2)

Hmm.  What's going on here is that the stored default expression is
actually of the form('TODO'::varchar)::varchar(2)
where you don't see the coercion to varchar(2) in \d becayuse ruleutils.c
doesn't show implicit casts.  After the ALTER COLUMN it's of the form(('TODO'::varchar)::varchar(2))::varchar(4)
which of course will give an error when used.

Possibly we should make ALTER COLUMN strip any implicit coercions that
appear at the top level of the default expression before it adds on the
implicit coercion to the new column datatype.  I am not sure that this
is a good idea, however; it seems like it might alter the semantics in
unexpected ways.  (The default expression could potentially come through
differently than an actually stored value of the column would do.)

The alternative would seem to be decreeing that this is not a bug.

Comments anyone?
        regards, tom lane


Re: [BUGS] BUG #1290: Default value and ALTER...TYPE

From
Rod Taylor
Date:
On Wed, 2004-10-20 at 14:07, Tom Lane wrote:
> "PostgreSQL Bugs List" <pgsql-bugs@postgresql.org> writes:
> > troels=# create table lookat_feature(
> > troels(#   feature_id char(4),
> > troels(#   status varchar(2) default 'TODO'

> The alternative would seem to be decreeing that this is not a bug.
> 
> Comments anyone?

I think the bug is that default takes a value which does not fit the
columns data type.

bric=# create table test (col numeric(6,2) DEFAULT '1023456632');
CREATE TABLE
bric=# insert into test default values;
ERROR:  numeric field overflow
DETAIL:  The absolute value is greater than or equal to 10^9 for field
with precision 6, scale 2.





Re: [BUGS] BUG #1290: Default value and ALTER...TYPE

From
Tom Lane
Date:
Rod Taylor <pg@rbt.ca> writes:
> On Wed, 2004-10-20 at 14:07, Tom Lane wrote:
>> "PostgreSQL Bugs List" <pgsql-bugs@postgresql.org> writes:
>>> troels=# create table lookat_feature(
>>> troels(#   feature_id char(4),
>>> troels(#   status varchar(2) default 'TODO'

> I think the bug is that default takes a value which does not fit the
> columns data type.

You could argue that this CREATE TABLE should have failed, but I think
it's an orthogonal issue.  It's easy to think of cases where a default
expression will fail only some of the time.  For instance
mycol int2 default 100000 * random()

which might well pass muster if CREATE TABLE checks it, and yet would
fail more than half the time in use.  This is a pretty bogus example of
course, but I think that more-plausible examples could be invented
involving timestamps and now().

I think the real issue posed by Troels' example is that data values
stored within the converted column will be converted from the original
column datatype to the new type.  If we change the handling of the
default expression to avoid this failure, then the default expression
will not be converted in quite the same way.  Maybe that is a good
thing, or maybe it's not.  I'm not quite sold on either viewpoint...
        regards, tom lane


Re: [BUGS] BUG #1290: Default value and ALTER...TYPE

From
Tom Lane
Date:
Troels Arvin <troels@arvin.dk> writes:
> On Wed, 20 Oct 2004 14:07:29 -0400, Tom Lane wrote:
>> Hmm.  What's going on here is that the stored default expression is
>> actually of the form
>> ('TODO'::varchar)::varchar(2)

> Would it be possible to check the compatibility of a default value for
> the associated column?

I think that would introduce as many problems as it would fix.  AFAICS
the only way to make such a check is to evaluate the expression and see
what happens.  There are significant cases in which this is a bad idea
--- consider "default nextval('seq')".  I think people would be
justifiably upset if we changed the state of their sequences just
through installing such a default.

Another issue: consider a column definition likefoo integer not null
The default (NULL) is explicitly not a legal value for the column.
But this is not wrong, nor even bad practice.  The intent may be to
force users to explicitly supply a value in all cases.  Suppose that
you want to force that, but you want it to be okay to explicitly supply
NULL --- then you can't use "not null", so AFAICS the only way is to
specify a default value that will fail if used.  This works:

regression=# create table t1(f1 int default 'please supply a value'::text::int);
CREATE TABLE
regression=# insert into t1 default values;
ERROR:  invalid input syntax for integer: "please supply a value"
regression=#
        regards, tom lane


Re: [BUGS] BUG #1290: Default value and ALTER...TYPE

From
Dennis Bjorklund
Date:
On Thu, 21 Oct 2004, Tom Lane wrote:

> > Would it be possible to check the compatibility of a default value for
> > the associated column?
> 
> I think that would introduce as many problems as it would fix.  AFAICS
> the only way to make such a check is to evaluate the expression and see
> what happens.

The problem in the reported case was that the default value was a string
and pg infered that the default value should be a varchar(2) because that
was the column type. The problem I assume is only when the type is unknown
for pg, for values with a known type I hope that is the type that is used. 
The typecast to the column type should be added on when we use the default 
value and not before.

Maybe one could come up with some method to give the default value the
most general type. For a string we could give it the type text, for a
number we could give it say the smallest int type that fits it (when it is
an integer).

If one have a default value like '2' it would get the type text but that 
would still work even if the column is a int column, wouldn't it?

I assume here that we must give the default value a real type. Values of 
type unknown are hard to handle.

-- 
/Dennis Björklund



Re: [BUGS] BUG #1290: Default value and ALTER...TYPE

From
Kevin Brown
Date:
Tom Lane wrote:
> "PostgreSQL Bugs List" <pgsql-bugs@postgresql.org> writes:
> > troels=# create table lookat_feature(
> > troels(#   feature_id char(4),
> > troels(#   status varchar(2) default 'TODO'
> > troels(# );
> > CREATE TABLE
> > troels=# alter table lookat_feature
> > troels-#   alter column status type varchar(4);
> > ALTER TABLE
> > troels=# insert into lookat_feature (feature_id) values('B034');
> > ERROR:  value too long for type character varying(2)
> 
> Hmm.  What's going on here is that the stored default expression is
> actually of the form
>     ('TODO'::varchar)::varchar(2)
> where you don't see the coercion to varchar(2) in \d becayuse ruleutils.c
> doesn't show implicit casts.  After the ALTER COLUMN it's of the form
>     (('TODO'::varchar)::varchar(2))::varchar(4)
> which of course will give an error when used.
> 
> Possibly we should make ALTER COLUMN strip any implicit coercions that
> appear at the top level of the default expression before it adds on the
> implicit coercion to the new column datatype.  I am not sure that this
> is a good idea, however; it seems like it might alter the semantics in
> unexpected ways.  (The default expression could potentially come through
> differently than an actually stored value of the column would do.)
> 
> The alternative would seem to be decreeing that this is not a bug.
> 
> Comments anyone?

I think the conversion from 'TODO' to varchar(2) and then to
varchar(4) is much more surprising than converting 'TODO' directly to
varchar(4) after the ALTER TABLE.  In short, as a DBA I would expect
the database to do any conversion into the column target type based on
the original specified default value, and not some intermediate form
that exists only because of the history of the column's datatype.

So, my vote is for the form to be ('TODO'::varchar)::varchar(4) after
the ALTER TABLE in the example.


FWIW...



-- 
Kevin Brown                          kevin@sysexperts.com


Re: [BUGS] BUG #1290: Default value and ALTER...TYPE

From
Neil Conway
Date:
Tom Lane wrote:
> Possibly we should make ALTER COLUMN strip any implicit coercions that
> appear at the top level of the default expression before it adds on the
> implicit coercion to the new column datatype.

That seems like a kludge. When processing a column default expression, we:

(1) Accept the default's raw parsetree from the parser
(2) Convert it to a cooked parsetree via transformExpr()
(3) Add a coercion to the table's column type

Can't we save the cooked parsetree that we produced in #2? That would 
mean we can just reuse the cooked parsetree (w/o the coercion) and add a 
coercion to the correct column type @ ALTER TABLE time.

-Neil


Re: [BUGS] BUG #1290: Default value and ALTER...TYPE

From
Dennis Bjorklund
Date:
On Sun, 24 Oct 2004, Neil Conway wrote:

> (1) Accept the default's raw parsetree from the parser
> (2) Convert it to a cooked parsetree via transformExpr()
> (3) Add a coercion to the table's column type
> 
> Can't we save the cooked parsetree that we produced in #2?

One could even save the string as it was before parsning if one wants to 
(could make it easier to edit in a graphical client like pgadmin3).

-- 
/Dennis Björklund



Re: [BUGS] BUG #1290: Default value and ALTER...TYPE

From
Tom Lane
Date:
Neil Conway <neilc@samurai.com> writes:
> Tom Lane wrote:
>> Possibly we should make ALTER COLUMN strip any implicit coercions that
>> appear at the top level of the default expression before it adds on the
>> implicit coercion to the new column datatype.

> That seems like a kludge. When processing a column default expression, we:

> (1) Accept the default's raw parsetree from the parser
> (2) Convert it to a cooked parsetree via transformExpr()
> (3) Add a coercion to the table's column type

> Can't we save the cooked parsetree that we produced in #2?

Not without an initdb (to have another column to put it in).  And it
would produce exactly the same result anyway, because the only way there
could be implicit coercion steps at the top of the expression is because
step 3 put them there.
        regards, tom lane


Re: [BUGS] BUG #1290: Default value and ALTER...TYPE

From
Dennis Bjorklund
Date:
On Sun, 24 Oct 2004, Tom Lane wrote:

> > (1) Accept the default's raw parsetree from the parser
> > (2) Convert it to a cooked parsetree via transformExpr()
> > (3) Add a coercion to the table's column type
> 
> > Can't we save the cooked parsetree that we produced in #2?
> 
> Not without an initdb (to have another column to put it in).  And it
> would produce exactly the same result anyway, because the only way there
> could be implicit coercion steps at the top of the expression is because
> step 3 put them there.

Yes, and he suggested to not perform step 3. Instead one need to do that 
when the default value is used.

-- 
/Dennis Björklund



Re: [BUGS] BUG #1290: Default value and ALTER...TYPE

From
Neil Conway
Date:
On Mon, 2004-10-25 at 00:30, Tom Lane wrote:
> Not without an initdb (to have another column to put it in).

We're already requiring an initdb for beta4; if this is the right way to
fix this (and I'm not insisting that it is), then ISTM we can just push
back beta4 a few days.

> And it
> would produce exactly the same result anyway, because the only way there
> could be implicit coercion steps at the top of the expression is because
> step 3 put them there.

Per your earlier comment: "I am not sure that this is a good idea,
however; it seems like it might alter the semantics in
unexpected ways.  (The default expression could potentially come through
differently than an actually stored value of the column would do.)"

So you can't have it both ways :)

I think it's somewhat fragile to remove the coercion by hand at ALTER
TABLE time, but as you say, I can't think of a reason why it wouldn't
work.

-Neil




Re: [BUGS] BUG #1290: Default value and ALTER...TYPE

From
Tom Lane
Date:
Neil Conway <neilc@samurai.com> writes:
> On Mon, 2004-10-25 at 00:30, Tom Lane wrote:
>> And it
>> would produce exactly the same result anyway, because the only way there
>> could be implicit coercion steps at the top of the expression is because
>> step 3 put them there.

> Per your earlier comment: "I am not sure that this is a good idea,
> however; it seems like it might alter the semantics in
> unexpected ways.  (The default expression could potentially come through
> differently than an actually stored value of the column would do.)"

> So you can't have it both ways :)

Sure I can.  The method you propose will produce exactly the same
results as what's in there.  I am still a bit concerned about
nonintuitive results in some cases, but changing it like this wouldn't
fix that; it's just a different way of getting to the same place.

The bottom line here is what will produce the least surprise in the most
cases.  The case that actually convinced me to do it was thinking about
serial columns.  As it is now, "ALTER COLUMN TYPE bigint" will
successfully convert a serial to a bigserial; before, it wouldn't,
because the result of nextval() would still get squeezed down to int4.
        regards, tom lane