alter_table_set_not_null_by_constraints_only_v10_fixes.patch

diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 84d72fdc75..926b3361ea 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -218,11 +218,13 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
      </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.
+      <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 f8427865ee..b03e1bc875 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -4507,8 +4507,9 @@ 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->verify_new_notnull ||
 				tab->partition_constraint != NULL)
@@ -4674,10 +4675,10 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, LOCKMODE lockmode)
 	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++)
 		{
@@ -5683,9 +5684,7 @@ 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->verify_new_notnull |= colDef->is_not_null;
 		}
@@ -6056,14 +6055,16 @@ ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
 		CatalogTupleUpdate(attr_rel, &tuple->t_self, tuple);
 
 		/*
-		 * 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
+		 * 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 (!NotNullImpliedByRelConstraints(rel, (Form_pg_attribute) GETSTRUCT(tuple)))
+		if (!tab->verify_new_notnull &&
+			!NotNullImpliedByRelConstraints(rel, (Form_pg_attribute) GETSTRUCT(tuple)))
 		{
-			/* Tell Phase 3 it needs to test the constraints */
+			/* Tell Phase 3 it needs to test the constraint */
 			tab->verify_new_notnull = true;
 		}
 
@@ -6083,12 +6084,11 @@ ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
 
 /*
  * NotNullImpliedByRelConstraints
- *		Does rel's existing constraints imply NOT NULL for given attribute
+ *		Does rel's existing constraints imply NOT NULL for the given attribute
  */
 static bool
 NotNullImpliedByRelConstraints(Relation rel, Form_pg_attribute attr)
 {
-	List	   *notNullConstraint = NIL;
 	NullTest   *nnulltest = makeNode(NullTest);
 
 	nnulltest->arg = (Expr *) makeVar(1,
@@ -6100,16 +6100,14 @@ NotNullImpliedByRelConstraints(Relation rel, Form_pg_attribute attr)
 	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.
+	 * 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))
+	if (ConstraintImpliedByRelConstraint(rel, list_make1(nnulltest)))
 	{
 		ereport(DEBUG1,
 				(errmsg("verifying table \"%s\" NOT NULL constraint "
@@ -14368,11 +14366,12 @@ ComputePartitionAttrs(ParseState *pstate, Relation rel, List *partParams, AttrNu
 
 /*
  * ConstraintImpliedByRelConstraint
- *		Do scanrel's existing constraints imply given constraint?
+ *		Do scanrel's existing constraints imply the given constraint?
  *
  * "Existing constraints" include its check constraints and column-level
- * NOT NULL constraints.  testConstraint describes the checked 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
 ConstraintImpliedByRelConstraint(Relation scanrel, List *testConstraint)
@@ -14445,10 +14444,6 @@ ConstraintImpliedByRelConstraint(Relation scanrel, List *testConstraint)
 	 * 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 testConstraint.
-	 *
-	 * Note that predicate_implied_by assumes its first argument is known
-	 * immutable.  That should always be true for both not null and
-	 * partition constraints, so we don't test it here.
 	 */
 	return predicate_implied_by(testConstraint, existConstraint, true);
 }