[Sergei Kornilov <sk@zsrv.org>]

Hello
I write patch to speed up ALTER TABLE SET NOT NULL by check existed check constraints or indexes. Huge phase 3 with verify table data will be skipped if table has valid check constraint cover "alteredfield IS NOT NULL" condition or by SPI query if found index with compatible condition or regular amsearchnulls index on processed field.

Patch based on current master branch, i believe it has no platform-dependent code, of course code compiled and pass tests locally.
Tell me please, what i forgot or make incorrectly.

Implementation notes:
I use existed PartConstraintImpliedByRelConstraint method to check relation constraints. But i rename original method to static ConstraintImpliedByRelConstraint (because method now used not only in partitions) and leave PartConstraintImpliedByRelConstraint as proxy to not change public API.
I found it difficult to do index scan and choose index with lower costs if found many suitable indexes. Is it acceptable to use SPI here?

Related archive discussions:
https://www.postgresql.org/message-id/flat/530C10CF.4020101%40strangersgate.com
https://www.postgresql.org/message-id/flat/CAASwCXdAK55BzuOy_FtYj2zQWg26PriDKL5pRoWiyFJe0eg-Hg%40mai...

Thanks!

Attachments:

  [text/x-diff] alter_table_set_not_null_by_index_or_check_v1.patch (7.6K, ../81911511895540@web58j.yandex.ru/2-alter_table_set_not_null_by_index_or_check_v1.patch)
  download | inline diff:
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d979ce2..424f608 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -60,6 +60,7 @@
 #include "commands/typecmds.h"
 #include "commands/user.h"
 #include "executor/executor.h"
+#include "executor/spi.h"
 #include "foreign/foreign.h"
 #include "miscadmin.h"
 #include "nodes/makefuncs.h"
@@ -370,6 +371,8 @@ 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 isSetNotNullNeedTableScan(Relation rel, Form_pg_attribute attr);
+static bool ConstraintImpliedByRelConstraint(Relation scanrel, List *partConstraint);
 static ObjectAddress ATExecColumnDefault(Relation rel, const char *colName,
 					Node *newDefault, LOCKMODE lockmode);
 static ObjectAddress ATExecAddIdentity(Relation rel, const char *colName,
@@ -5863,8 +5866,10 @@ 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;
+		if (isSetNotNullNeedTableScan(rel, (Form_pg_attribute) GETSTRUCT(tuple))) {
+			/* Tell Phase 3 it needs to test the constraint */
+			tab->new_notnull = true;
+		}
 
 		ObjectAddressSubSet(address, RelationRelationId,
 							RelationGetRelid(rel), attnum);
@@ -5880,6 +5885,148 @@ ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
 	return address;
 }
 
+static bool
+isSetNotNullNeedTableScan(Relation rel, Form_pg_attribute attr)
+{
+	List       *indexoidlist;
+	ListCell   *indexoidscan;
+	bool       isMatchedIndexFound = false;
+	StringInfoData querybuf;
+	int        indexAttr;
+	int        checkIndexAttrNum;
+
+	// same part in PartConstraintImpliedByRelConstraint
+	List       *notNullConstraint = NIL;
+	NullTest   *nnulltest = makeNode(NullTest);
+	List       *nullConstraint = NIL;
+	NullTest   *ntest = 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;
+	notNullConstraint = lappend(notNullConstraint, nnulltest);
+
+	// first check existed constraints
+	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 false;
+	}
+
+	ntest->arg = (Expr *) makeVar(1,
+				  attr->attnum,
+				  attr->atttypid,
+				  attr->atttypmod,
+				  attr->attcollation,
+				  0);
+	ntest->nulltesttype = IS_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.
+	 */
+	ntest->argisrow = false;
+	ntest->location = -1;
+	nullConstraint = lappend(nullConstraint, ntest);
+
+	indexoidlist = RelationGetIndexList(rel);
+	foreach(indexoidscan, indexoidlist)
+	{
+		Oid		     indexoid = lfirst_oid(indexoidscan);
+		Relation	indexRel;
+		Form_pg_index indexStruct;
+
+		indexRel = index_open(indexoid, AccessShareLock);
+		indexStruct = indexRel->rd_index;
+
+		if (IndexIsValid(indexStruct))
+		{
+			if (RelationGetIndexPredicate(indexRel) != NIL) {
+				if (predicate_implied_by(
+					RelationGetIndexPredicate(indexRel),
+					nullConstraint,
+					false
+				)) {
+					// regardless indexed attributes we can use this index by predicate
+					isMatchedIndexFound = true;
+				}
+			} else if (indexRel->rd_amroutine->amsearchnulls && indexStruct->indnatts > 0) {
+				checkIndexAttrNum = (indexRel->rd_amroutine->amoptionalkey ? indexStruct->indnatts : 1);
+
+				for (indexAttr = 0; indexAttr < checkIndexAttrNum; ++indexAttr) {
+					if (attr->attnum == indexStruct->indkey.values[ indexAttr ]) {
+						// index has no predicate and cover target attribute
+						isMatchedIndexFound = true;
+						break;
+					}
+				}
+			}
+		}
+
+		if (isMatchedIndexFound) {
+			ereport(DEBUG1,
+				(errmsg("verifying table \"%s\" NOT NULL constraint "
+					"on %s attribute by index",
+					RelationGetRelationName(rel),
+					NameStr(attr->attname))));
+
+			initStringInfo(&querybuf);
+			appendStringInfo(
+				&querybuf,
+				"SELECT 1 FROM %s WHERE %s IS NULL LIMIT 1",
+				quote_qualified_identifier(
+					get_namespace_name(RelationGetNamespace(rel)),
+					RelationGetRelationName(rel)
+				),
+				quote_identifier(NameStr(attr->attname))
+			);
+
+			/* Open SPI context. */
+			if (SPI_connect() != SPI_OK_CONNECT)
+				elog(ERROR, "SPI_connect failed");
+			if (SPI_execute(querybuf.data, false, 1) != SPI_OK_SELECT)
+				elog(ERROR, "SPI_exec failed: %s", querybuf.data);
+			if (SPI_processed > 0)
+			{
+				ereport(ERROR,
+					(errcode(ERRCODE_NOT_NULL_VIOLATION),
+					 errmsg("column \"%s\" contains null values",
+							NameStr(attr->attname)),
+					 errtablecol(rel, attr->attnum)));
+			}
+
+			/* Close SPI context. */
+			if (SPI_finish() != SPI_OK_FINISH)
+				elog(ERROR, "SPI_finish failed");
+		}
+
+		index_close(indexRel, AccessShareLock);
+
+		if (isMatchedIndexFound) {
+			break;
+		}
+	}
+
+	list_free(indexoidlist);
+
+	return ! isMatchedIndexFound;
+}
+
 /*
  * ALTER TABLE ALTER COLUMN SET/DROP DEFAULT
  *
@@ -13628,6 +13775,13 @@ bool
 PartConstraintImpliedByRelConstraint(Relation scanrel,
 									 List *partConstraint)
 {
+	return ConstraintImpliedByRelConstraint(scanrel, partConstraint);
+}
+
+static bool
+ConstraintImpliedByRelConstraint(Relation scanrel,
+									 List *partConstraint)
+{
 	List	   *existConstraint = NIL;
 	TupleConstr *constr = RelationGetDescr(scanrel)->constr;
 	int			num_check,
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 11f0baa..38c007e 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -1005,6 +1005,14 @@ alter table atacc1 alter test set not null;
 ERROR:  column "test" contains null values
 delete from atacc1;
 alter table atacc1 alter test set not null;
+-- set not null can use index
+create index on atacc1 (test);
+alter table atacc1 alter column test drop not null;
+insert into atacc1 values (null);
+alter table atacc1 alter test set not null;
+ERROR:  column "test" contains null values
+delete from atacc1;
+alter table atacc1 alter test set not null;
 -- 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
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 02a33ca..8a1e007 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -763,6 +763,14 @@ alter table atacc1 alter test set not null;
 delete from atacc1;
 alter table atacc1 alter test set not null;
 
+-- set not null can use index
+create index on atacc1 (test);
+alter table atacc1 alter column test drop not null;
+insert into atacc1 values (null);
+alter table atacc1 alter test set not null;
+delete from atacc1;
+alter table atacc1 alter test set not null;
+
 -- try altering a non-existent column, should fail
 alter table atacc1 alter bar set not null;
 alter table atacc1 alter bar drop not null;