Thread: Re: [GENERAL] I think this is a BUG?
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@gmail.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-----