diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index afe2139..783a0ef 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -215,8 +215,15 @@ WITH ( MODULUS numeric_literal, REM These forms change whether a column is marked to allow null - values or to reject null values. You can only use SET - NOT NULL when the column contains no null values. + values or to reject null values. + + + + You can only use SET NOT NULL when the column + contains no null values. Full table scan is performed to check + this condition unless there are valid CHECK + constraints that prohibit NULL values for specified column. + In the latter case table scan is skipped. diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c index fcf7655..4fec36d 100644 --- a/src/backend/catalog/partition.c +++ b/src/backend/catalog/partition.c @@ -1258,7 +1258,7 @@ check_default_allows_bound(Relation parent, Relation default_rel, * not contain any row that would belong to the new partition, we can * avoid scanning the default partition. */ - if (PartConstraintImpliedByRelConstraint(default_rel, def_part_constraints)) + if (ConstraintImpliedByRelConstraint(default_rel, def_part_constraints)) { ereport(INFO, (errmsg("updated partition constraint for default partition \"%s\" is implied by existing constraints", @@ -1302,8 +1302,8 @@ check_default_allows_bound(Relation parent, Relation default_rel, * that it will not contain any row that would belong to the new * partition, we can avoid scanning the child table. */ - if (PartConstraintImpliedByRelConstraint(part_rel, - def_part_constraints)) + if (ConstraintImpliedByRelConstraint(part_rel, + def_part_constraints)) { ereport(INFO, (errmsg("updated partition constraint for default partition \"%s\" is implied by existing constraints", diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index e559afb..b6d395b 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -162,7 +162,7 @@ typedef struct AlteredTableInfo /* Information saved by Phases 1/2 for Phase 3: */ List *constraints; /* List of NewConstraint */ List *newvals; /* List of NewColumnValue */ - bool new_notnull; /* T if we added new NOT NULL constraints */ + bool verify_new_notnull; /* T if we should recheck NOT NULL */ int rewrite; /* Reason for forced rewrite, if any */ Oid newTableSpace; /* new tablespace; 0 means no change */ bool chgPersistence; /* T if SET LOGGED/UNLOGGED is used */ @@ -377,6 +377,7 @@ static ObjectAddress ATExecDropNotNull(Relation rel, const char *colName, LOCKMO static void ATPrepSetNotNull(Relation rel, bool recurse, bool recursing); static ObjectAddress ATExecSetNotNull(AlteredTableInfo *tab, Relation rel, const char *colName, LOCKMODE lockmode); +static bool NotNullImpliedByRelConstraints(Relation rel, Form_pg_attribute attr); static ObjectAddress ATExecColumnDefault(Relation rel, const char *colName, Node *newDefault, LOCKMODE lockmode); static ObjectAddress ATExecAddIdentity(Relation rel, const char *colName, @@ -4371,7 +4372,7 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode) * Test the current data within the table against new constraints * generated by ALTER TABLE commands, but don't rebuild data. */ - if (tab->constraints != NIL || tab->new_notnull || + if (tab->constraints != NIL || tab->verify_new_notnull || tab->partition_constraint != NULL) ATRewriteTable(tab, InvalidOid, lockmode); @@ -4532,7 +4533,7 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, LOCKMODE lockmode) } notnull_attrs = NIL; - if (newrel || tab->new_notnull) + if (newrel || tab->verify_new_notnull) { /* * If we are rebuilding the tuples OR if we added any new NOT NULL @@ -5545,7 +5546,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel, * null, but since it's filled specially, there's no need to test * anything.) */ - tab->new_notnull |= colDef->is_not_null; + tab->verify_new_notnull |= colDef->is_not_null; } /* @@ -5948,8 +5949,17 @@ ATExecSetNotNull(AlteredTableInfo *tab, Relation rel, CatalogTupleUpdate(attr_rel, &tuple->t_self, tuple); - /* Tell Phase 3 it needs to test the constraint */ - tab->new_notnull = true; + /* + * Verifying table data can be done in Phase 3 with single table scan + * for all constraint (both existing and new) + * We can skip this check only if every new NOT NULL contraint + * implied by existed table constraints + */ + if (!NotNullImpliedByRelConstraints(rel, (Form_pg_attribute) GETSTRUCT(tuple))) + { + /* Tell Phase 3 it needs to test the constraints */ + tab->verify_new_notnull = true; + } ObjectAddressSubSet(address, RelationRelationId, RelationGetRelid(rel), attnum); @@ -5966,6 +5976,46 @@ ATExecSetNotNull(AlteredTableInfo *tab, Relation rel, } /* + * NotNullImpliedByRelConstraints + * Does rel's existing constraints imply NOT NULL for given attribute + */ +static bool +NotNullImpliedByRelConstraints(Relation rel, Form_pg_attribute attr) +{ + List *notNullConstraint = NIL; + NullTest *nnulltest = makeNode(NullTest); + + nnulltest->arg = (Expr *) makeVar(1, + attr->attnum, + attr->atttypid, + attr->atttypmod, + attr->attcollation, + 0); + nnulltest->nulltesttype = IS_NOT_NULL; + + /* + * same thing as in ConstraintImpliedByRelConstraint argisrow=false is + * correct even for a composite column, because attnotnull does not + * represent a SQL-spec IS NOT NULL test in such a case, just IS DISTINCT + * FROM NULL. + */ + nnulltest->argisrow = false; + nnulltest->location = -1; + notNullConstraint = lappend(notNullConstraint, nnulltest); + + if (ConstraintImpliedByRelConstraint(rel, notNullConstraint)) + { + ereport(DEBUG1, + (errmsg("verifying table \"%s\" NOT NULL constraint " + "on %s attribute by existed constraints", + RelationGetRelationName(rel), NameStr(attr->attname)))); + return true; + } + + return false; +} + +/* * ALTER TABLE ALTER COLUMN SET/DROP DEFAULT * * Return the address of the affected column. @@ -13650,16 +13700,15 @@ ComputePartitionAttrs(Relation rel, List *partParams, AttrNumber *partattrs, } /* - * PartConstraintImpliedByRelConstraint - * Do scanrel's existing constraints imply the partition constraint? + * ConstraintImpliedByRelConstraint + * Do scanrel's existing constraints imply given constraint? * * "Existing constraints" include its check constraints and column-level - * NOT NULL constraints. partConstraint describes the partition constraint, + * NOT NULL constraints. testConstraint describes the checked constraint, * in implicit-AND form. */ bool -PartConstraintImpliedByRelConstraint(Relation scanrel, - List *partConstraint) +ConstraintImpliedByRelConstraint(Relation scanrel, List *testConstraint) { List *existConstraint = NIL; TupleConstr *constr = RelationGetDescr(scanrel)->constr; @@ -13728,13 +13777,13 @@ PartConstraintImpliedByRelConstraint(Relation scanrel, /* * Try to make the proof. Since we are comparing CHECK constraints, we * need to use weak implication, i.e., we assume existConstraint is - * not-false and try to prove the same for partConstraint. + * not-false and try to prove the same for testConstraint. * * Note that predicate_implied_by assumes its first argument is known - * immutable. That should always be true for partition constraints, so we - * don't test it here. + * immutable. That should always be true for both not null and + * partition constraints, so we don't test it here. */ - return predicate_implied_by(partConstraint, existConstraint, true); + return predicate_implied_by(testConstraint, existConstraint, true); } /* @@ -13762,7 +13811,7 @@ ValidatePartitionConstraints(List **wqueue, Relation scanrel, * Based on the table's existing constraints, determine if we can skip * scanning the table to validate the partition constraint. */ - if (PartConstraintImpliedByRelConstraint(scanrel, partConstraint)) + if (ConstraintImpliedByRelConstraint(scanrel, partConstraint)) { if (!validate_default) ereport(INFO, @@ -13814,7 +13863,7 @@ ValidatePartitionConstraints(List **wqueue, Relation scanrel, elog(ERROR, "unexpected whole-row reference found in partition key"); /* Can we skip scanning this part_rel? */ - if (PartConstraintImpliedByRelConstraint(part_rel, my_partconstr)) + if (ConstraintImpliedByRelConstraint(part_rel, my_partconstr)) { if (!validate_default) ereport(INFO, diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h index 06e5180..8e3d0c2 100644 --- a/src/include/commands/tablecmds.h +++ b/src/include/commands/tablecmds.h @@ -88,7 +88,7 @@ extern void RangeVarCallbackOwnsTable(const RangeVar *relation, extern void RangeVarCallbackOwnsRelation(const RangeVar *relation, Oid relId, Oid oldRelId, void *noCatalogs); -extern bool PartConstraintImpliedByRelConstraint(Relation scanrel, - List *partConstraint); +extern bool ConstraintImpliedByRelConstraint(Relation scanrel, + List *testConstraint); #endif /* TABLECMDS_H */ diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index ccd2c38..217c22d 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -1038,6 +1038,41 @@ alter table myview alter column test set not null; ERROR: "myview" is not a table or foreign table drop view myview; drop table atacc1; +-- set not null verified by constraints +create table atacc1 (test_a int, test_b int); +insert into atacc1 values (null, 1); +-- constraint not cover all values, should fail +alter table atacc1 add constraint atacc1_constr_or check(test_a is not null or test_b < 10); +alter table atacc1 alter test_a set not null; +ERROR: column "test_a" contains null values +alter table atacc1 drop constraint atacc1_constr_or; +-- not valid constraint, should fail +alter table atacc1 add constraint atacc1_constr_invalid check(test_a is not null) not valid; +alter table atacc1 alter test_a set not null; +ERROR: column "test_a" contains null values +alter table atacc1 drop constraint atacc1_constr_invalid; +-- with valid constraint +update atacc1 set test_a = 1; +alter table atacc1 add constraint atacc1_constr_a_valid check(test_a is not null); +alter table atacc1 alter test_a set not null; +delete from atacc1; +insert into atacc1 values (2, null); +alter table atacc1 alter test_a drop not null; +-- test multiple set not null at same time +-- test_a checked by atacc1_constr_a_valid, test_b should fail by table scan +alter table atacc1 alter test_a set not null, alter test_b set not null; +ERROR: column "test_b" contains null values +-- commands order has no importance +alter table atacc1 alter test_b set not null, alter test_a set not null; +ERROR: column "test_b" contains null values +-- valid one by table scan, one by check constraints +update atacc1 set test_b = 1; +alter table atacc1 alter test_b set not null, alter test_a set not null; +alter table atacc1 alter test_a drop not null, alter test_b drop not null; +-- both column has check constraints +alter table atacc1 add constraint atacc1_constr_b_valid check(test_b is not null); +alter table atacc1 alter test_b set not null, alter test_a set not null; +drop table atacc1; -- test inheritance create table parent (a int); create table child (b varchar(255)) inherits (parent); diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index b73f523..0aba0fb 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -798,6 +798,41 @@ drop view myview; drop table atacc1; +-- set not null verified by constraints +create table atacc1 (test_a int, test_b int); +insert into atacc1 values (null, 1); +-- constraint not cover all values, should fail +alter table atacc1 add constraint atacc1_constr_or check(test_a is not null or test_b < 10); +alter table atacc1 alter test_a set not null; +alter table atacc1 drop constraint atacc1_constr_or; +-- not valid constraint, should fail +alter table atacc1 add constraint atacc1_constr_invalid check(test_a is not null) not valid; +alter table atacc1 alter test_a set not null; +alter table atacc1 drop constraint atacc1_constr_invalid; +-- with valid constraint +update atacc1 set test_a = 1; +alter table atacc1 add constraint atacc1_constr_a_valid check(test_a is not null); +alter table atacc1 alter test_a set not null; +delete from atacc1; + +insert into atacc1 values (2, null); +alter table atacc1 alter test_a drop not null; +-- test multiple set not null at same time +-- test_a checked by atacc1_constr_a_valid, test_b should fail by table scan +alter table atacc1 alter test_a set not null, alter test_b set not null; +-- commands order has no importance +alter table atacc1 alter test_b set not null, alter test_a set not null; + +-- valid one by table scan, one by check constraints +update atacc1 set test_b = 1; +alter table atacc1 alter test_b set not null, alter test_a set not null; + +alter table atacc1 alter test_a drop not null, alter test_b drop not null; +-- both column has check constraints +alter table atacc1 add constraint atacc1_constr_b_valid check(test_b is not null); +alter table atacc1 alter test_b set not null, alter test_a set not null; +drop table atacc1; + -- test inheritance create table parent (a int); create table child (b varchar(255)) inherits (parent);