From bbb96c3704c041d139181c6601e5bc770e045d26 Mon Sep 17 00:00:00 2001 From: Robert Haas Date: Wed, 13 Mar 2019 08:55:00 -0400 Subject: [PATCH] Allow ALTER TABLE .. SET NOT NULL to skip provably unnecessary scans. If existing CHECK or NOT NULL constraints preclude the presence of nulls, we need not look to see whether any are present. Sergei Kornilov, reviewed by Stephen Frost, Ildar Musin, David Rowley, and by me. Discussion: http://postgr.es/m/81911511895540@web58j.yandex.ru --- doc/src/sgml/ref/alter_table.sgml | 13 ++- src/backend/commands/tablecmds.c | 111 ++++++++++++++++++---- src/test/regress/expected/alter_table.out | 54 +++++++++++ src/test/regress/sql/alter_table.sql | 40 ++++++++ 4 files changed, 195 insertions(+), 23 deletions(-) diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index 890b23afd6..e360728c02 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -214,8 +214,17 @@ 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. + + + + SET NOT NULL may only be applied to a column + providing none of the records in the table contain a + NULL value for the column. Ordinarily this is + checked during the ALTER TABLE by scanning the + entire table; however, if a valid CHECK constraint is + found which proves no NULL can exist, then the + table scan is skipped. diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 5ed560b02f..754b59581b 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -160,7 +160,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 */ @@ -372,6 +372,9 @@ 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 bool ConstraintImpliedByRelConstraint(Relation scanrel, + List *partConstraint, List *existedConstraints); static ObjectAddress ATExecColumnDefault(Relation rel, const char *colName, Node *newDefault, LOCKMODE lockmode); static ObjectAddress ATExecAddIdentity(Relation rel, const char *colName, @@ -4550,10 +4553,11 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode) else { /* - * Test the current data within the table against new constraints - * generated by ALTER TABLE commands, but don't rebuild data. + * If required, 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); @@ -4714,13 +4718,13 @@ 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 - * constraints, check all not-null constraints. This is a bit of - * overkill but it minimizes risk of bugs, and heap_attisnull is a - * pretty cheap test anyway. + * If we are rebuilding the tuples OR if we added any new but not + * verified NOT NULL constraints, check all not-null constraints. + * This is a bit of overkill but it minimizes risk of bugs, and + * heap_attisnull is a pretty cheap test anyway. */ for (i = 0; i < newTupDesc->natts; i++) { @@ -5749,11 +5753,9 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel, { /* * If the new column is NOT NULL, and there is no missing value, - * tell Phase 3 it needs to test that. (Note we don't do this for - * an OID column. OID will be marked not null, but since it's - * filled specially, there's no need to test anything.) + * tell Phase 3 it needs to check for NULLs. */ - tab->new_notnull |= colDef->is_not_null; + tab->verify_new_notnull |= colDef->is_not_null; } } @@ -6121,8 +6123,19 @@ 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; + /* + * Ordinarily phase 3 must ensure that no NULLs exist in columns that + * are set NOT NULL; however, if we can find a constraint which proves + * this then we can skip that. We needn't bother looking if + * we've already found that we must verify some other NOT NULL + * constraint. + */ + if (!tab->verify_new_notnull && + !NotNullImpliedByRelConstraints(rel, (Form_pg_attribute) GETSTRUCT(tuple))) + { + /* Tell Phase 3 it needs to test the constraint */ + tab->verify_new_notnull = true; + } ObjectAddressSubSet(address, RelationRelationId, RelationGetRelid(rel), attnum); @@ -6138,6 +6151,42 @@ ATExecSetNotNull(AlteredTableInfo *tab, Relation rel, return address; } +/* + * NotNullImpliedByRelConstraints + * Does rel's existing constraints imply NOT NULL for the given attribute? + */ +static bool +NotNullImpliedByRelConstraints(Relation rel, Form_pg_attribute attr) +{ + NullTest *nnulltest = makeNode(NullTest); + + nnulltest->arg = (Expr *) makeVar(1, + attr->attnum, + attr->atttypid, + attr->atttypmod, + attr->attcollation, + 0); + nnulltest->nulltesttype = IS_NOT_NULL; + + /* + * 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; + + if (ConstraintImpliedByRelConstraint(rel, list_make1(nnulltest), NIL)) + { + ereport(DEBUG1, + (errmsg("existing constraints on column \"%s\".\"%s\" are sufficient to prove that it does not contain nulls", + RelationGetRelationName(rel), NameStr(attr->attname)))); + return true; + } + + return false; +} + /* * ALTER TABLE ALTER COLUMN SET/DROP DEFAULT * @@ -14416,8 +14465,7 @@ PartConstraintImpliedByRelConstraint(Relation scanrel, { List *existConstraint = NIL; TupleConstr *constr = RelationGetDescr(scanrel)->constr; - int num_check, - i; + int i; if (constr && constr->has_not_null) { @@ -14451,6 +14499,27 @@ PartConstraintImpliedByRelConstraint(Relation scanrel, } } + return ConstraintImpliedByRelConstraint(scanrel, partConstraint, existConstraint); +} + +/* + * ConstraintImpliedByRelConstraint + * Do scanrel's existing constraints imply the given constraint? + * + * testConstraint is the constraint to validate. provenConstraint is a + * caller-provided list of conditions which this function may assume + * to be true. Both provenConstraint and testConstraint must be in + * implicit-AND form, must only contain immutable clauses, and must + * contain only Vars with varno = 1. + */ +bool +ConstraintImpliedByRelConstraint(Relation scanrel, List *testConstraint, List *provenConstraint) +{ + List *existConstraint = list_copy(provenConstraint); + TupleConstr *constr = RelationGetDescr(scanrel)->constr; + int num_check, + i; + num_check = (constr != NULL) ? constr->num_check : 0; for (i = 0; i < num_check; i++) { @@ -14481,13 +14550,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); } /* diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index fd14c4b4f2..d0dc671c47 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -1028,6 +1028,8 @@ insert into atacc1 (test2, test) values (1, NULL); ERROR: null value in column "test" violates not-null constraint DETAIL: Failing row contains (null, 1). drop table atacc1; +-- we want check if not null was implied by constraint +set client_min_messages to 'debug1'; -- alter table / alter column [set/drop] not null tests -- try altering system catalogs, should fail alter table pg_class alter column relname drop not null; @@ -1043,15 +1045,19 @@ ERROR: relation "non_existent" does not exist -- test checking for null values and primary key create table atacc1 (test int not null); alter table atacc1 add constraint "atacc1_pkey" primary key (test); +DEBUG: ALTER TABLE / ADD PRIMARY KEY will create implicit index "atacc1_pkey" for table "atacc1" +DEBUG: building index "atacc1_pkey" on table "atacc1" serially alter table atacc1 alter column test drop not null; ERROR: column "test" is in a primary key alter table atacc1 drop constraint "atacc1_pkey"; alter table atacc1 alter column test drop not null; insert into atacc1 values (null); alter table atacc1 alter test set not null; +DEBUG: verifying table "atacc1" ERROR: column "test" contains null values delete from atacc1; alter table atacc1 alter test set not null; +DEBUG: verifying table "atacc1" -- try altering a non-existent column, should fail alter table atacc1 alter bar set not null; ERROR: column "bar" of relation "atacc1" does not exist @@ -1065,6 +1071,54 @@ 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); +DEBUG: verifying table "atacc1" +alter table atacc1 alter test_a set not null; +DEBUG: verifying table "atacc1" +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; +DEBUG: verifying table "atacc1" +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); +DEBUG: verifying table "atacc1" +alter table atacc1 alter test_a set not null; +DEBUG: existing constraints on column "atacc1"."test_a" are sufficient to prove that it does not contain nulls +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; +DEBUG: existing constraints on column "atacc1"."test_a" are sufficient to prove that it does not contain nulls +DEBUG: verifying table "atacc1" +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; +DEBUG: verifying table "atacc1" +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; +DEBUG: verifying table "atacc1" +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); +DEBUG: verifying table "atacc1" +alter table atacc1 alter test_b set not null, alter test_a set not null; +DEBUG: existing constraints on column "atacc1"."test_b" are sufficient to prove that it does not contain nulls +DEBUG: existing constraints on column "atacc1"."test_a" are sufficient to prove that it does not contain nulls +drop table atacc1; +reset client_min_messages; -- 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 298bde179b..b8714e27d3 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -776,6 +776,9 @@ insert into atacc1 (test2, test) values (2, 3); insert into atacc1 (test2, test) values (1, NULL); drop table atacc1; +-- we want check if not null was implied by constraint +set client_min_messages to 'debug1'; + -- alter table / alter column [set/drop] not null tests -- try altering system catalogs, should fail alter table pg_class alter column relname drop not null; @@ -809,6 +812,43 @@ 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; + +reset client_min_messages; + -- test inheritance create table parent (a int); create table child (b varchar(255)) inherits (parent); -- 2.40.0