Hello
> Dispatches from the department of grammatical nitpicking...
Thank you!
> + entire table, however if a valid <literal>CHECK</literal> constraint is
>
> I think this should be:
>
> entire table; however, if...
>
> + * are set NOT NULL, however, if we can find a constraint which proves
>
> similarly here
Changed
> + ereport(DEBUG1,
> + (errmsg("verifying table \"%s\" NOT NULL constraint "
> + "on %s attribute by existed constraints",
> + RelationGetRelationName(rel), NameStr(attr->attname))));
>
> Ugh, that doesn't read well at all. How about:
>
> existing constraints on column "%s"."%s" are sufficient to prove that
> it does not contain nulls
Changed
> - * in implicit-AND form.
> + * in implicitly-AND form, must only contain immutable clauses
> + * and all Vars must be varno=1.
>
> I think you should leave the existing sentence alone (implicit-AND is
> correct, implicitly-AND is not) and add a new sentence that says the
> stuff you want to add.
Ok
> + * "Existing constraints" include its check constraints and optional
> + * caller-provided existConstraint list. existConstraint list is modified
> + * during ConstraintImpliedByRelConstraint call and would represent all
> + * assumed conditions. testConstraint describes the constraint to validate.
> + * Both existConstraint and testConstraint must be in implicitly-AND form,
> + * must only contain immutable clauses and all Vars must be varno=1.
>
> I think that it might be better to copy the list rather than to have
> the comment note that it gets mutated, but regardless the grammar
> needs improvement here.
Agreed, in attached new version I copy the list and do not modify parameter.
> I object to removing this.
Okay, I revert this David's change
regards, Sergeidiff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 890b23afd6..cda0ea8972 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -214,8 +214,17 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
<listitem>
<para>
These forms change whether a column is marked to allow null
- values or to reject null values. You can only use <literal>SET
- NOT NULL</literal> when the column contains no null values.
+ values or to reject null values.
+ </para>
+
+ <para>
+ <literal>SET NOT NULL</literal> may only be applied to a column
+ providing none of the records in the table contain a
+ <literal>NULL</literal> value for the column. Ordinarily this is
+ checked during the <literal>ALTER TABLE</literal> by scanning the
+ entire table; however if a valid <literal>CHECK</literal> constraint is
+ found which proves no <literal>NULL</literal> can exist, then the
+ table scan is skipped.
</para>
<para>
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 5ed560b02f..15407778b4 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. However, 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,43 @@ 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 +14466,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 +14500,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 +14551,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..a76bc0dd6a 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -1065,6 +1065,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 298bde179b..5bda7febde 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -809,6 +809,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);
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index a76bc0dd6a..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
@@ -1070,36 +1076,49 @@ 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 5bda7febde..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;
@@ -844,6 +847,8 @@ 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);
