Bug in pgAdminIII or in pg 8 beta3 ?

The following bug has been logged online:

Bug reference: 1290
Logged by: Troels Arvin

Email address: troels(at)arvin(dot)dk

PostgreSQL version: 8.0 Beta

Operating system: Linux, Fedora Core 2 + stuff from Red Hat Rawhide

Description: Default value and ALTER...TYPE

Details:

In latest CVS (updated 2004-10-20 18:30 CEST), a too-large default column
value seems to block the complete effects of an ALTER TABLE ... ALTER COLUMN
... TYPE operation, see below:

troels=# select version();
version
---------------------------------------------------------------------------
-----------------------------
PostgreSQL 8.0.0beta3 on i686-pc-linux-gnu, compiled by GCC gcc (GCC) 3.4.2
20040907 (Red Hat 3.4.2-2)
(1 row)

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=# \d lookat_feature
Table "public.lookat_feature"
Column | Type | Modifiers
------------+----------------------+-----------------------------------
feature_id | character(4) |
status | character varying(4) | default 'TODO'::character varying

troels=# insert into lookat_feature (feature_id) values('B034');
ERROR: value too long for type character varying(2)

If instead, the "DEFAULT 'TODO'" is left out for the "status" column:

troels=# create table lookat_feature(
troels(# feature_id char(4),
troels(# status varchar(2)
troels(# );
CREATE TABLE
troels=# alter table lookat_feature
troels-# alter column status type varchar(4);
ALTER TABLE
troels=# \d lookat_feature
Table "public.lookat_feature"
Column | Type | Modifiers
------------+----------------------+-----------
feature_id | character(4) |
status | character varying(4) |

troels=# insert into lookat_feature (feature_id) values('B034');
INSERT 17073 1

"PostgreSQL Bugs List" <pgsql-bugs(at)postgresql(dot)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(at)postgresql(dot)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.

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? Such a check might probably have to be limited to
checking of scalar default values - not function calls - to make it
doable.
This would be nice anyway, to catch stupid CREATE TABLE errors earlier.

If such a compatibility check could somehow be implemented, then the check
could be applied for

- CREATE TABLE ...
- ALTER TABLE ... ALTER COLUMN ... SET DEFAULT...
- ALTER TABLE ... ALTER COLUMN ... TYPE...
- CREATE DOMAIN ...

--
Greetings from Troels Arvin, Copenhagen, Denmark

Rod Taylor <pg(at)rbt(dot)ca> writes:
> On Wed, 2004-10-20 at 14:07, Tom Lane wrote:
>> "PostgreSQL Bugs List" <pgsql-bugs(at)postgresql(dot)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(at)arvin(dot)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 like
foo 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(at)postgresql(dot)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(at)sysexperts(dot)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(at)samurai(dot)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(at)samurai(dot)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

Hi

I think that is can be a bug... When I create a new database in pg8
beta3 (win32) like this

CREATE DATABASE test
WITH OWNER = admin
ENCODING = 'UNICODE';

System objects became visible in public schema :
tables like pg_ts_cfg , pg_ts_cfgmap, pg_ts_dict ...
functions , operators etc.

but if I create a database from template0 this objects are not visibles

My installation is with problem or this is a bug ?

Thanks in advance

On Thu, 2004-11-04 at 13:06, Luiz K. Matsumura wrote:
> Hi
>
> I think that is can be a bug... When I create a new database in pg8
> beta3 (win32) like this
>
> CREATE DATABASE test
> WITH OWNER = admin
> ENCODING = 'UNICODE';
>
> System objects became visible in public schema :
> tables like pg_ts_cfg , pg_ts_cfgmap, pg_ts_dict ...
> functions , operators etc.

I don't think those are system objects, unless they are specific to the
Win32 version. I believe they are created when you use tsearch2 in a
database. You must have done that some time in template1.

> but if I create a database from template0 this objects are not visibles
>
> My installation is with problem or this is a bug ?

No bug; these objects exist in the template1 database - someone created
them there - so they get copied into every new database. This is a
_feature_. It lets you ensure that every new database contains certain
objects.

If you don't want them in new databases, delete them from template1.

Oliver Elphick

On Thu, 2004-11-04 at 13:49, Oliver Elphick wrote:
> On Thu, 2004-11-04 at 13:06, Luiz K. Matsumura wrote:
> > Hi
> >
> > I think that is can be a bug... When I create a new database in pg8
> > beta3 (win32) like this
> >
> > CREATE DATABASE test
> > WITH OWNER = admin
> > ENCODING = 'UNICODE';
> >
> > System objects became visible in public schema :
> > tables like pg_ts_cfg , pg_ts_cfgmap, pg_ts_dict ...
> > functions , operators etc.
>
> I don't think those are system objects, unless they are specific to the
> Win32 version. I believe they are created when you use tsearch2 in a
> database. You must have done that some time in template1.
>
> > but if I create a database from template0 this objects are not visibles
> >
> > My installation is with problem or this is a bug ?
>
> No bug; these objects exist in the template1 database - someone created
> them there - so they get copied into every new database. This is a
> _feature_. It lets you ensure that every new database contains certain
> objects.
>

I believe tsearch2 is part of the install in Magnus' .mxi Windows
Installer version.

As Olly says, not a bug.

--
Best Regards, Simon Riggs