Re: [HACKERS] I think this is a BUG?

-----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-----