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%40mail.gmail.com
Thanks!
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;
