I think this is a BUG?

Hi,

I find something very interesting which I think is a bug and I want to
discuss it.
---------------------------------------------------------------------------
Here is the example1:
1.I create a table without PK;
2.Insert 1 row;
3.I ADD PK;
4.When I select all ID's are with NULL values, but the column is NOT NULL;
5.But If I try to create a regular NOT NULL column the postgres stops
me(as it should) with ERROR "ERROR: column "id" contains null values".

I think that PG should create squence and set it as default, fill the
rows as it does in example2 from the sequence and then make the column
NOT NULL;

r=# begin;
BEGIN
r=# SELECT version();
version
-------------------------------------------------------------------------------------------------
PostgreSQL 8.2.7 on amd64-portbld-freebsd6.3, compiled by GCC cc (GCC)
3.4.6 [FreeBSD] 20060305
(1 row)

r=# CREATE TABLE test( a text, b int);
CREATE TABLE
r=# INSERT INTO test VALUES ('test',1);
INSERT 0 1
r=# ALTER TABLE test ADD COLUMN id INT NOT NULL PRIMARY KEY;
NOTICE: ALTER TABLE / ADD PRIMARY KEY will create implicit index
"test_pkey" for table "test"
ALTER TABLE
r=# SELECT * FROM test WHERE id is null;
a | b | id
------+---+----
test | 1 |
(1 row)

r=# \d test;
Table "public.test"
Column | Type | Modifiers
--------+---------+-----------
a | text |
b | integer |
id | integer | not null
Indexes:
"test_pkey" PRIMARY KEY, btree (id)

regbgrgr=# ALTER TABLE test ADD COLUMN not_null INT NOT NULL ;
ERROR: column "id" contains null values

==========================================EXAMPLE2======================================
Example2:
In this case the postgress fill the NOT NULL column ID from the sequence.

r=# begin;
BEGIN
r=# SELECT version();
version
-------------------------------------------------------------------------------------------------
PostgreSQL 8.2.7 on amd64-portbld-freebsd6.3, compiled by GCC cc (GCC)
3.4.6 [FreeBSD] 20060305
(1 row)

r=# CREATE TABLE test( a text, b int);
CREATE TABLE
r=# INSERT INTO test VALUES ('test',1);
INSERT 0 1
regbgrgr=# SELECT * from test;
a | b
------+---
test | 1
(1 row)

r=# CREATE SEQUENCE test_id_seq;
CREATE SEQUENCE
r=# ALTER TABLE test ADD COLUMN id INT NOT NULL PRIMARY KEY default
nextval('test_id_seq'::regclass);
NOTICE: ALTER TABLE / ADD PRIMARY KEY will create implicit index
"test_pkey" for table "test"
ALTER TABLE
r=# SELECT * from test;
a | b | id
------+---+----
test | 1 | 1
(1 row)

r=# \d test
Table "public.test"
Column | Type | Modifiers
--------+---------+---------------------------------------------------
a | text |
b | integer |
id | integer | not null default nextval('test_id_seq'::regclass)
Indexes:
"test_pkey" PRIMARY KEY, btree (id)

r=# ALTER TABLE test ADD COLUMN not_null int NOT NULL;
ERROR: column "not_null" contains null values

My question is why didn't PG create the sequence and fill the values in
the first example.
And why creates an NOT NULL column with null values in it!

Best Regards,
Kaloyan Iliev

Kaloyan Iliev wrote:
> Hi,
>
> I find something very interesting which I think is a bug and I want to
> discuss it.
> ---------------------------------------------------------------------------
> Here is the example1:
> 1.I create a table without PK;
> 2.Insert 1 row;
> 3.I ADD PK;
> 4.When I select all ID's are with NULL values, but the column is NOT NULL;
> 5.But If I try to create a regular NOT NULL column the postgres stops
> me(as it should) with ERROR "ERROR: column "id" contains null values".

> PostgreSQL 8.2.7 on amd64-portbld-freebsd6.3, compiled by GCC cc (GCC)
> 3.4.6 [FreeBSD] 20060305

> r=# CREATE TABLE test( a text, b int);
> CREATE TABLE
> r=# INSERT INTO test VALUES ('test',1);
> INSERT 0 1
> r=# ALTER TABLE test ADD COLUMN id INT NOT NULL PRIMARY KEY;
> NOTICE: ALTER TABLE / ADD PRIMARY KEY will create implicit index
> "test_pkey" for table "test"
> ALTER TABLE
> r=# SELECT * FROM test WHERE id is null;
> a | b | id
> ------+---+----
> test | 1 |

Well that's clearly broken (seems to do the same in 8.3 too). I've cc-ed
the hackers list so they can investigate further. Presumably the "not
null" test is being missed somehow when the column is initially created.

> r=# ALTER TABLE test ADD COLUMN not_null int NOT NULL;
> ERROR: column "not_null" contains null values
>
> My question is why didn't PG create the sequence and fill the values in
> the first example.

Not sure what you mean here.

> And why creates an NOT NULL column with null values in it!

Because it hasn't got any other value to put in it. Try:
ALTER TABLE test ADD COLUMN id3 integer NOT NULL default 0;

--
Richard Huxton
Archonet Ltd

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Thu, Apr 24, 2008 at 7:13 PM, Richard Huxton wrote:
> Kaloyan Iliev wrote:
> > r=# CREATE TABLE test( a text, b int);
> > CREATE TABLE
> > r=# INSERT INTO test VALUES ('test',1);
> > INSERT 0 1
> > r=# ALTER TABLE test ADD COLUMN id INT NOT NULL PRIMARY KEY;
> > NOTICE: ALTER TABLE / ADD PRIMARY KEY will create implicit index
> "test_pkey" for table "test"
> > ALTER TABLE
> > r=# SELECT * FROM test WHERE id is null;
> > a | b | id
> > ------+---+----
> > test | 1 |
> >
>
> Well that's clearly broken (seems to do the same in 8.3 too). I've cc-ed
> the hackers list so they can investigate further. Presumably the "not null"
> test is being missed somehow when the column is initially created.
>

Confirmed on HEAD.

I think I know why this is happening. When ALTER TABLE ... ADD COLUMN
... PRIMARY KEY is transformed, you end up with ADD COLUMN, followed
by an ADD INDEX.

transformIndexConstraint sets the is_not_null flag on the ColumnDefs
associated with the primary key. That works great in a CREATE TABLE
context, but in ADD COLUMN, when we haven't created the column yet,
this means that the column is created with attnotnull set to true,
which tricks DefineIndex into thinking that the column already has a
NOT NULL constraint.

So the NOT NULL constraint never gets added and hence the check for
NULL values never occurs, which leaves you with a column which is
bogusly marked "NOT NULL".

I'm currently working on a solution for this, and I've thought of a
couple different general approaches:

1. Teach transformIndexConstraint not to set ->is_not_null for
primary keys on columns added with ALTER TABLE. This way, defineIndex
will add the NOT NULL constraint as normal while defining the primary
key. We could try scanning for columns to see whether they already
exist, or rig up some kind of communication path between
transformAlterTableStmt and transformIndexConstraint ...

2. Delay the logic in transformAlterTableStmt which pulls ADD COLUMN
... NOT NULL into a separate command, so that it occurs *after* we've
called transformIndexConstraints. That way, transformAlterTableStmt
will pick up on the fact that transformIndexConstraint has set the
column's is_not_null field, and create the AT_SetNotNull command
pre-emptively, which means that defineIndex doesn't have any extra
work to do ...

3. Force defineIndex and ATExecSetNotNull to add the NOT NULL
constraint, even if the column already has attnotnull = true.

Of these two, I'm leaning towards 2 because it seems less convoluted
than 1 and less clumsy/wasteful than 3. However, I'm keen to hear
what others have to say about it.

Cheers,
BJ
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (GNU/Linux)
Comment: http://getfiregpg.org

iD8DBQFIEKdq5YBsbHkuyV0RAvOuAJ9b63xqPcomtTDQYLeL8P2W1+rEBQCfWZFy
rL3Wld2xIc5bOEPnSSiEbbE=
=VTFo
-----END PGP SIGNATURE-----

"Brendan Jurd" <direvus(at)gmail(dot)com> writes:
> transformIndexConstraint sets the is_not_null flag on the ColumnDefs
> associated with the primary key. That works great in a CREATE TABLE
> context, but in ADD COLUMN, when we haven't created the column yet,
> this means that the column is created with attnotnull set to true,
> which tricks DefineIndex into thinking that the column already has a
> NOT NULL constraint.

Huh? The attnotnull bit *is* the constraint, there is no other
representation. (There has been talk of making a pg_constraint
entry but it isn't done today.)

I think the bug here is that ADD COLUMN NOT NULL has to fail if
there's not a default expression supplied (except maybe we could
allow it if the table contains no rows). ISTM we got this right
in the past, wonder why it's not working now ...

regards, tom lane

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Fri, Apr 25, 2008 at 2:27 AM, Tom Lane wrote:
> "Brendan Jurd" writes:
> > transformIndexConstraint sets the is_not_null flag on the ColumnDefs
> > associated with the primary key. That works great in a CREATE TABLE
> > context, but in ADD COLUMN, when we haven't created the column yet,
> > this means that the column is created with attnotnull set to true,
> > which tricks DefineIndex into thinking that the column already has a
> > NOT NULL constraint.
>
> Huh? The attnotnull bit *is* the constraint, there is no other
> representation. (There has been talk of making a pg_constraint
> entry but it isn't done today.)
>

Yes, I know. Sorry if my terminology was confusing. When I said
"constraint" I didn't mean a pg_constraint record, but the de facto
constraint created by ATExecSetNotNull -- it sets attnotnull to true
and checks that all of the values in the table are not null. It
doesn't do any of that if attnotnull has been set to true already,
which is how we arrive at the bug.

What we have here is a path where attnotnull can be set to true,
without the check ever occurring.

> I think the bug here is that ADD COLUMN NOT NULL has to fail if
> there's not a default expression supplied (except maybe we could
> allow it if the table contains no rows). ISTM we got this right
> in the past, wonder why it's not working now ...
>

No, ADD COLUMN NOT NULL is fine. It does fail if you don't supply a
default. This problem is particular to ADD COLUMN PRIMARY KEY.

Hmm, it's obvious that I didn't manage to convey my meaning in my
previous post. =/

Perhaps I should just post a patch; the changes to the code might
paint a clearer picture than my prose.

Cheers,
BJ
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (GNU/Linux)
Comment: http://getfiregpg.org

iD8DBQFIELml5YBsbHkuyV0RAg7xAKCYt9+BidrOKLthgN4SVdiEGApNsQCfa8T1
1D2DdttGCNMu1cXa4DZZnYQ=
=xkpm
-----END PGP SIGNATURE-----

I wrote:
> I think the bug here is that ADD COLUMN NOT NULL has to fail if
> there's not a default expression supplied (except maybe we could
> allow it if the table contains no rows). ISTM we got this right
> in the past, wonder why it's not working now ...

Hm, we do get it right if PRIMARY KEY isn't in the picture:

regression=# create table t1 (f1 int);
CREATE TABLE
regression=# insert into t1 values(1);
INSERT 0 1
regression=# alter table t1 add column f2 int not null;
ERROR: column "f2" contains null values

The above is correct, but if I now try

regression=# alter table t1 add column f2 int primary key;
NOTICE: ALTER TABLE / ADD PRIMARY KEY will create implicit index "t1_pkey" for table "t1"
ALTER TABLE
regression=# \d t1
Table "public.t1"
Column | Type | Modifiers
--------+---------+-----------
f1 | integer |
f2 | integer | not null
Indexes:
"t1_pkey" PRIMARY KEY, btree (f2)

So somehow the constraint-validation code isn't getting applied in
this case. I suspect you'll find it's a pretty localized fix.

regards, tom lane

I wrote:
> So somehow the constraint-validation code isn't getting applied in
> this case. I suspect you'll find it's a pretty localized fix.

I traced through this and found that for

alter table t1 add column f2 int not null;

transformAlterTableStmt will produce an AT_AddColumn subcommand
containing a ColumnDef with is_not_null = false, followed by an
AT_SetNotNull subcommand. But for

alter table t1 add column f2 int primary key;

it produces an AT_AddColumn subcommand containing a ColumnDef with
is_not_null = true, followed by an AT_AddIndex subcommand.

This is not super consistent, and maybe should be cleaned up;
but the intent is perfectly clear in both cases so I think tablecmds.c
should be able to do the right thing with either.

It looks to me that the appropriate fix involves doing

tab->new_notnull |= colDef->is_not_null;

somewhere in ATExecAddColumn; but I haven't tested this much.

regards, tom lane

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Fri, Apr 25, 2008 at 3:46 AM, Tom Lane wrote:
> alter table t1 add column f2 int not null;
>
> transformAlterTableStmt will produce an AT_AddColumn subcommand
> containing a ColumnDef with is_not_null = false, followed by an
> AT_SetNotNull subcommand. But for
>

Yep, that's what I was trying to describe in my first post.

The ADD COLUMN code checks to see if the new column has been set to
is_not_null = true by transformColumnDefinition. That's at
parse_utilcmd.c:1738.

If it detects a NOT NULL column, it creates the new AT_SetNotNull
subcommand and adds it to the list, and sets the column's is_not_null
back to false.

> alter table t1 add column f2 int primary key;
>
> it produces an AT_AddColumn subcommand containing a ColumnDef with
> is_not_null = true, followed by an AT_AddIndex subcommand.
>

This is because transformIndexConstraint sets is_not_null = true for
any columns in a primary key constraint. That's at
parse_utilcmd.c:1127.

Note that this happens *after* transformAlterTableStmt performs the
check I mentioned above.

> This is not super consistent, and maybe should be cleaned up;
> but the intent is perfectly clear in both cases so I think tablecmds.c
> should be able to do the right thing with either.
>
> It looks to me that the appropriate fix involves doing
>
> tab->new_notnull |= colDef->is_not_null;
>
> somewhere in ATExecAddColumn; but I haven't tested this much.

Makes sense to me. I'll give it go.

Cheers,
BJ
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (GNU/Linux)
Comment: http://getfiregpg.org

iD8DBQFIEMqL5YBsbHkuyV0RAhjMAJ4lA0C9GQAcHFyFdom70OPgo+jzPQCg8eo2
ZVW9YjxpcvQVQmTSde5hApI=
=5Kzi
-----END PGP SIGNATURE-----

On Apr 24, 2008, at 10:11 AM, Kaloyan Iliev wrote:
> regbgrgr=# ALTER TABLE test ADD COLUMN not_null INT NOT NULL ;
> ERROR: column "id" contains null values
>
> ==========================================EXAMPLE2====================
> ==================
> Example2:
> In this case the postgress fill the NOT NULL column ID from the
> sequence.

What sequence? You never told it you wanted one. A PRIMARY KEY
doesn't automatically add a sequence nor does a NOT NULL constraint,
the serial type does that but you defined the column as type int, not
as type serial.

Alban Hertroys

--
If you can't see the forest for the trees,
cut the trees and you'll see there is no forest.

!DSPAM:737,4810d219927662597012045!