Hello
My patch does not apply after commit 5748f3a0aa7cf78ac6979010273bd9d50869bb8e.
Here is update to current master. Not null constraint is immutable too, so here
is no changes in PartConstraintImpliedByRelConstraint excepts rename and
comments fix.
In this patch version i also revert tests to v4 state: i use DEBUG ereport
instead INFO and code path not tested. Please tell me if i must change tests
some way.
regards, Sergei
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 <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>
+ You can only use <literal>SET NOT NULL</literal> when the column
+ contains no null values. Full table scan is performed to check
+ this condition unless there are valid <literal>CHECK</literal>
+ constraints that prohibit NULL values for specified column.
+ In the latter case table scan is skipped.
</para>
<para>
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);
