From aa8d3fbf55326aab14a05660e429f908472e15c4 Mon Sep 17 00:00:00 2001 From: Rushabh Lathia Date: Wed, 5 Feb 2025 15:36:43 +0530 Subject: [PATCH 2/3] Support NOT VALID and VALIDATE CONSTRAINT for named NOT NULL constraints. --- src/backend/commands/tablecmds.c | 119 +++++++++++++++++++++--------- src/backend/executor/execMain.c | 3 +- src/backend/parser/gram.y | 4 +- src/bin/psql/describe.c | 10 ++- src/include/catalog/pg_attribute.h | 1 + src/test/regress/expected/constraints.out | 85 +++++++++++++++++++++ src/test/regress/sql/constraints.sql | 40 ++++++++++ 7 files changed, 220 insertions(+), 42 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 933530d..2efbacc 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -405,9 +405,9 @@ static ObjectAddress ATExecValidateConstraint(List **wqueue, bool recurse, bool recursing, LOCKMODE lockmode); static void QueueFKConstraintValidation(List **wqueue, Relation conrel, Relation rel, HeapTuple contuple, LOCKMODE lockmode); -static void QueueCheckConstraintValidation(List **wqueue, Relation conrel, Relation rel, - char *constrName, HeapTuple contuple, - bool recurse, bool recursing, LOCKMODE lockmode); +static void QueueConstraintValidation(List **wqueue, Relation conrel, Relation rel, + char *constrName, HeapTuple contuple, + bool recurse, bool recursing, LOCKMODE lockmode); static int transformColumnNameList(Oid relId, List *colList, int16 *attnums, Oid *atttypids, Oid *attcollids); static int transformFkeyGetPrimaryKey(Relation pkrel, Oid *indexOid, @@ -471,7 +471,7 @@ static void add_column_collation_dependency(Oid relid, int32 attnum, Oid collid) static ObjectAddress ATExecDropNotNull(Relation rel, const char *colName, bool recurse, LOCKMODE lockmode); static void set_attnotnull(List **wqueue, Relation rel, AttrNumber attnum, - LOCKMODE lockmode); + bool skip_validation, LOCKMODE lockmode); static ObjectAddress ATExecSetNotNull(List **wqueue, Relation rel, char *constrname, char *colName, bool recurse, bool recursing, @@ -700,6 +700,7 @@ static List *GetParentedForeignKeyRefs(Relation partition); static void ATDetachCheckNoForeignKeyRefs(Relation partition); static char GetAttributeCompression(Oid atttypid, const char *compression); static char GetAttributeStorage(Oid atttypid, const char *storagemode); +static bool check_for_invalid_notnull(Oid relid, const char *attname); /* ---------------------------------------------------------------- @@ -1317,7 +1318,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId, nncols = AddRelationNotNullConstraints(rel, stmt->nnconstraints, old_notnulls); foreach_int(attrnum, nncols) - set_attnotnull(NULL, rel, attrnum, NoLock); + set_attnotnull(NULL, rel, attrnum, false, NoLock); ObjectAddressSet(address, RelationRelationId, relationId); @@ -6171,7 +6172,8 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap) { Form_pg_attribute attr = TupleDescAttr(newTupDesc, i); - if (attr->attnotnull == ATTRIBUTE_NOTNULL_TRUE && + if ((attr->attnotnull == ATTRIBUTE_NOTNULL_TRUE || + attr->attnotnull == ATTRIBUTE_NOTNULL_INVALID) && !attr->attisdropped) notnull_attrs = lappend_int(notnull_attrs, i); } @@ -7692,7 +7694,7 @@ ATExecDropNotNull(Relation rel, const char *colName, bool recurse, */ static void set_attnotnull(List **wqueue, Relation rel, AttrNumber attnum, - LOCKMODE lockmode) + bool skip_validation, LOCKMODE lockmode) { Form_pg_attribute attr; @@ -7720,14 +7722,19 @@ set_attnotnull(List **wqueue, Relation rel, AttrNumber attnum, attr = (Form_pg_attribute) GETSTRUCT(tuple); Assert(attr->attnotnull == ATTRIBUTE_NOTNULL_FALSE); - attr->attnotnull = ATTRIBUTE_NOTNULL_TRUE; + + if (skip_validation) + attr->attnotnull = ATTRIBUTE_NOTNULL_INVALID; + else + attr->attnotnull = ATTRIBUTE_NOTNULL_TRUE; CatalogTupleUpdate(attr_rel, &tuple->t_self, tuple); /* * If the nullness isn't already proven by validated constraints, have * ALTER TABLE phase 3 test for it. */ - if (wqueue && !NotNullImpliedByRelConstraints(rel, attr)) + if (!skip_validation && + wqueue && !NotNullImpliedByRelConstraints(rel, attr)) { AlteredTableInfo *tab; @@ -7888,7 +7895,7 @@ ATExecSetNotNull(List **wqueue, Relation rel, char *conName, char *colName, RelationGetRelid(rel), attnum); /* Mark pg_attribute.attnotnull for the column */ - set_attnotnull(wqueue, rel, attnum, lockmode); + set_attnotnull(wqueue, rel, attnum, constraint->skip_validation, lockmode); /* * Recurse to propagate the constraint to children that don't have one. @@ -9272,6 +9279,12 @@ ATPrepAddPrimaryKey(List **wqueue, Relation rel, AlterTableCmd *cmd, { AlterTableCmd *newcmd; Constraint *nnconstr; + String *colname = lfirst(lc); + + if (check_for_invalid_notnull(RelationGetRelid(rel), colname->sval)) + ereport(ERROR, + errmsg("column \"%s\" of table \"%s\" is marked as NOT VALID NOT NULL constrint", + colname->sval, RelationGetRelationName(rel))); nnconstr = makeNotNullConstraint(lfirst(lc)); @@ -9285,6 +9298,30 @@ ATPrepAddPrimaryKey(List **wqueue, Relation rel, AlterTableCmd *cmd, } /* + * This function checks for any invalid NOT NULL constraint on the given + * relation and attribute name. It returns true if found, false otherwise. + */ +static bool +check_for_invalid_notnull(Oid relid, const char *attname) +{ + HeapTuple tuple; + bool retval = false; + + tuple = SearchSysCache2(ATTNAME, + ObjectIdGetDatum(relid), + CStringGetDatum(attname)); + if (!HeapTupleIsValid(tuple)) + return false; + if (!((Form_pg_attribute) GETSTRUCT(tuple))->attisdropped && + ((Form_pg_attribute) GETSTRUCT(tuple))->attnotnull == ATTRIBUTE_NOTNULL_INVALID) + retval = true; + + ReleaseSysCache(tuple); + + return retval; +} + +/* * ALTER TABLE ADD INDEX * * There is no such command in the grammar, but parse_utilcmd.c converts @@ -9651,7 +9688,7 @@ ATAddCheckNNConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, * phase 3 to verify existing rows, if needed. */ if (constr->contype == CONSTR_NOTNULL) - set_attnotnull(wqueue, rel, ccon->attnum, lockmode); + set_attnotnull(wqueue, rel, ccon->attnum, ccon->skip_validation, lockmode); ObjectAddressSet(address, ConstraintRelationId, ccon->conoid); } @@ -12079,7 +12116,8 @@ ATExecValidateConstraint(List **wqueue, Relation rel, char *constrName, con = (Form_pg_constraint) GETSTRUCT(tuple); if (con->contype != CONSTRAINT_FOREIGN && - con->contype != CONSTRAINT_CHECK) + con->contype != CONSTRAINT_CHECK && + con->contype != CONSTRAINT_NOTNULL) ereport(ERROR, (errcode(ERRCODE_WRONG_OBJECT_TYPE), errmsg("constraint \"%s\" of relation \"%s\" is not a foreign key or check constraint", @@ -12096,10 +12134,11 @@ ATExecValidateConstraint(List **wqueue, Relation rel, char *constrName, { QueueFKConstraintValidation(wqueue, conrel, rel, tuple, lockmode); } - else if (con->contype == CONSTRAINT_CHECK) + else if (con->contype == CONSTRAINT_CHECK || + con->contype == CONSTRAINT_NOTNULL) { - QueueCheckConstraintValidation(wqueue, conrel, rel, constrName, - tuple, recurse, recursing, lockmode); + QueueConstraintValidation(wqueue, conrel, rel, constrName, + tuple, recurse, recursing, lockmode); } ObjectAddressSet(address, ConstraintRelationId, con->oid); @@ -12217,14 +12256,14 @@ QueueFKConstraintValidation(List **wqueue, Relation conrel, Relation rel, /* * QueueCheckConstraintValidation * - * Add an entry to the wqueue to validate the given check constraint in Phase 3 - * and update the convalidated field in the pg_constraint catalog for the - * specified relation and all its inheriting children. + * Add an entry to the wqueue to validate the given check or notnull constraint + * in Phase 3 and update the convalidated field in the pg_constraint catalog + * for the specified relation and all its inheriting children. */ static void -QueueCheckConstraintValidation(List **wqueue, Relation conrel, Relation rel, - char *constrName, HeapTuple contuple, - bool recurse, bool recursing, LOCKMODE lockmode) +QueueConstraintValidation(List **wqueue, Relation conrel, Relation rel, + char *constrName, HeapTuple contuple, + bool recurse, bool recursing, LOCKMODE lockmode) { Form_pg_constraint con; AlteredTableInfo *tab; @@ -12238,7 +12277,7 @@ QueueCheckConstraintValidation(List **wqueue, Relation conrel, Relation rel, char *conbin; con = (Form_pg_constraint) GETSTRUCT(contuple); - Assert(con->contype == CONSTRAINT_CHECK); + Assert(con->contype == CONSTRAINT_CHECK || con->contype == CONSTRAINT_NOTNULL); /* * If we're recursing, the parent has already done this, so skip it. Also, @@ -12283,21 +12322,31 @@ QueueCheckConstraintValidation(List **wqueue, Relation conrel, Relation rel, } /* Queue validation for phase 3 */ - newcon = (NewConstraint *) palloc0(sizeof(NewConstraint)); - newcon->name = constrName; - newcon->contype = CONSTR_CHECK; - newcon->refrelid = InvalidOid; - newcon->refindid = InvalidOid; - newcon->conid = con->oid; + if (con->contype == CONSTRAINT_CHECK) + { + newcon = (NewConstraint *) palloc0(sizeof(NewConstraint)); + newcon->name = constrName; + newcon->contype = CONSTR_CHECK; + newcon->refrelid = InvalidOid; + newcon->refindid = InvalidOid; + newcon->conid = con->oid; - val = SysCacheGetAttrNotNull(CONSTROID, contuple, - Anum_pg_constraint_conbin); - conbin = TextDatumGetCString(val); - newcon->qual = (Node *) stringToNode(conbin); + val = SysCacheGetAttrNotNull(CONSTROID, contuple, + Anum_pg_constraint_conbin); + conbin = TextDatumGetCString(val); + newcon->qual = (Node *) stringToNode(conbin); - /* Find or create work queue entry for this table */ - tab = ATGetQueueEntry(wqueue, rel); - tab->constraints = lappend(tab->constraints, newcon); + /* Find or create work queue entry for this table */ + tab = ATGetQueueEntry(wqueue, rel); + tab->constraints = lappend(tab->constraints, newcon); + } + else + { + Assert(con->contype == CONSTRAINT_NOTNULL); + + tab = ATGetQueueEntry(wqueue, rel); + tab->verify_new_notnull = true; + } /* * Invalidate relcache so that others see the new validated constraint. diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index d4e09a1..ebd759e 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -1959,7 +1959,8 @@ ExecConstraints(ResultRelInfo *resultRelInfo, { Form_pg_attribute att = TupleDescAttr(tupdesc, attrChk - 1); - if (att->attnotnull == ATTRIBUTE_NOTNULL_TRUE && + if ((att->attnotnull == ATTRIBUTE_NOTNULL_TRUE || + att->attnotnull == ATTRIBUTE_NOTNULL_INVALID) && slot_attisnull(slot, attrChk)) { char *val_desc; diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index d7f9c00..a02e8ac 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -4189,9 +4189,9 @@ ConstraintElem: n->keys = list_make1(makeString($3)); /* no NOT VALID support yet */ processCASbits($4, @4, "NOT NULL", - NULL, NULL, NULL, NULL, + NULL, NULL, NULL, &n->skip_validation, &n->is_no_inherit, yyscanner); - n->initially_valid = true; + n->initially_valid = !n->skip_validation; $$ = (Node *) n; } | UNIQUE opt_unique_null_treatment '(' columnList opt_without_overlaps ')' opt_c_include opt_definition OptConsTableSpace diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c index aa4363b..c5f7fdc 100644 --- a/src/bin/psql/describe.c +++ b/src/bin/psql/describe.c @@ -2107,7 +2107,7 @@ describeOneTableDetails(const char *schemaname, printTableAddCell(&cont, PQgetvalue(res, i, attcoll_col), false, false); printTableAddCell(&cont, - strcmp(PQgetvalue(res, i, attnotnull_col), "t") == 0 ? "not null" : "", + strcmp(PQgetvalue(res, i, attnotnull_col), "f") == 0 ? "" : "not null", false, false); identity = PQgetvalue(res, i, attidentity_col); @@ -3108,7 +3108,7 @@ describeOneTableDetails(const char *schemaname, { printfPQExpBuffer(&buf, "SELECT c.conname, a.attname, c.connoinherit,\n" - " c.conislocal, c.coninhcount <> 0\n" + " c.conislocal, c.coninhcount <> 0, c.convalidated \n" "FROM pg_catalog.pg_constraint c JOIN\n" " pg_catalog.pg_attribute a ON\n" " (a.attrelid = c.conrelid AND a.attnum = c.conkey[1])\n" @@ -3132,13 +3132,15 @@ describeOneTableDetails(const char *schemaname, bool islocal = PQgetvalue(result, i, 3)[0] == 't'; bool inherited = PQgetvalue(result, i, 4)[0] == 't'; - printfPQExpBuffer(&buf, " \"%s\" NOT NULL \"%s\"%s", + printfPQExpBuffer(&buf, " \"%s\" NOT NULL \"%s\"%s%s", PQgetvalue(result, i, 0), PQgetvalue(result, i, 1), PQgetvalue(result, i, 2)[0] == 't' ? " NO INHERIT" : islocal && inherited ? _(" (local, inherited)") : - inherited ? _(" (inherited)") : ""); + inherited ? _(" (inherited)") : "", + PQgetvalue(result, i, 5)[0] == 'f' ? + " NOT VALID " : ""); printTableAddFooter(&cont, buf.data); } diff --git a/src/include/catalog/pg_attribute.h b/src/include/catalog/pg_attribute.h index bc59a76..5d543af 100644 --- a/src/include/catalog/pg_attribute.h +++ b/src/include/catalog/pg_attribute.h @@ -228,6 +228,7 @@ MAKE_SYSCACHE(ATTNUM, pg_attribute_relid_attnum_index, 128); #define ATTRIBUTE_NOTNULL_TRUE 't' #define ATTRIBUTE_NOTNULL_FALSE 'f' +#define ATTRIBUTE_NOTNULL_INVALID 'i' #endif /* EXPOSE_TO_CLIENT_CODE */ diff --git a/src/test/regress/expected/constraints.out b/src/test/regress/expected/constraints.out index 692a69f..073e940 100644 --- a/src/test/regress/expected/constraints.out +++ b/src/test/regress/expected/constraints.out @@ -897,6 +897,91 @@ Not-null constraints: "foobar" NOT NULL "a" DROP TABLE notnull_tbl1; +-- verify NOT NULL VALID/NOT VALID +CREATE TABLE notnull_tbl1 (a INTEGER, b INTEGER); +INSERT INTO notnull_tbl1 VALUES (NULL, 1); +INSERT INTO notnull_tbl1 VALUES (NULL, 2); +INSERT INTO notnull_tbl1 VALUES (300, 3); +-- Below statement should throw an error +ALTER TABLE notnull_tbl1 ADD CONSTRAINT nn NOT NULL a; +ERROR: column "a" of relation "notnull_tbl1" contains null values +ALTER TABLE notnull_tbl1 ADD CONSTRAINT nn NOT NULL a NOT VALID; +\d+ notnull_tbl1 + Table "public.notnull_tbl1" + Column | Type | Collation | Nullable | Default | Storage | Stats target | Description +--------+---------+-----------+----------+---------+---------+--------------+------------- + a | integer | | not null | | plain | | + b | integer | | | | plain | | +Not-null constraints: + "nn" NOT NULL "a" NOT VALID + +-- Try to insert new record with NULL, should throw an error +INSERT INTO notnull_tbl1 VALUES (NULL, 4); +ERROR: null value in column "a" of relation "notnull_tbl1" violates not-null constraint +DETAIL: Failing row contains (null, 4). +-- SELECT NULL values for COLUMN a, should return 2 records. +SELECT * FROM notnull_tbl1 WHERE a is NULL; + a | b +---+--- + | 1 + | 2 +(2 rows) + +-- UPDATE the one of the NULL values +UPDATE notnull_tbl1 SET a = 100 WHERE b = 1; +-- DELETE the record (NULL, 2) +DELETE FROM notnull_tbl1 WHERE b = 2; +SELECT * FROM notnull_tbl1; + a | b +-----+--- + 300 | 3 + 100 | 1 +(2 rows) + +-- Try to add primary key on table column marked as NOT VALID NOT NULL +-- constraint. This should throw an error. +ALTER TABLE notnull_tbl1 add primary key (a); +ERROR: column "a" of table "notnull_tbl1" is marked as NOT VALID NOT NULL constrint +-- INHERITS table having NOT VALID NOT NULL constraints. +CREATE TABLE notnull_tbl1_child(a INTEGER, b INTEGER) INHERITS(notnull_tbl1); +NOTICE: merging column "a" with inherited definition +NOTICE: merging column "b" with inherited definition +-- Child table NOT NULL constraints should be valid. +SELECT conname, convalidated FROM pg_catalog.pg_constraint WHERE conrelid = 'notnull_tbl1_child'::regclass; + conname | convalidated +---------+-------------- + nn | t +(1 row) + +DROP TABLE notnull_tbl1_child; +ALTER TABLE notnull_tbl1 validate constraint nn; +\d+ notnull_tbl1 + Table "public.notnull_tbl1" + Column | Type | Collation | Nullable | Default | Storage | Stats target | Description +--------+---------+-----------+----------+---------+---------+--------------+------------- + a | integer | | not null | | plain | | + b | integer | | | | plain | | +Not-null constraints: + "nn" NOT NULL "a" + +DROP TABLE notnull_tbl1; +-- Verify NOT NULL VALID/NOT VALID with partition table. +CREATE TABLE notnull_tbl1 (a INTEGER, b INTEGER) PARTITION BY RANGE (a); +ALTER TABLE notnull_tbl1 ADD CONSTRAINT notnull_con NOT NULL a NOT VALID; +CREATE TABLE notnull_tbl1_1 PARTITION OF notnull_tbl1 FOR VALUES FROM (0) TO (10); +CREATE TABLE notnull_tbl1_2 PARTITION OF notnull_tbl1 FOR VALUES FROM (20) TO (30); +-- Parent table NOT NULL constraints will be market as validated false, where +-- for child table it will be true +SELECT conrelid::regclass, conname, convalidated FROM pg_catalog.pg_constraint WHERE +conrelid IN ('notnull_tbl1'::regclass, 'notnull_tbl1_1'::regclass, 'notnull_tbl1_2'::regclass); + conrelid | conname | convalidated +----------------+-------------+-------------- + notnull_tbl1 | notnull_con | f + notnull_tbl1_1 | notnull_con | t + notnull_tbl1_2 | notnull_con | t +(3 rows) + +DROP TABLE notnull_tbl1, notnull_tbl1_1, notnull_tbl1_2; -- Verify that constraint names and NO INHERIT are properly considered when -- multiple constraint are specified, either explicitly or via SERIAL/PK/etc, -- and that conflicting cases are rejected. Mind that table constraints diff --git a/src/test/regress/sql/constraints.sql b/src/test/regress/sql/constraints.sql index d6742f8..85cc13a 100644 --- a/src/test/regress/sql/constraints.sql +++ b/src/test/regress/sql/constraints.sql @@ -641,6 +641,46 @@ ALTER TABLE notnull_tbl1 ADD CONSTRAINT foobar NOT NULL a; \d+ notnull_tbl1 DROP TABLE notnull_tbl1; +-- verify NOT NULL VALID/NOT VALID +CREATE TABLE notnull_tbl1 (a INTEGER, b INTEGER); +INSERT INTO notnull_tbl1 VALUES (NULL, 1); +INSERT INTO notnull_tbl1 VALUES (NULL, 2); +INSERT INTO notnull_tbl1 VALUES (300, 3); +-- Below statement should throw an error +ALTER TABLE notnull_tbl1 ADD CONSTRAINT nn NOT NULL a; +ALTER TABLE notnull_tbl1 ADD CONSTRAINT nn NOT NULL a NOT VALID; +\d+ notnull_tbl1 +-- Try to insert new record with NULL, should throw an error +INSERT INTO notnull_tbl1 VALUES (NULL, 4); +-- SELECT NULL values for COLUMN a, should return 2 records. +SELECT * FROM notnull_tbl1 WHERE a is NULL; +-- UPDATE the one of the NULL values +UPDATE notnull_tbl1 SET a = 100 WHERE b = 1; +-- DELETE the record (NULL, 2) +DELETE FROM notnull_tbl1 WHERE b = 2; +SELECT * FROM notnull_tbl1; +-- Try to add primary key on table column marked as NOT VALID NOT NULL +-- constraint. This should throw an error. +ALTER TABLE notnull_tbl1 add primary key (a); +-- INHERITS table having NOT VALID NOT NULL constraints. +CREATE TABLE notnull_tbl1_child(a INTEGER, b INTEGER) INHERITS(notnull_tbl1); +-- Child table NOT NULL constraints should be valid. +SELECT conname, convalidated FROM pg_catalog.pg_constraint WHERE conrelid = 'notnull_tbl1_child'::regclass; +DROP TABLE notnull_tbl1_child; +ALTER TABLE notnull_tbl1 validate constraint nn; +\d+ notnull_tbl1 +DROP TABLE notnull_tbl1; +-- Verify NOT NULL VALID/NOT VALID with partition table. +CREATE TABLE notnull_tbl1 (a INTEGER, b INTEGER) PARTITION BY RANGE (a); +ALTER TABLE notnull_tbl1 ADD CONSTRAINT notnull_con NOT NULL a NOT VALID; +CREATE TABLE notnull_tbl1_1 PARTITION OF notnull_tbl1 FOR VALUES FROM (0) TO (10); +CREATE TABLE notnull_tbl1_2 PARTITION OF notnull_tbl1 FOR VALUES FROM (20) TO (30); +-- Parent table NOT NULL constraints will be market as validated false, where +-- for child table it will be true +SELECT conrelid::regclass, conname, convalidated FROM pg_catalog.pg_constraint WHERE +conrelid IN ('notnull_tbl1'::regclass, 'notnull_tbl1_1'::regclass, 'notnull_tbl1_2'::regclass); +DROP TABLE notnull_tbl1, notnull_tbl1_1, notnull_tbl1_2; + -- Verify that constraint names and NO INHERIT are properly considered when -- multiple constraint are specified, either explicitly or via SERIAL/PK/etc, -- and that conflicting cases are rejected. Mind that table constraints -- 1.8.3.1