From 0a02d47a113ae67b35c5fa060bac6114609f6c5b Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 2 Jan 2003 19:29:22 +0000 Subject: [PATCH] Enforces NOT NULL constraints to be applied against new PRIMARY KEY columns in DefineIndex. So, ALTER TABLE ... PRIMARY KEY will now automatically add the NOT NULL constraint. It appeared the alter_table regression test wanted this to occur, as after the change the regression test better matched in inline 'fails'/'succeeds' comments. Rod Taylor --- src/backend/commands/indexcmds.c | 50 ++++++++++++++++++++++- src/backend/parser/analyze.c | 36 +++++----------- src/test/regress/expected/alter_table.out | 25 +++++++++--- src/test/regress/expected/inherit.out | 5 +++ src/test/regress/sql/alter_table.sql | 6 ++- src/test/regress/sql/inherit.sql | 4 ++ 6 files changed, 91 insertions(+), 35 deletions(-) diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index d46b7a389a..df7e358b89 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/commands/indexcmds.c,v 1.95 2002/12/15 16:17:39 tgl Exp $ + * $Header: /cvsroot/pgsql/src/backend/commands/indexcmds.c,v 1.96 2003/01/02 19:29:22 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -19,11 +19,13 @@ #include "catalog/catalog.h" #include "catalog/catname.h" #include "catalog/dependency.h" +#include "catalog/heap.h" #include "catalog/index.h" #include "catalog/namespace.h" #include "catalog/pg_opclass.h" #include "catalog/pg_proc.h" #include "commands/defrem.h" +#include "commands/tablecmds.h" #include "executor/executor.h" #include "miscadmin.h" #include "optimizer/clauses.h" @@ -165,6 +167,50 @@ DefineIndex(RangeVar *heapRelation, CheckPredicate(cnfPred, rangetable, relationId); } + /* + * Check that all of the attributes in a primary key are marked + * as not null, otherwise attempt to ALTER TABLE .. SET NOT NULL + */ + if (primary && !IsFuncIndex(attributeList)) + { + List *keys; + + foreach(keys, attributeList) + { + IndexElem *key = (IndexElem *) lfirst(keys); + HeapTuple atttuple; + + /* System attributes are never null, so no problem */ + if (SystemAttributeByName(key->name, rel->rd_rel->relhasoids)) + continue; + + atttuple = SearchSysCacheAttName(relationId, key->name); + if (HeapTupleIsValid(atttuple)) + { + if (! ((Form_pg_attribute) GETSTRUCT(atttuple))->attnotnull) + { + /* + * Try to make it NOT NULL. + * + * XXX: Shouldn't the ALTER TABLE .. SET NOT NULL cascade + * to child tables? Currently, since the PRIMARY KEY + * itself doesn't cascade, we don't cascade the notnull + * constraint either; but this is pretty debatable. + */ + AlterTableAlterColumnSetNotNull(relationId, false, + key->name); + } + ReleaseSysCache(atttuple); + } + else + { + /* This shouldn't happen if parser did its job ... */ + elog(ERROR, "DefineIndex: column \"%s\" named in key does not exist", + key->name); + } + } + } + /* * Prepare arguments for index_create, primarily an IndexInfo * structure @@ -296,7 +342,7 @@ FuncIndexArgs(IndexInfo *indexInfo, tuple = SearchSysCacheAttName(relId, arg); if (!HeapTupleIsValid(tuple)) - elog(ERROR, "DefineIndex: attribute \"%s\" not found", arg); + elog(ERROR, "DefineIndex: column \"%s\" named in key does not exist", arg); att = (Form_pg_attribute) GETSTRUCT(tuple); indexInfo->ii_KeyAttrNumbers[nargs] = att->attnum; argTypes[nargs] = att->atttypid; diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c index e771d66659..2609de18f5 100644 --- a/src/backend/parser/analyze.c +++ b/src/backend/parser/analyze.c @@ -6,7 +6,7 @@ * Portions Copyright (c) 1996-2002, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * - * $Header: /cvsroot/pgsql/src/backend/parser/analyze.c,v 1.258 2002/12/17 01:18:29 tgl Exp $ + * $Header: /cvsroot/pgsql/src/backend/parser/analyze.c,v 1.259 2003/01/02 19:29:22 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -1083,7 +1083,9 @@ transformIndexConstraints(ParseState *pstate, CreateStmtContext *cxt) /* * Make sure referenced keys exist. If we are making a PRIMARY - * KEY index, also make sure they are NOT NULL. + * KEY index, also make sure they are NOT NULL, if possible. + * (Although we could leave it to DefineIndex to mark the columns NOT + * NULL, it's more efficient to get it right the first time.) */ foreach(keys, constraint->keys) { @@ -1142,25 +1144,12 @@ transformIndexConstraints(ParseState *pstate, CreateStmtContext *cxt) if (strcmp(key, inhname) == 0) { found = true; - /* - * If the column is inherited, we currently - * have no easy way to force it to be NOT - * NULL. Only way I can see to fix this would - * be to convert the inherited-column info to - * ColumnDef nodes before we reach this point, - * and then create the table from those nodes - * rather than referencing the parent tables - * later. That would likely be cleaner, but - * too much work to contemplate right now. - * Instead, raise an error if the inherited - * column won't be NOT NULL. (Would a WARNING - * be more reasonable?) + * We currently have no easy way to force an + * inherited column to be NOT NULL at creation, if + * its parent wasn't so already. We leave it to + * DefineIndex to fix things up in this case. */ - if (constraint->contype == CONSTR_PRIMARY && - !inhattr->attnotnull) - elog(ERROR, "inherited attribute \"%s\" cannot be a PRIMARY KEY because it is not marked NOT NULL", - inhname); break; } } @@ -1178,15 +1167,10 @@ transformIndexConstraints(ParseState *pstate, CreateStmtContext *cxt) if (HeapTupleIsValid(atttuple)) { found = true; - /* - * We require pre-existing column to be already marked - * NOT NULL. + * If it's not already NOT NULL, leave it to DefineIndex + * to fix later. */ - if (constraint->contype == CONSTR_PRIMARY && - !((Form_pg_attribute) GETSTRUCT(atttuple))->attnotnull) - elog(ERROR, "Existing attribute \"%s\" cannot be a PRIMARY KEY because it is not marked NOT NULL", - key); ReleaseSysCache(atttuple); } } diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index 4d61811045..e0e9b8dc51 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -539,16 +539,23 @@ drop table atacc1; create table atacc1 ( test int ); -- add a primary key constraint alter table atacc1 add constraint atacc_test1 primary key (test); -ERROR: Existing attribute "test" cannot be a PRIMARY KEY because it is not marked NOT NULL +NOTICE: ALTER TABLE / ADD PRIMARY KEY will create implicit index 'atacc_test1' for table 'atacc1' -- insert first value insert into atacc1 (test) values (2); -- should fail insert into atacc1 (test) values (2); +ERROR: Cannot insert a duplicate key into unique index atacc_test1 -- should succeed insert into atacc1 (test) values (4); -- inserting NULL should fail insert into atacc1 (test) values(NULL); --- try adding a primary key oid constraint +ERROR: ExecInsert: Fail to add null value in not null attribute test +-- try adding a second primary key (should fail) +alter table atacc1 add constraint atacc_oid1 primary key(oid); +ERROR: ALTER TABLE / PRIMARY KEY multiple primary keys for table 'atacc1' are not allowed +-- drop first primary key constraint +alter table atacc1 drop constraint atacc_test1 restrict; +-- try adding a primary key on oid (should succeed) alter table atacc1 add constraint atacc_oid1 primary key(oid); NOTICE: ALTER TABLE / ADD PRIMARY KEY will create implicit index 'atacc_oid1' for table 'atacc1' drop table atacc1; @@ -559,7 +566,8 @@ insert into atacc1 (test) values (2); insert into atacc1 (test) values (2); -- add a primary key (fails) alter table atacc1 add constraint atacc_test1 primary key (test); -ERROR: Existing attribute "test" cannot be a PRIMARY KEY because it is not marked NOT NULL +NOTICE: ALTER TABLE / ADD PRIMARY KEY will create implicit index 'atacc_test1' for table 'atacc1' +ERROR: Cannot create unique index. Table contains non-unique values insert into atacc1 (test) values (3); drop table atacc1; -- let's do another one where the primary key constraint fails when added @@ -568,7 +576,8 @@ create table atacc1 ( test int ); insert into atacc1 (test) values (NULL); -- add a primary key (fails) alter table atacc1 add constraint atacc_test1 primary key (test); -ERROR: Existing attribute "test" cannot be a PRIMARY KEY because it is not marked NOT NULL +NOTICE: ALTER TABLE / ADD PRIMARY KEY will create implicit index 'atacc_test1' for table 'atacc1' +ERROR: ALTER TABLE: Attribute "test" contains NULL values insert into atacc1 (test) values (3); drop table atacc1; -- let's do one where the primary key constraint fails @@ -582,17 +591,21 @@ drop table atacc1; create table atacc1 ( test int, test2 int); -- add a primary key constraint alter table atacc1 add constraint atacc_test1 primary key (test, test2); -ERROR: Existing attribute "test" cannot be a PRIMARY KEY because it is not marked NOT NULL +NOTICE: ALTER TABLE / ADD PRIMARY KEY will create implicit index 'atacc_test1' for table 'atacc1' -- try adding a second primary key - should fail alter table atacc1 add constraint atacc_test2 primary key (test); -ERROR: Existing attribute "test" cannot be a PRIMARY KEY because it is not marked NOT NULL +ERROR: ALTER TABLE / PRIMARY KEY multiple primary keys for table 'atacc1' are not allowed -- insert initial value insert into atacc1 (test,test2) values (4,4); -- should fail insert into atacc1 (test,test2) values (4,4); +ERROR: Cannot insert a duplicate key into unique index atacc_test1 insert into atacc1 (test,test2) values (NULL,3); +ERROR: ExecInsert: Fail to add null value in not null attribute test insert into atacc1 (test,test2) values (3, NULL); +ERROR: ExecInsert: Fail to add null value in not null attribute test2 insert into atacc1 (test,test2) values (NULL,NULL); +ERROR: ExecInsert: Fail to add null value in not null attribute test -- should all succeed insert into atacc1 (test,test2) values (4,5); insert into atacc1 (test,test2) values (5,4); diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out index b56798124f..6ca59e799b 100644 --- a/src/test/regress/expected/inherit.out +++ b/src/test/regress/expected/inherit.out @@ -534,3 +534,8 @@ SELECT relname, d.* FROM ONLY d, pg_class where d.tableoid = pg_class.oid; ---------+----+----+----+---- (0 rows) +-- Confirm PRIMARY KEY adds NOT NULL constraint to child table +CREATE TEMP TABLE z (b TEXT, PRIMARY KEY(aa, b)) inherits (a); +NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index 'z_pkey' for table 'z' +INSERT INTO z VALUES (NULL, 'text'); -- should fail +ERROR: ExecInsert: Fail to add null value in not null attribute aa diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index 7fcfb09df2..4d20705f53 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -415,7 +415,11 @@ insert into atacc1 (test) values (2); insert into atacc1 (test) values (4); -- inserting NULL should fail insert into atacc1 (test) values(NULL); --- try adding a primary key oid constraint +-- try adding a second primary key (should fail) +alter table atacc1 add constraint atacc_oid1 primary key(oid); +-- drop first primary key constraint +alter table atacc1 drop constraint atacc_test1 restrict; +-- try adding a primary key on oid (should succeed) alter table atacc1 add constraint atacc_oid1 primary key(oid); drop table atacc1; diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql index c60a89266a..d8d73f8860 100644 --- a/src/test/regress/sql/inherit.sql +++ b/src/test/regress/sql/inherit.sql @@ -92,3 +92,7 @@ SELECT relname, a.* FROM ONLY a, pg_class where a.tableoid = pg_class.oid; SELECT relname, b.* FROM ONLY b, pg_class where b.tableoid = pg_class.oid; SELECT relname, c.* FROM ONLY c, pg_class where c.tableoid = pg_class.oid; SELECT relname, d.* FROM ONLY d, pg_class where d.tableoid = pg_class.oid; + +-- Confirm PRIMARY KEY adds NOT NULL constraint to child table +CREATE TEMP TABLE z (b TEXT, PRIMARY KEY(aa, b)) inherits (a); +INSERT INTO z VALUES (NULL, 'text'); -- should fail