Hello, David!
> I've made a pass over v10. I think it's in pretty good shape, but I
> did end up changing a few small things.
Thank you! I merged your changes to new patch version.
> The only thing that I'm a bit unsure of is the tests. I've read the
> thread and I see the discussion above about it. I'd personally have
> thought INFO was fine since ATTACH PARTITION does that, but I see
> objections.
It is unclear for me if we have consensus about INFO ereport in ATTACH
PARTITION.
> It appears that all the tests just assume that the CHECK
> constraint was used to validate the SET NOT NULL. I'm not all that
> sure if there's any good reason not to set client_min_messages =
> 'debug1' just before your test then RESET client_min_messages;
> afterwards. No other tests seem to do it, but my only thoughts on the
> reason not to would be that it might fail if someone added another
> debug somewhere, but so what? they should update the expected results
> too.
Not sure we have consensus about debug messages in tests, so I did such test
changes in additional patch.
> separate that part out and
> put back the function named PartConstraintImpliedByRelConstraint and
> have it do the IS NOT NULL building before calling
> ConstraintImpliedByRelConstraint(). No duplicate code that way and you
> also don't need to touch partbound.c
In this case we need extra argument for ConstraintImpliedByRelConstraint for
some caller-provided existConstraint, right? Along with Relation itself? Then I
need make copy of existConstraint, append relation constraints and call
predicate_implied_by. If I understood correctly pseudocode would be:
PartConstraintImpliedByRelConstraint(rel, testConstraint)
generate notNullConstraint from NOT NULL table attributes
call ConstraintImpliedByRelConstraint(rel, testConstraint, notNullConstraint)
ConstraintImpliedByRelConstraint(rel, testConstraint, existsConstraint)
copy existsConstraint to localExistsConstraint variable
append relation constraints to localExistsConstraint list
call predicate_implied_by
I thing redundant IS NOT NULL check is not issue here and we not need extra
arguments for ConstraintImpliedByRelConstraint. Was not changed in attached
version, but I will change if you think this would be better design.
regards, Sergei
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 890b23afd6..926b3361ea 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 a93b13c2fe..b03e1bc875 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -158,7 +158,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 */
@@ -370,6 +370,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,
@@ -4506,10 +4507,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);
@@ -4670,13 +4672,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++)
{
@@ -5682,11 +5684,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;
}
}
@@ -6054,8 +6054,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);
@@ -6071,6 +6082,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)))
+ {
+ 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
*
@@ -14317,16 +14365,16 @@ ComputePartitionAttrs(ParseState *pstate, Relation rel, List *partParams, AttrNu
}
/*
- * PartConstraintImpliedByRelConstraint
- * Do scanrel's existing constraints imply the partition constraint?
+ * ConstraintImpliedByRelConstraint
+ * Do scanrel's existing constraints imply the given constraint?
*
* "Existing constraints" include its check constraints and column-level
- * NOT NULL constraints. partConstraint describes the partition constraint,
- * in implicit-AND form.
+ * NOT NULL constraints. testConstraint describes the constraint to validate.
+ * This must be in implicitly-AND form, must only contain immutable clauses
+ * and all Vars must be varno=1.
*/
bool
-PartConstraintImpliedByRelConstraint(Relation scanrel,
- List *partConstraint)
+ConstraintImpliedByRelConstraint(Relation scanrel, List *testConstraint)
{
List *existConstraint = NIL;
TupleConstr *constr = RelationGetDescr(scanrel)->constr;
@@ -14395,13 +14443,9 @@ 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.
- *
- * 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.
+ * not-false and try to prove the same for testConstraint.
*/
- return predicate_implied_by(partConstraint, existConstraint, true);
+ return predicate_implied_by(testConstraint, existConstraint, true);
}
/*
@@ -14423,7 +14467,7 @@ QueuePartitionConstraintValidation(List **wqueue, Relation scanrel,
* Based on the table's existing constraints, determine whether or not we
* may skip scanning the table.
*/
- if (PartConstraintImpliedByRelConstraint(scanrel, partConstraint))
+ if (ConstraintImpliedByRelConstraint(scanrel, partConstraint))
{
if (!validate_default)
ereport(INFO,
diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index e71eb3793b..a6bf41ac11 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -1177,7 +1177,7 @@ check_default_partition_contents(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",
@@ -1221,7 +1221,7 @@ check_default_partition_contents(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,
+ if (ConstraintImpliedByRelConstraint(part_rel,
def_part_constraints))
{
ereport(INFO,
diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h
index ec3bb90b01..759abba3f2 100644
--- a/src/include/commands/tablecmds.h
+++ b/src/include/commands/tablecmds.h
@@ -94,7 +94,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 4db792cf2f..c8de23cb19 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 d80643037d..94c6a43a10 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 c8de23cb19..efa8126962 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: verifying table "atacc1" NOT NULL constraint on test_a attribute by existed constraints
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: verifying table "atacc1" NOT NULL constraint on test_a attribute by existed constraints
+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: verifying table "atacc1" NOT NULL constraint on test_b attribute by existed constraints
+DEBUG: verifying table "atacc1" NOT NULL constraint on test_a attribute by existed constraints
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 94c6a43a10..aa678ff6b8 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);
