Thread: Re: [BUGS] BUG #1290: Default value and ALTER...TYPE
"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
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.
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
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
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
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
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
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
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
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
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
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