Hi
I noticed new merge conflict, updated version attached.
regards, Sergei
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index bd22627..db98a98 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 0f5932f..6fd3663 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -1153,7 +1153,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",
@@ -1197,8 +1197,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 f810885..e10edb1 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,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,
@@ -4461,7 +4462,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);
@@ -4622,7 +4623,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
@@ -5657,7 +5658,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
* an OID column. OID will be marked not 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;
}
}
@@ -6061,8 +6062,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);
@@ -6079,6 +6089,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.
@@ -13850,16 +13900,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;
@@ -13928,13 +13977,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);
}
/*
@@ -13956,7 +14005,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/include/commands/tablecmds.h b/src/include/commands/tablecmds.h
index 70ee3da..1f03b92 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 6384591..76ced3b 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 4929a36..9f90508 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);
