using index or check in ALTER TABLE SET NOT NULL
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:
/messages/by-id/530C10CF.4020101@strangersgate.com
/messages/by-id/CAASwCXdAK55BzuOy_FtYj2zQWg26PriDKL5pRoWiyFJe0eg-Hg@mail.gmail.com
Thanks!
Attachments:
alter_table_set_not_null_by_index_or_check_v1.patchtext/x-diff; name=alter_table_set_not_null_by_index_or_check_v1.patchDownload
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;
On Tue, Nov 28, 2017 at 1:59 PM, Sergei Kornilov <sk@zsrv.org> wrote:
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.
Doing this based on the existence of a valid constraint which implies
that no nulls can be present seems like a good idea. Doing it based
on an index scan doesn't necessarily seem like a good idea. We have
no guarantee at all that the index scan will be faster than scanning
the table would have been, and a single table scan can do multiple
verification steps if, for example, multiple columns are set NOT NULL
at the same time.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert,
* Robert Haas (robertmhaas@gmail.com) wrote:
On Tue, Nov 28, 2017 at 1:59 PM, Sergei Kornilov <sk@zsrv.org> wrote:
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.
Doing this based on the existence of a valid constraint which implies
that no nulls can be present seems like a good idea. Doing it based
on an index scan doesn't necessarily seem like a good idea. We have
no guarantee at all that the index scan will be faster than scanning
the table would have been, and a single table scan can do multiple
verification steps if, for example, multiple columns are set NOT NULL
at the same time.
Isn't the first concern addressed by using SPI..?
As for the second concern, couldn't that be done with a more complicated
query through SPI, though things might have to be restructured some to
make it possible to do that.
Just, generally speaking, this is definitely something that I think we
want and neither of the above concerns seem like they're technical
reasons why we can't use something like this approach, just needs to
perhaps be reworked to handle multiple columns in a single query.
Thanks!
Stephen
Stephen Frost <sfrost@snowman.net> writes:
Isn't the first concern addressed by using SPI..?
I did not look at the patch yet, but TBH if it uses SPI for sub-operations
of ALTER TABLE I think that is sufficient reason to reject it out of hand.
Doing things that way would create way too much of a vulnerability surface
for code touching a partially-updated table. At minimum, we'd have to
blow holes in existing protections like CheckTableNotInUse, and I think
we'd be forever finding other stuff that failed to work quite right in
that context. I do not want ALTER TABLE going anywhere near the planner
or executor; I'm not even happy that it uses the parser (for index
definition reconstruction).
regards, tom lane
Thanks all for reply!
Robert,
Doing it based on an index scan doesn't necessarily seem like a good idea. We have
no guarantee at all that the index scan will be faster than scanning
the table would have been
I agree this. Thinking a little about idea of index scan i can not give reasonable usecase which required index. My target problem of adding NOT NULL to big relation without long downtime can be done with ADD CONSTRAINT NOT VALID, VALIDATE it in second transaction, then SET NOT NULL by my patch and drop unneeded constraint.
Stephen,
Just, generally speaking, this is definitely something that I think we
want and neither of the above concerns seem like they're technical
reasons why we can't use something like this approach, just needs to
perhaps be reworked to handle multiple columns in a single query.
I understood the idea, thank you.
Tom,
I did not look at the patch yet, but TBH if it uses SPI for sub-operations
of ALTER TABLE I think that is sufficient reason to reject it out of hand.
I understood, thank you.
So, i will soon delete SPI usage and index scan and post new simplified patch with verify data only by constraints.
Tom,
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
Stephen Frost <sfrost@snowman.net> writes:
Isn't the first concern addressed by using SPI..?
I did not look at the patch yet, but TBH if it uses SPI for sub-operations
of ALTER TABLE I think that is sufficient reason to reject it out of hand.
You mean like what ALTER TABLE ... ADD FOREIGN KEY does?
Doing things that way would create way too much of a vulnerability surface
for code touching a partially-updated table. At minimum, we'd have to
blow holes in existing protections like CheckTableNotInUse, and I think
we'd be forever finding other stuff that failed to work quite right in
that context. I do not want ALTER TABLE going anywhere near the planner
or executor; I'm not even happy that it uses the parser (for index
definition reconstruction).
That's more along the lines of the kind of response I was expecting
given the suggestion, and perhaps a good reason to just go with the
index-based lookup, when an index is available to do so with, but I'm
not entirely sure how this is different from how we handle foreign keys.
Thanks!
Stephen
On Wed, Nov 29, 2017 at 10:52 AM, Sergei Kornilov <sk@zsrv.org> wrote:
I agree this. Thinking a little about idea of index scan i can not give reasonable usecase which required index. My target problem of adding NOT NULL to big relation without long downtime can be done with ADD CONSTRAINT NOT VALID, VALIDATE it in second transaction, then SET NOT NULL by my patch and drop unneeded constraint.
Yes, I had the same thought.
Thinking a little bit further, maybe the idea you had in mind using
the index scan was that some indexes offer cheap ways of testing
whether ANY nulls are present in the column. For example, if we had a
non-partial btree index whose first column is the one being made NOT
NULL, we could try an index scan - via index_beginscan() /
index_getnext() / index_endscan() - trying to pull exactly one null
from the index, and judge whether or not there are nulls present based
only whether we get one. This would be a lot cheaper than scanning a
large table, but it needs some careful thought because of visibility
issues. It's not sufficient that the index contains no nulls that are
visible to our snapshot; it must contain no nulls that are visible to
any plausible current or future snapshot. I doubt that test can be
written in SQL, but it can probably be written in C. Moreover, we
need to avoid not only false negatives (thinking that there is no NULL
when there is one) but also false positives (thinking there's a NULL
in the column when there isn't, and thus failing spuriously). But it
seems like it might be useful if someone can figure out the details of
how to make it 100% correct; one index lookup is sure to be a lot
quicker than a full table scan.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Stephen Frost <sfrost@snowman.net> writes:
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
I did not look at the patch yet, but TBH if it uses SPI for sub-operations
of ALTER TABLE I think that is sufficient reason to reject it out of hand.
You mean like what ALTER TABLE ... ADD FOREIGN KEY does?
Yeah, and if you look at the warts that SPI has grown to support that
usage, you'll see why I'm so unhappy. We should never have allowed
FKs to be built on top of SPI; they require semantics that don't exist
in SQL. I think this would lead to more of the same --- not exactly
the same of course, but more warts. See Robert's nearby musings about
semantics of index null checks for an example.
regards, tom lane
Robert,
* Robert Haas (robertmhaas@gmail.com) wrote:
On Wed, Nov 29, 2017 at 10:52 AM, Sergei Kornilov <sk@zsrv.org> wrote:
I agree this. Thinking a little about idea of index scan i can not give reasonable usecase which required index. My target problem of adding NOT NULL to big relation without long downtime can be done with ADD CONSTRAINT NOT VALID, VALIDATE it in second transaction, then SET NOT NULL by my patch and drop unneeded constraint.
Yes, I had the same thought.
I have to admit that the case I was thinking about was the one you
outline below..
Thinking a little bit further, maybe the idea you had in mind using
the index scan was that some indexes offer cheap ways of testing
whether ANY nulls are present in the column. For example, if we had a
non-partial btree index whose first column is the one being made NOT
NULL, we could try an index scan - via index_beginscan() /
index_getnext() / index_endscan() - trying to pull exactly one null
from the index, and judge whether or not there are nulls present based
only whether we get one. This would be a lot cheaper than scanning a
large table, but it needs some careful thought because of visibility
issues. It's not sufficient that the index contains no nulls that are
visible to our snapshot; it must contain no nulls that are visible to
any plausible current or future snapshot. I doubt that test can be
written in SQL, but it can probably be written in C. Moreover, we
need to avoid not only false negatives (thinking that there is no NULL
when there is one) but also false positives (thinking there's a NULL
in the column when there isn't, and thus failing spuriously). But it
seems like it might be useful if someone can figure out the details of
how to make it 100% correct; one index lookup is sure to be a lot
quicker than a full table scan.
This was along the lines I was thinking also (though I had thought SPI
might be reasonable given our usage of it with FKs, but if it'd require
uglier warts than what SPI already has, then just finding an
appropriate index and using the internal methods would be best).
As for conflicting snapshots, isn't the lock we're taking already
AccessExclusive..?
Thanks!
Stephen
Here is new patch with check only existed valid constraints and without SPI at all.
Thanks
Attachments:
alter_table_set_not_null_by_constraints_only_v2.patchtext/x-diff; name=alter_table_set_not_null_by_constraints_only_v2.patchDownload
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d979ce2..7ab7580 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -370,6 +370,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 isSetNotNullNeedsTableScan(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 +5865,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 (isSetNotNullNeedsTableScan(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 +5884,42 @@ ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
return address;
}
+static bool
+isSetNotNullNeedsTableScan(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 false;
+ }
+
+ return true;
+}
+
/*
* ALTER TABLE ALTER COLUMN SET/DROP DEFAULT
*
@@ -13620,12 +13660,23 @@ ComputePartitionAttrs(Relation rel, List *partParams, AttrNumber *partattrs,
/*
* PartConstraintImpliedByRelConstraint
* Does scanrel's existing constraints imply the partition constraint?
+ */
+bool
+PartConstraintImpliedByRelConstraint(Relation scanrel,
+ List *partConstraint)
+{
+ return ConstraintImpliedByRelConstraint(scanrel, partConstraint);
+}
+
+/*
+ * PartConstraintImpliedByRelConstraint
+ * Does scanrel's existing constraints imply given constraint
*
* Existing constraints includes its check constraints and column-level
* NOT NULL constraints and partConstraint describes the partition constraint.
*/
-bool
-PartConstraintImpliedByRelConstraint(Relation scanrel,
+static bool
+ConstraintImpliedByRelConstraint(Relation scanrel,
List *partConstraint)
{
List *existConstraint = NIL;
On November 29, 2017 8:50:31 AM PST, Stephen Frost <sfrost@snowman.net> wrote:
As for conflicting snapshots, isn't the lock we're taking already
AccessExclusive..?
Doesn't help if e.g. the current xact is repeatable read or if your own xact deleted things (other xacts with snapshots could still see null rows, despite new take definition).
Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
On Wed, Nov 29, 2017 at 11:53 AM, Sergei Kornilov <sk@zsrv.org> wrote:
Here is new patch with check only existed valid constraints and without SPI at all.
Please use pgindent or anyway try to follow project style. { needs to
go on a line by itself, for example.
isSetNotNullNeedsTableScan seems different than other similarly-named
functions we have. NotNullImpliedByRelConstraints?
It also lacks a header comment.
Having both PartConstraintImpliedByRelConstraint and
ConstraintImpliedByRelConstraint with identical implementations
doesn't seem like a good idea to me. I guess just renaming it is
probably fine.
The comment /* T if we added new NOT NULL constraints */ should
probably be changed to /* T if we should recheck NOT NULL constraints
*/ or similar.
I'd suggest registering this with the next CommitFest.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Thank you for review! My apologies for my errors. It seems i read developers wiki pages not enough carefully. I will reread wiki, code style and then update patch with all your remarks.
The comment /* T if we added new NOT NULL constraints */ should
probably be changed to /* T if we should recheck NOT NULL constraints
*/ or similar.
Correct. Hm, probably I will also rename this property to verify_new_notnull for clarity
I'd suggest registering this with the next CommitFest.
Already done in https://commitfest.postgresql.org/16/1389/ , i noticed this step in wiki
Hello
I update patch and also rebase to current head. I hope now it is better aligned with project style
Attachments:
alter_table_set_not_null_by_constraints_only_v3.patchtext/x-diff; name=alter_table_set_not_null_by_constraints_only_v3.patchDownload
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index dd4a8d3..36bcb3f 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -1227,7 +1227,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",
@@ -1271,8 +1271,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 d979ce2..35eac39 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 */
@@ -370,6 +370,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,
@@ -4304,7 +4305,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);
@@ -4465,7 +4466,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
@@ -5461,7 +5462,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;
}
/*
@@ -5863,8 +5864,11 @@ 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 (!NotNullImpliedByRelConstraints(rel, (Form_pg_attribute) GETSTRUCT(tuple)))
+ {
+ /* Tell Phase 3 it needs to test the constraint */
+ tab->verify_new_notnull = true;
+ }
ObjectAddressSubSet(address, RelationRelationId,
RelationGetRelid(rel), attnum);
@@ -5881,6 +5885,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.
@@ -13618,15 +13662,14 @@ ComputePartitionAttrs(Relation rel, List *partParams, AttrNumber *partattrs,
}
/*
- * PartConstraintImpliedByRelConstraint
- * Does scanrel's existing constraints imply the partition constraint?
+ * ConstraintImpliedByRelConstraint
+ * Does scanrel's existing constraints imply given constraint
*
* Existing constraints includes its check constraints and column-level
* NOT NULL constraints and partConstraint describes the partition constraint.
*/
bool
-PartConstraintImpliedByRelConstraint(Relation scanrel,
- List *partConstraint)
+ConstraintImpliedByRelConstraint(Relation scanrel, List *partConstraint)
{
List *existConstraint = NIL;
TupleConstr *constr = RelationGetDescr(scanrel)->constr;
@@ -13724,7 +13767,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,
@@ -13776,7 +13819,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 da3ff5d..41e8cdf 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 *partConstraint);
#endif /* TABLECMDS_H */
Greetings Sergei,
* Sergei Kornilov (sk@zsrv.org) wrote:
I update patch and also rebase to current head. I hope now it is better aligned with project style
Thanks for updating it and continuing to work on it. I haven't done a
full review but there were a few things below that I thought could be
improved-
@@ -5863,8 +5864,11 @@ 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 (!NotNullImpliedByRelConstraints(rel, (Form_pg_attribute) GETSTRUCT(tuple))) + { + /* Tell Phase 3 it needs to test the constraint */ + tab->verify_new_notnull = true; + }ObjectAddressSubSet(address, RelationRelationId,
RelationGetRelid(rel), attnum);
This could really use some additional comments, imv. In particular, we
need to make it clear that verify_new_notnull only moves from the
initial 'false' value to 'true', since we could be asked to add multiple
NOT NULL constraints and if any of them aren't already covered by an
existing CHECK constraint then we need to perform the full table check.
@@ -13618,15 +13662,14 @@ ComputePartitionAttrs(Relation rel, List *partParams, AttrNumber *partattrs,
}/* - * PartConstraintImpliedByRelConstraint - * Does scanrel's existing constraints imply the partition constraint? + * ConstraintImpliedByRelConstraint + * Does scanrel's existing constraints imply given constraint * * Existing constraints includes its check constraints and column-level * NOT NULL constraints and partConstraint describes the partition constraint. */ bool -PartConstraintImpliedByRelConstraint(Relation scanrel, - List *partConstraint) +ConstraintImpliedByRelConstraint(Relation scanrel, List *partConstraint) { List *existConstraint = NIL; TupleConstr *constr = RelationGetDescr(scanrel)->constr;
I would also rename 'partConstraint' since this function is no longer,
necessairly, working with a partition's constraint.
This could also really use some regression tests and I'd be sure to
include tests of adding multiple NOT NULL constraints, sometimes where
they're all covered by existing CHECK constraints and other times when
only one or the other is (and there's existing NULL values in the other
column), etc.
Thanks!
Stephen
Hello Stephen
I changed the suggested things and added some regression tests. Also i wrote few words to the documentation. New patch is attached.
Thank you for feedback!
regards, Sergei
Attachments:
alter_table_set_not_null_by_constraints_only_v4.patchtext/x-diff; name=alter_table_set_not_null_by_constraints_only_v4.patchDownload
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 286c7a8..cf2c56f 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -189,6 +189,13 @@ ALTER TABLE [ IF EXISTS ] <replaceable class="parameter">name</replaceable>
</para>
<para>
+ Full table scan is performed to check that no existing row
+ in the table has null values in given column. It is possible to avoid
+ this scan by adding a valid <literal>CHECK</literal> constraint to
+ the table that would allow only NOT NULL values for given column.
+ </para>
+
+ <para>
If this table is a partition, one cannot perform <literal>DROP NOT NULL</literal>
on a column if it is marked <literal>NOT NULL</literal> in the parent
table. To drop the <literal>NOT NULL</literal> constraint from all the
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 8adc4ee..6f7ec4a 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -1235,7 +1235,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",
@@ -1279,8 +1279,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 2e768dd..4f0916d 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,
@@ -4368,7 +4369,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);
@@ -4529,7 +4530,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
@@ -5528,7 +5529,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;
}
/*
@@ -5930,8 +5931,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);
@@ -5948,6 +5958,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.
@@ -13621,15 +13671,14 @@ ComputePartitionAttrs(Relation rel, List *partParams, AttrNumber *partattrs,
}
/*
- * PartConstraintImpliedByRelConstraint
- * Does scanrel's existing constraints imply the partition constraint?
+ * ConstraintImpliedByRelConstraint
+ * Does scanrel's existing constraints imply given constraint
*
- * Existing constraints includes its check constraints and column-level
+ * Existing constraints includes its check constraints, column-level
* NOT NULL constraints and partConstraint describes the partition constraint.
*/
bool
-PartConstraintImpliedByRelConstraint(Relation scanrel,
- List *partConstraint)
+ConstraintImpliedByRelConstraint(Relation scanrel, List *testConstraint)
{
List *existConstraint = NIL;
TupleConstr *constr = RelationGetDescr(scanrel)->constr;
@@ -13699,7 +13748,7 @@ PartConstraintImpliedByRelConstraint(Relation scanrel,
existConstraint = list_make1(make_ands_explicit(existConstraint));
/* And away we go ... */
- return predicate_implied_by(partConstraint, existConstraint, true);
+ return predicate_implied_by(testConstraint, existConstraint, true);
}
/*
@@ -13727,7 +13776,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,
@@ -13779,7 +13828,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 e9a1d37..491bf60 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 b27e8f6..1a64a55 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);
Hello Sergei,
I couldn't find any case when your code doesn't work properly. So it
seems ok to me.
@@ -220,6 +220,13 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
</para><para> + Full table scan is performed to check that no existing row + in the table has null values in given column. It is possible to avoid + this scan by adding a valid <literal>CHECK</literal> constraint to + the table that would allow only NOT NULL values for given column. + </para>
Adding check constraint will also force the full table scan. So I think
it would be better to rephrased it as follows:
"Full table scan is performed to check that column doesn't contain NULL
values unless there are valid check constraints that prohibit NULL
values for specified column. In the latter case table scan is skipped."
A native English speaker input would be helpful here.
Regarding regression tests it may be useful to set client_min_messages
to 'debug1' before setting "NOT NULL" attribute for a column. In this
case you can tell for sure that NotNullImpliedByRelConstraints()
returned true (i.e. your code actually did the job) as the special debug
message is printed to the log.
Thanks!
--
Ildar Musin
i.musin@postgrespro.ru
Hello
thank you for review!
Adding check constraint will also force the full table scan. So I think
it would be better to rephrased it as follows:
Agree. I updated docs in new attached patch slightly different
Regarding regression tests it may be useful to set client_min_messages
to 'debug1' before setting "NOT NULL" attribute for a column. In this
case you can tell for sure that NotNullImpliedByRelConstraints()
returned true (i.e. your code actually did the job) as the special debug
message is printed to the log.
I can not find any debug messages in tests: grep client_min_messages -irn src/test/ Only some warning level and few error. So i verify in regression tests only top-level behavior.
Or this paragraph was for other people, not for tests?
regards, Sergei
Attachments:
alter_table_set_not_null_by_constraints_only_v5.patchtext/x-diff; name=alter_table_set_not_null_by_constraints_only_v5.patchDownload
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 74e020b..3f7218d 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,15 +13700,14 @@ ComputePartitionAttrs(Relation rel, List *partParams, AttrNumber *partattrs,
}
/*
- * PartConstraintImpliedByRelConstraint
- * Does scanrel's existing constraints imply the partition constraint?
+ * ConstraintImpliedByRelConstraint
+ * Does scanrel's existing constraints imply given constraint
*
- * Existing constraints includes its check constraints and column-level
+ * Existing constraints includes its check constraints, column-level
* NOT NULL constraints and partConstraint describes the partition constraint.
*/
bool
-PartConstraintImpliedByRelConstraint(Relation scanrel,
- List *partConstraint)
+ConstraintImpliedByRelConstraint(Relation scanrel, List *testConstraint)
{
List *existConstraint = NIL;
TupleConstr *constr = RelationGetDescr(scanrel)->constr;
@@ -13728,7 +13777,7 @@ PartConstraintImpliedByRelConstraint(Relation scanrel,
existConstraint = list_make1(make_ands_explicit(existConstraint));
/* And away we go ... */
- return predicate_implied_by(partConstraint, existConstraint, true);
+ return predicate_implied_by(testConstraint, existConstraint, true);
}
/*
@@ -13756,7 +13805,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,
@@ -13808,7 +13857,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);
On 06.03.2018 16:12, Sergei Kornilov wrote:
Hello thank you for review!
Adding check constraint will also force the full table scan. So I
think it would be better to rephrased it as follows:Agree. I updated docs in new attached patch slightly different
Regarding regression tests it may be useful to set
client_min_messages to 'debug1' before setting "NOT NULL" attribute
for a column. In this case you can tell for sure that
NotNullImpliedByRelConstraints() returned true (i.e. your code
actually did the job) as the special debug message is printed to
the log.I can not find any debug messages in tests: grep client_min_messages
-irn src/test/ Only some warning level and few error. So i verify in
regression tests only top-level behavior. Or this paragraph was for
other people, not for tests?
Sorry, probably I didn't explain it clearly enough. I meant that your
regression tests can't distinguish cases when the full table scan was
actually performed from the ones when it was skipped due to
NotNullImpliedByRelConstraints() check. For instance, consider this
piece from the test suite:
# create table atacc1 (test_a int, test_b int);
CREATE TABLE
...
# alter table atacc1 add constraint atacc1_constr_a_valid check(test_a
is not null);
ALTER TABLE
# alter table atacc1 alter test_a set not null;
ALTER TABLE
It is not obvious that full table scan was omitted. But if we set
client_min_messages to 'debug1', we'll be able to see what is really
happened by the different debug messages. For example:
# create table atacc1 (test_a int, test_b int);
CREATE TABLE
...
# set client_min_messages to 'debug1';
SET
# alter table atacc1 alter test_a set not null;
DEBUG: verifying table "atacc1" <<<< full table scan!
ALTER TABLE
# alter table atacc1 alter test_a drop not null;
ALTER TABLE
# alter table atacc1 add constraint atacc1_constr_a_valid check(test_a
is not null);
DEBUG: verifying table "atacc1"
ALTER TABLE
# alter table atacc1 alter test_a set not null;
DEBUG: verifying table "atacc1" NOT NULL constraint on test_a attribute
by existed constraints <<<< full scan was skipped!
ALTER TABLE
--
Ildar Musin
i.musin@postgrespro.ru
Hello, Ildar
Thanks. I looked ATTACH PARTITION tests and found such checks. If no one is against i will just use in my patch INFO level instead DEBUG1, similarly ATTACH PARTITION code. Updated patch attached.
Or i can rewrite tests to use DEBUG1 level.
regards, Sergei
Attachments:
alter_table_set_not_null_by_constraints_only_v6.patchtext/x-diff; name=alter_table_set_not_null_by_constraints_only_v6.patchDownload
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 74e020b..c35539a 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(INFO,
+ (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,15 +13700,14 @@ ComputePartitionAttrs(Relation rel, List *partParams, AttrNumber *partattrs,
}
/*
- * PartConstraintImpliedByRelConstraint
- * Does scanrel's existing constraints imply the partition constraint?
+ * ConstraintImpliedByRelConstraint
+ * Does scanrel's existing constraints imply given constraint
*
- * Existing constraints includes its check constraints and column-level
+ * Existing constraints includes its check constraints, column-level
* NOT NULL constraints and partConstraint describes the partition constraint.
*/
bool
-PartConstraintImpliedByRelConstraint(Relation scanrel,
- List *partConstraint)
+ConstraintImpliedByRelConstraint(Relation scanrel, List *testConstraint)
{
List *existConstraint = NIL;
TupleConstr *constr = RelationGetDescr(scanrel)->constr;
@@ -13728,7 +13777,7 @@ PartConstraintImpliedByRelConstraint(Relation scanrel,
existConstraint = list_make1(make_ands_explicit(existConstraint));
/* And away we go ... */
- return predicate_implied_by(partConstraint, existConstraint, true);
+ return predicate_implied_by(testConstraint, existConstraint, true);
}
/*
@@ -13756,7 +13805,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,
@@ -13808,7 +13857,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..6904ff8 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -1038,6 +1038,47 @@ 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;
+INFO: verifying table "atacc1" NOT NULL constraint on test_a attribute by existed constraints
+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;
+INFO: verifying table "atacc1" NOT NULL constraint on test_a attribute by existed constraints
+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;
+INFO: verifying table "atacc1" NOT NULL constraint on test_a attribute by existed constraints
+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;
+INFO: verifying table "atacc1" NOT NULL constraint on test_a attribute by existed constraints
+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;
+INFO: verifying table "atacc1" NOT NULL constraint on test_b attribute by existed constraints
+INFO: verifying table "atacc1" NOT NULL constraint on test_a attribute by existed constraints
+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);
Sergei Kornilov wrote:
Hello, Ildar
Thanks. I looked ATTACH PARTITION tests and found such checks. If no one is against i will just use in my patch INFO level instead DEBUG1, similarly ATTACH PARTITION code. Updated patch attached.Or i can rewrite tests to use DEBUG1 level.
You should be able to use an event trigger that raises a message when
table_rewrite is hit, to notify the test driver that a rewrite happens.
(If any DDL that causes a table rewrite fails to trigger the
table_rewrite event correctly, that is a bug.)
ISTM that depending on DEBUG messages is bad because debugging lines
added elsewhere will make your tests fail; and INFO is generally frowned
upon (I, for one, would frown upon an INFO message raised by ALTER
TABLE, for sure.) so let's not do that either.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hello
You should be able to use an event trigger that raises a message when
table_rewrite is hit, to notify the test driver that a rewrite happens.
Here is no table rewrite, only verify. So here EventTriggerTableRewrite is not called. Or i missed something?
ISTM that depending on DEBUG messages is bad because debugging lines
added elsewhere will make your tests fail;
I agree and this is reason why i not used DEBUG message in tests as was proposed. I found INFO messages in tests and decided that this was an acceptable option year ago.
and INFO is generally frowned upon (I, for one, would frown upon an INFO message raised by ALTER
TABLE, for sure.) so let's not do that either.
In this case we need remove INFO from ALTER TABLE ATTACH PARTITION code and tests. I think is a bad idea of using different rules for same stuff, right? Probably i can do this work too.
But it is necessary to decide how the test should look.
regards, Sergei
Sergei Kornilov <sk@zsrv.org> writes:
ISTM that depending on DEBUG messages is bad because debugging lines
added elsewhere will make your tests fail;
I agree and this is reason why i not used DEBUG message in tests as was proposed. I found INFO messages in tests and decided that this was an acceptable option year ago.
INFO is for user-facing messages, such as output from VACUUM VERBOSE,
where the user has specifically requested the output. I do not think
this falls into that category. Even if you persuade some committer
to commit it like that, there will be user pushback asking us to get
this noise out of their faces ... and then we'll have to find some
other way to test it.
Do you actually need test output proving that this code path was taken
rather than the default one? Seems like looking at the code coverage
report might be enough.
In this case we need remove INFO from ALTER TABLE ATTACH PARTITION code and tests. I think is a bad idea of using different rules for same stuff, right? Probably i can do this work too.
I did not see any INFO messages in a quick test of ALTER TABLE ATTACH
PARTITION, but if there are any lurking in there, they probably need
to be downgraded.
regards, tom lane
Hello
Do you actually need test output proving that this code path was taken
rather than the default one? Seems like looking at the code coverage
report might be enough.
I not known. In v4 i use DEBUG1 message and do not check code path in tests at all: by full table scan or by constraint, i tested only command result to not break behavior.
Today Ildar Musin proposed to test code path through NotNullImpliedByRelConstraints function. This is my first patch and I do not have the confidence to write a test. So I looked more closely at the alter table tests, found several info in attach partition and updated my patch.
I did not see any INFO messages in a quick test of ALTER TABLE ATTACH
PARTITION, but if there are any lurking in there, they probably need
to be downgraded.
In src/test/regress/expected/alter_table.out i found 7 test with
INFO: partition constraint for table "..." is implied by existing constraints
and 5 with
INFO: updated partition constraint for default partition "..." is implied by existing constraints
ereport's are in ValidatePartitionConstraints function src/backend/commands/tablecmds.c
regards, Sergei
Hello Sergei, Alvaro, Tom,
On 06.03.2018 20:25, Alvaro Herrera wrote:
You should be able to use an event trigger that raises a message when
table_rewrite is hit, to notify the test driver that a rewrite happens.(If any DDL that causes a table rewrite fails to trigger the
table_rewrite event correctly, that is a bug.)
It seems that 'table_rewrite' event trigger isn't fired in case of
simple scan (without actual rewrite).
ISTM that depending on DEBUG messages is bad because debugging lines
added elsewhere will make your tests fail;
I think that between test depending on DEBUG message and no test at all
the former one is preferable. Are there other options to test it?
On 06.03.2018 21:51, Tom Lane wrote:
Do you actually need test output proving that this code path was taken
rather than the default one? Seems like looking at the code coverage
report might be enough.
Code coverage cannot guarantee correctness. I think there should be at
least two tests:
* full scan is performed when there are no check constraints that
restrict NULL values;
* full scan is omitted when there are such constraints.
The last one could also include some unobvious cases like CHECK (col >
0), complex expressions with boolean operators etc.
PS: Btw, some check constraints that implies NOT NULL still won't
prevent from full table scan. For instance:
# create table test (a int, b int check ((a + b) is not null));
CREATE TABLE
# set client_min_messages to 'debug1';
SET
# insert into test values (1, 1);
INSERT 0 1
# alter table test alter column a set not null;
DEBUG: verifying table "test" <<<< full table scan!
ALTER TABLE
Since operator+ (aka int4pl) is strict it returns NULL if at least one
of its operands is NULL. And this check constraint ensures therefore
that both columns 'a' and 'b' are NOT NULL. It isn't probably the issue
of this patch, just something that someone may find interesting.
--
Ildar Musin
i.musin@postgrespro.ru
Hello all!
Summarizing, how i must update tests?
We want test real skip of verify phase or only result correctness?
I can verify what i do not break alter table by tests in v4 of my patch. But here is no check code path. So my feature may be broken in future without test fail. alter table will be correct regardless this feature.
Or i must test code path too? I understood Tom and will remove INFO ereport from patch anyway. But currently i have no idea how check code path. As mentioned earlier using debug ereport is uncomfortable too. Here is no suitable event trigger to track what exactly happens with table. Adding new one for test purposes seems overkill.
regards, Sergei
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
Attachments:
alter_table_set_not_null_by_constraints_only_v7.patchtext/x-diff; name=alter_table_set_not_null_by_constraints_only_v7.patchDownload
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);
Hello Sergei,
On 10.03.2018 12:35, Sergei Kornilov wrote:
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
Ok, I can't think of any other ways to test it so I have to agree with
Tom Lane i.e. rely only on coverage. (There also were another suggestion
to use statistics [select seq_scan from pg_stat_user_tables where
relid='test'::regclass] which show number of table scans. But statistics
is collected by stat collector process with some latency and hence
cannot be reliable for tests).
Patch seems correct to me, it applies and compiles cleanly, docs
compiles as well, tests pass. Changed status to Ready for Committer.
--
Ildar Musin
i.musin@postgrespro.ru
On 11/29/17 10:52, Sergei Kornilov wrote:
My target problem of adding NOT NULL to big relation without long downtime can be done with ADD CONSTRAINT NOT VALID, VALIDATE it in second transaction, then SET NOT NULL by my patch and drop unneeded constraint.
It seems to me that this is a workaround for not properly cataloguing
NOT NULL constraints. If we fixed that, you could do SET NOT NULL NOT
VALID and go from there. Maybe we should look again into fixing that.
That would solve so many problems.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hello, Peter!
I agree my patch seems as strange workaround. And better will be SET NOT NULL NOT VALID feature. But this change is very complicated for me
For now my patch is small, simple and provides way for users.
regards, Sergei
06.04.2018, 06:29, "Peter Eisentraut" <peter.eisentraut@2ndquadrant.com>:
Show quoted text
On 11/29/17 10:52, Sergei Kornilov wrote:
My target problem of adding NOT NULL to big relation without long downtime can be done with ADD CONSTRAINT NOT VALID, VALIDATE it in second transaction, then SET NOT NULL by my patch and drop unneeded constraint.
It seems to me that this is a workaround for not properly cataloguing
NOT NULL constraints. If we fixed that, you could do SET NOT NULL NOT
VALID and go from there. Maybe we should look again into fixing that.
That would solve so many problems.--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hello
I notice my patch does not apply again. Here is update to current HEAD
regards, Sergei
Attachments:
alter_table_set_not_null_by_constraints_only_v8.patchtext/x-diff; name=alter_table_set_not_null_by_constraints_only_v8.patchDownload
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 43b2fce..11dc92c 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,
@@ -4462,7 +4463,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);
@@ -4623,7 +4624,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
@@ -5658,7 +5659,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;
}
}
@@ -6062,8 +6063,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);
@@ -6080,6 +6090,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.
@@ -13851,16 +13901,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;
@@ -13929,13 +13978,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);
}
/*
@@ -13963,7 +14012,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,
@@ -14015,7 +14064,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 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 1d6cc9c..d486dd1 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 61799b6..eca8f05 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);
Hi
I noticed new merge conflict, updated version attached.
regards, Sergei
Attachments:
alter_table_set_not_null_by_constraints_only_v9.patchtext/x-diff; name=alter_table_set_not_null_by_constraints_only_v9.patchDownload
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);
Hello
Attached updated patch follows recent Reorganize partitioning code commit.
regards, Sergei
Attachments:
alter_table_set_not_null_by_constraints_only_v10.patchtext/x-diff; name=alter_table_set_not_null_by_constraints_only_v10.patchDownload
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/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index c1a9bda..164c148 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 */
@@ -374,6 +374,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,
@@ -4465,7 +4466,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);
@@ -4626,7 +4627,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
@@ -5661,7 +5662,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;
}
}
@@ -6065,8 +6066,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);
@@ -6083,6 +6093,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.
@@ -13854,16 +13904,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;
@@ -13932,13 +13981,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);
}
/*
@@ -13960,7 +14009,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/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index 229ce3f..e53e317 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -615,7 +615,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",
@@ -659,7 +659,7 @@ 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,
+ if (ConstraintImpliedByRelConstraint(part_rel,
def_part_constraints))
{
ereport(INFO,
diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h
index d46e09c..8b6f13d 100644
--- a/src/include/commands/tablecmds.h
+++ b/src/include/commands/tablecmds.h
@@ -93,7 +93,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 50b9443..4d07212 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 d508a69..6b2a3d1 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);
On Sun, 15 Apr 2018 at 09:09, Sergei Kornilov <sk@zsrv.org> wrote:
Attached updated patch follows recent Reorganize partitioning code commit.
regards, Sergei
This patch went through the last tree commit fests without any noticeable
activity, but cfbot says it still applies and doesn't break any tests. The
patch itself is rather small, the only significant objection I see from the
thread is that probably "SET NOT NULL NOT VALID" feature could be more
appropriate way of fixing the original problem, but looks like no one is
working on that (the only related thread I could found was this one [1]/messages/by-id/CAASwCXcOokBqHf8BXECbDgOLkXxJyL_Q_twhg2dGWSvgMzz=xA@mail.gmail.com). If
not properly cataloguing NOT NULL constraints would be fixed, can it
potentially conflict with the current patch or not?
[1]: /messages/by-id/CAASwCXcOokBqHf8BXECbDgOLkXxJyL_Q_twhg2dGWSvgMzz=xA@mail.gmail.com
Hello
This patch went through the last tree commit fests without any noticeable activity
Well, last two CF. During march commitfest patch has activity and was marked as ready for committer. But at end of july CF was no further activity and patch was reverted back to "need review" with reason [1]/messages/by-id/c4c7556d-a046-ac29-2549-bdef0078b6fe@2ndQuadrant.com:
It's not clear that there is consensus that this is wanted.
I can't say something here.
If not properly cataloguing NOT NULL constraints would be fixed, can it
potentially conflict with the current patch or not?
We already doing same stuff for "alter table attach partition" and in this patch i use exactly this routine. If proper cataloguing would conflict with my patch - it would conflict with "attach partition" validation too.
I think proper cataloguing can be implemented without conflict with proposed feature.
regards, Sergei
[1]: /messages/by-id/c4c7556d-a046-ac29-2549-bdef0078b6fe@2ndQuadrant.com
On Sun, 4 Nov 2018 at 19:03, Sergei Kornilov <sk@zsrv.org> wrote:
If not properly cataloguing NOT NULL constraints would be fixed, can it
potentially conflict with the current patch or not?We already doing same stuff for "alter table attach partition" and in this patch i use exactly this routine. If proper cataloguing would conflict with my patch - it would conflict with "attach partition" validation too.
I think proper cataloguing can be implemented without conflict with proposed feature.
Yes, indeed, this patch relies on the PartConstraintImpliedByRelConstraint.
Then maybe it makes sense to go with the solution, proposed in this thread,
while leaving open the possibility of having "SET NOT NULL NOT VALID"? From
the functionality point of view it definitely would be beneficial. Any other
opinions?
On 2018-Nov-13, Dmitry Dolgov wrote:
On Sun, 4 Nov 2018 at 19:03, Sergei Kornilov <sk@zsrv.org> wrote:
If not properly cataloguing NOT NULL constraints would be fixed, can it
potentially conflict with the current patch or not?We already doing same stuff for "alter table attach partition" and in this patch i use exactly this routine. If proper cataloguing would conflict with my patch - it would conflict with "attach partition" validation too.
I think proper cataloguing can be implemented without conflict with proposed feature.Yes, indeed, this patch relies on the PartConstraintImpliedByRelConstraint.
Then maybe it makes sense to go with the solution, proposed in this thread,
while leaving open the possibility of having "SET NOT NULL NOT VALID"? From
the functionality point of view it definitely would be beneficial. Any other
opinions?
I think we should ignore any SET NOT NULL NOT VALID patches, because
in practice they don't exist. Let's move forward with this, because the
optimization being proposed is clearly very useful.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Nov 13, 2018 at 1:59 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
On 2018-Nov-13, Dmitry Dolgov wrote:
On Sun, 4 Nov 2018 at 19:03, Sergei Kornilov <sk@zsrv.org> wrote:
If not properly cataloguing NOT NULL constraints would be fixed, can it
potentially conflict with the current patch or not?We already doing same stuff for "alter table attach partition" and in this patch i use exactly this routine. If proper cataloguing would conflict with my patch - it would conflict with "attach partition" validation too.
I think proper cataloguing can be implemented without conflict with proposed feature.Yes, indeed, this patch relies on the PartConstraintImpliedByRelConstraint.
Then maybe it makes sense to go with the solution, proposed in this thread,
while leaving open the possibility of having "SET NOT NULL NOT VALID"? From
the functionality point of view it definitely would be beneficial. Any other
opinions?I think we should ignore any SET NOT NULL NOT VALID patches, because
in practice they don't exist. Let's move forward with this, because the
optimization being proposed is clearly very useful.
Absolutely agree. Looking at the history of the patch I see that it went
through some extensive review and even was marked as Ready for Committer before
the commentary from Peter, but since then some changes were made (rebase and
code reorganization). Can someone from the reviewers confirm (or not) if the
patch is in a good shape? If no, then I'll try to allocate some time for that,
but can't promise any exact date.
On Thu, Nov 22, 2018 at 6:04 PM Dmitry Dolgov <9erthalion6@gmail.com> wrote:
Absolutely agree. Looking at the history of the patch I see that it went
through some extensive review and even was marked as Ready for Committer before
the commentary from Peter, but since then some changes were made (rebase and
code reorganization). Can someone from the reviewers confirm (or not) if the
patch is in a good shape? If no, then I'll try to allocate some time for that,
but can't promise any exact date.
Ok, I've reviewed this patch a bit. I don't see any significant problems with
the code itself, but to be honest I've got an impression that simplicity of
this patch is misleading and it covers too narrow use case. E.g. one can do
something equal to SET NOT NULL, like adding a new constraint
CREATE TABLE test(data text, CHECK(data IS NOT NULL));
ALTER TABLE test ADD CONTSTRAINT data_chk CHECK (data is not null);
or use not null domain
CREATE DOMAIN not_null AS text CHECK(VALUE IS NOT NULL);
CREATE TABLE test(data not_null);
ALTER TABLE test ALTER COLUMN data SET NOT NULL;
and it still leads to a full table scan. For foreign tables the control flow
jump over NotNullImpliedByRelConstraints, so this logic is never checked. I'm
not sure if taking care of mentioned examples is enough (maybe there are more
of them), or the patch needs to suggest some more general solution here.
Of course there is a possibility that improvement even in this form for this
narrow use case is better than potentially more general approach in the future.
Any opinions about this?
On Sun, 15 Apr 2018 at 19:09, Sergei Kornilov <sk@zsrv.org> wrote:
Attached updated patch follows recent Reorganize partitioning code commit.
I've made a pass over v10. I think it's in pretty good shape, but I
did end up changing a few small things.
1. Tweaked the documents a bit. I was going to just change "Full table
scan" to "A full table scan", but I ended up rewriting the entire
paragraph.
2. In ATRewriteTables, updated comment to mention "If required" since
the scan may not be required if SET NOT NULLs are already proved okay.
3. Updated another forgotten comment in ATRewriteTables.
4. Removed mention about table's with OIDs in ATExecAddColumn(). This
seems left over from 578b229718.
5. Changed ATExecSetNotNull not to check for matching constraints if
we're already going to make a pass over the table. Setting
verify_new_notnull to true again does not make it any more true. :)
6. Tweaked NotNullImpliedByRelConstraints() a bit. You were calling
lappend on an empty list. make_list1() is fine. Don't need a variable
for that, just pass it to the function.
7. Tightened up the comments in ConstraintImpliedByRelConstraint() to
mention that it only supports immutable quals containing vars with
varno=1. Removed the comment near the bottom that mentioned that in
passing.
The only thing that I'm a bit unsure of is the tests. I've read the
thread and I see the discussion above about it. I'd personally have
thought INFO was fine since ATTACH PARTITION does that, but I see
objections. It appears that all the tests just assume that the CHECK
constraint was used to validate the SET NOT NULL. I'm not all that
sure if there's any good reason not to set client_min_messages =
'debug1' just before your test then RESET client_min_messages;
afterwards. No other tests seem to do it, but my only thoughts on the
reason not to would be that it might fail if someone added another
debug somewhere, but so what? they should update the expected results
too.
Oh, one other thing I don't really like is that
ConstraintImpliedByRelConstraint() adds all of the other column's IS
NOT NULL constraints to the existConstraint. This is always wasted
effort for the SET NOT NULL use case. I wonder if a new flag should
be added to control this, or even better, separate that part out and
put back the function named PartConstraintImpliedByRelConstraint and
have it do the IS NOT NULL building before calling
ConstraintImpliedByRelConstraint(). No duplicate code that way and you
also don't need to touch partbound.c
Setting this on waiting on author.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachments:
alter_table_set_not_null_by_constraints_only_v10_fixes.patchapplication/octet-stream; name=alter_table_set_not_null_by_constraints_only_v10_fixes.patchDownload
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);
}
Hello, David!
I've made a pass over v10. I think it's in pretty good shape, but I
did end up changing a few small things.
Thank you! I merged your changes to new patch version.
The only thing that I'm a bit unsure of is the tests. I've read the
thread and I see the discussion above about it. I'd personally have
thought INFO was fine since ATTACH PARTITION does that, but I see
objections.
It is unclear for me if we have consensus about INFO ereport in ATTACH PARTITION.
It appears that all the tests just assume that the CHECK
constraint was used to validate the SET NOT NULL. I'm not all that
sure if there's any good reason not to set client_min_messages =
'debug1' just before your test then RESET client_min_messages;
afterwards. No other tests seem to do it, but my only thoughts on the
reason not to would be that it might fail if someone added another
debug somewhere, but so what? they should update the expected results
too.
Not sure we have consensus about debug messages in tests, so I did such test changes in additional patch.
separate that part out and
put back the function named PartConstraintImpliedByRelConstraint and
have it do the IS NOT NULL building before calling
ConstraintImpliedByRelConstraint(). No duplicate code that way and you
also don't need to touch partbound.c
In this case we need extra argument for ConstraintImpliedByRelConstraint for some caller-provided existConstraint, right? Along with Relation itself? Then I need make copy of existConstraint, append relation constraints and call predicate_implied_by. If I understood correctly pseudocode would be:
PartConstraintImpliedByRelConstraint(rel, testConstraint)
generate notNullConstraint from NOT NULL table attributes
call ConstraintImpliedByRelConstraint(rel, testConstraint, notNullConstraint)
ConstraintImpliedByRelConstraint(rel, testConstraint, existsConstraint)
copy existsConstraint to localExistsConstraint variable
append relation constraints to localExistsConstraint list
call predicate_implied_by
I thing redundant IS NOT NULL check is not issue here and we not need extra arguments for ConstraintImpliedByRelConstraint. Was not changed in attached version, but I will change if you think this would be better design.
regards, Sergei
Attachments:
0001_alter_table_set_not_null_v11.patchtext/x-diff; name=0001_alter_table_set_not_null_v11.patchDownload
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 890b23afd6..926b3361ea 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -214,8 +214,17 @@ 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>
+ <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 a93b13c2fe..b03e1bc875 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -158,7 +158,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 */
@@ -370,6 +370,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,
@@ -4506,10 +4507,11 @@ 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->new_notnull ||
+ if (tab->constraints != NIL || tab->verify_new_notnull ||
tab->partition_constraint != NULL)
ATRewriteTable(tab, InvalidOid, lockmode);
@@ -4670,13 +4672,13 @@ 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
- * 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++)
{
@@ -5682,11 +5684,9 @@ 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->new_notnull |= colDef->is_not_null;
+ tab->verify_new_notnull |= colDef->is_not_null;
}
}
@@ -6054,8 +6054,19 @@ 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;
+ /*
+ * 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 (!tab->verify_new_notnull &&
+ !NotNullImpliedByRelConstraints(rel, (Form_pg_attribute) GETSTRUCT(tuple)))
+ {
+ /* Tell Phase 3 it needs to test the constraint */
+ tab->verify_new_notnull = true;
+ }
ObjectAddressSubSet(address, RelationRelationId,
RelationGetRelid(rel), attnum);
@@ -6071,6 +6082,43 @@ ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
return address;
}
+/*
+ * NotNullImpliedByRelConstraints
+ * Does rel's existing constraints imply NOT NULL for the given attribute
+ */
+static bool
+NotNullImpliedByRelConstraints(Relation rel, Form_pg_attribute attr)
+{
+ NullTest *nnulltest = 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;
+
+ if (ConstraintImpliedByRelConstraint(rel, list_make1(nnulltest)))
+ {
+ 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
*
@@ -14317,16 +14365,16 @@ ComputePartitionAttrs(ParseState *pstate, Relation rel, List *partParams, AttrNu
}
/*
- * PartConstraintImpliedByRelConstraint
- * Do scanrel's existing constraints imply the partition constraint?
+ * ConstraintImpliedByRelConstraint
+ * Do scanrel's existing constraints imply the given constraint?
*
* "Existing constraints" include its check constraints and column-level
- * NOT NULL constraints. partConstraint describes the partition 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
-PartConstraintImpliedByRelConstraint(Relation scanrel,
- List *partConstraint)
+ConstraintImpliedByRelConstraint(Relation scanrel, List *testConstraint)
{
List *existConstraint = NIL;
TupleConstr *constr = RelationGetDescr(scanrel)->constr;
@@ -14395,13 +14443,9 @@ 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.
- *
- * 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.
+ * not-false and try to prove the same for testConstraint.
*/
- return predicate_implied_by(partConstraint, existConstraint, true);
+ return predicate_implied_by(testConstraint, existConstraint, true);
}
/*
@@ -14423,7 +14467,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/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index e71eb3793b..a6bf41ac11 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -1177,7 +1177,7 @@ check_default_partition_contents(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",
@@ -1221,7 +1221,7 @@ check_default_partition_contents(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,
+ if (ConstraintImpliedByRelConstraint(part_rel,
def_part_constraints))
{
ereport(INFO,
diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h
index ec3bb90b01..759abba3f2 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 4db792cf2f..c8de23cb19 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -1065,6 +1065,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 d80643037d..94c6a43a10 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -809,6 +809,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);
0002_alter_table_set_not_null_v11_debuglevel_tests.patchtext/x-diff; name=0002_alter_table_set_not_null_v11_debuglevel_tests.patchDownload
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index c8de23cb19..efa8126962 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -1028,6 +1028,8 @@ insert into atacc1 (test2, test) values (1, NULL);
ERROR: null value in column "test" violates not-null constraint
DETAIL: Failing row contains (null, 1).
drop table atacc1;
+-- we want check if not null was implied by constraint
+set client_min_messages to 'debug1';
-- alter table / alter column [set/drop] not null tests
-- try altering system catalogs, should fail
alter table pg_class alter column relname drop not null;
@@ -1043,15 +1045,19 @@ ERROR: relation "non_existent" does not exist
-- test checking for null values and primary key
create table atacc1 (test int not null);
alter table atacc1 add constraint "atacc1_pkey" primary key (test);
+DEBUG: ALTER TABLE / ADD PRIMARY KEY will create implicit index "atacc1_pkey" for table "atacc1"
+DEBUG: building index "atacc1_pkey" on table "atacc1" serially
alter table atacc1 alter column test drop not null;
ERROR: column "test" is in a primary key
alter table atacc1 drop constraint "atacc1_pkey";
alter table atacc1 alter column test drop not null;
insert into atacc1 values (null);
alter table atacc1 alter test set not null;
+DEBUG: verifying table "atacc1"
ERROR: column "test" contains null values
delete from atacc1;
alter table atacc1 alter test set not null;
+DEBUG: verifying table "atacc1"
-- 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
@@ -1070,36 +1076,49 @@ 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);
+DEBUG: verifying table "atacc1"
alter table atacc1 alter test_a set not null;
+DEBUG: verifying table "atacc1"
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;
+DEBUG: verifying table "atacc1"
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);
+DEBUG: verifying table "atacc1"
alter table atacc1 alter test_a set not null;
+DEBUG: verifying table "atacc1" NOT NULL constraint on test_a attribute by existed constraints
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;
+DEBUG: verifying table "atacc1" NOT NULL constraint on test_a attribute by existed constraints
+DEBUG: verifying table "atacc1"
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;
+DEBUG: verifying table "atacc1"
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;
+DEBUG: verifying table "atacc1"
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);
+DEBUG: verifying table "atacc1"
alter table atacc1 alter test_b set not null, alter test_a set not null;
+DEBUG: verifying table "atacc1" NOT NULL constraint on test_b attribute by existed constraints
+DEBUG: verifying table "atacc1" NOT NULL constraint on test_a attribute by existed constraints
drop table atacc1;
+reset client_min_messages;
-- 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 94c6a43a10..aa678ff6b8 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -776,6 +776,9 @@ insert into atacc1 (test2, test) values (2, 3);
insert into atacc1 (test2, test) values (1, NULL);
drop table atacc1;
+-- we want check if not null was implied by constraint
+set client_min_messages to 'debug1';
+
-- alter table / alter column [set/drop] not null tests
-- try altering system catalogs, should fail
alter table pg_class alter column relname drop not null;
@@ -844,6 +847,8 @@ 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;
+reset client_min_messages;
+
-- test inheritance
create table parent (a int);
create table child (b varchar(255)) inherits (parent);
On Wed, 6 Mar 2019 at 08:41, Sergei Kornilov <sk@zsrv.org> wrote:
In this case we need extra argument for ConstraintImpliedByRelConstraint for some caller-provided existConstraint, right? Along with Relation itself? Then I need make copy of existConstraint, append relation constraints and call predicate_implied_by. If I understood correctly pseudocode would be:
PartConstraintImpliedByRelConstraint(rel, testConstraint)
generate notNullConstraint from NOT NULL table attributes
call ConstraintImpliedByRelConstraint(rel, testConstraint, notNullConstraint)ConstraintImpliedByRelConstraint(rel, testConstraint, existsConstraint)
copy existsConstraint to localExistsConstraint variable
append relation constraints to localExistsConstraint list
call predicate_implied_byI thing redundant IS NOT NULL check is not issue here and we not need extra arguments for ConstraintImpliedByRelConstraint. Was not changed in attached version, but I will change if you think this would be better design.
I was really just throwing the idea out there for review. I don't
think that I'm insisting on it.
FWIW I think you could get away without the copy of the constraint
providing it was documented that the list is modified during the
ConstraintImpliedByRelConstraint call and callers must make a copy
themselves if they need an unmodified version. Certainly,
PartConstraintImpliedByRelConstraint won't need to make a copy, so
there's probably not much reason to assume that possible future
callers will.
Providing I'm imagining it correctly, I do think the patch is slightly
cleaner with that change.
It's:
a) slightly more efficient due to not needlessly checking a bunch of
IS NOT NULLs (imagine a 1000 column table with almost all NOT NULL and
a single CHECK constraint); and
b) patch has a smaller footprint (does not modify existing callers of
PartConstraintImpliedByRelConstraint()); and
c) does not modify an existing API function.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Hi
Providing I'm imagining it correctly, I do think the patch is slightly
cleaner with that change.
Ok, sounds reasonable. I changed patch this way.
regards, Sergei
Attachments:
0001_alter_table_set_not_null_v11.5.patchtext/x-diff; name=0001_alter_table_set_not_null_v11.5.patchDownload
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 890b23afd6..926b3361ea 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -214,8 +214,17 @@ 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>
+ <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 59341e2a40..5200aac530 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,9 @@ 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 bool ConstraintImpliedByRelConstraint(Relation scanrel,
+ List *partConstraint, List *existedConstraints);
static ObjectAddress ATExecColumnDefault(Relation rel, const char *colName,
Node *newDefault, LOCKMODE lockmode);
static ObjectAddress ATExecAddIdentity(Relation rel, const char *colName,
@@ -4550,10 +4553,11 @@ 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->new_notnull ||
+ if (tab->constraints != NIL || tab->verify_new_notnull ||
tab->partition_constraint != NULL)
ATRewriteTable(tab, InvalidOid, lockmode);
@@ -4714,13 +4718,13 @@ 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
- * 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++)
{
@@ -5726,11 +5730,9 @@ 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->new_notnull |= colDef->is_not_null;
+ tab->verify_new_notnull |= colDef->is_not_null;
}
}
@@ -6098,8 +6100,19 @@ 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;
+ /*
+ * 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 (!tab->verify_new_notnull &&
+ !NotNullImpliedByRelConstraints(rel, (Form_pg_attribute) GETSTRUCT(tuple)))
+ {
+ /* Tell Phase 3 it needs to test the constraint */
+ tab->verify_new_notnull = true;
+ }
ObjectAddressSubSet(address, RelationRelationId,
RelationGetRelid(rel), attnum);
@@ -6115,6 +6128,43 @@ ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
return address;
}
+/*
+ * NotNullImpliedByRelConstraints
+ * Does rel's existing constraints imply NOT NULL for the given attribute
+ */
+static bool
+NotNullImpliedByRelConstraints(Relation rel, Form_pg_attribute attr)
+{
+ NullTest *nnulltest = 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;
+
+ if (ConstraintImpliedByRelConstraint(rel, list_make1(nnulltest), NIL))
+ {
+ 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
*
@@ -14387,7 +14437,8 @@ ComputePartitionAttrs(ParseState *pstate, Relation rel, List *partParams, AttrNu
*
* "Existing constraints" include its check constraints and column-level
* NOT NULL constraints. partConstraint describes the partition constraint,
- * in implicit-AND form.
+ * in implicitly-AND form, must only contain immutable clauses
+ * and all Vars must be varno=1.
*/
bool
PartConstraintImpliedByRelConstraint(Relation scanrel,
@@ -14395,8 +14446,7 @@ PartConstraintImpliedByRelConstraint(Relation scanrel,
{
List *existConstraint = NIL;
TupleConstr *constr = RelationGetDescr(scanrel)->constr;
- int num_check,
- i;
+ int i;
if (constr && constr->has_not_null)
{
@@ -14430,6 +14480,27 @@ PartConstraintImpliedByRelConstraint(Relation scanrel,
}
}
+ return ConstraintImpliedByRelConstraint(scanrel, partConstraint, existConstraint);
+}
+
+/*
+ * ConstraintImpliedByRelConstraint
+ * Do scanrel's existing constraints imply the given constraint?
+ *
+ * "Existing constraints" include its check constraints and optional
+ * caller-provided existConstraint list. existConstraint list is modified
+ * during ConstraintImpliedByRelConstraint call and would represent all
+ * assumed conditions. testConstraint describes the constraint to validate.
+ * Both existConstraint and testConstraint must be in implicitly-AND form,
+ * must only contain immutable clauses and all Vars must be varno=1.
+ */
+bool
+ConstraintImpliedByRelConstraint(Relation scanrel, List *testConstraint, List *existConstraint)
+{
+ TupleConstr *constr = RelationGetDescr(scanrel)->constr;
+ int num_check,
+ i;
+
num_check = (constr != NULL) ? constr->num_check : 0;
for (i = 0; i < num_check; i++)
{
@@ -14460,13 +14531,9 @@ 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.
- *
- * 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.
+ * not-false and try to prove the same for testConstraint.
*/
- return predicate_implied_by(partConstraint, existConstraint, true);
+ return predicate_implied_by(testConstraint, existConstraint, true);
}
/*
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index fd14c4b4f2..a76bc0dd6a 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -1065,6 +1065,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 298bde179b..5bda7febde 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -809,6 +809,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);
0002_alter_table_set_not_null_v11_debuglevel_tests.patchtext/x-diff; name=0002_alter_table_set_not_null_v11_debuglevel_tests.patchDownload
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index c8de23cb19..efa8126962 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -1028,6 +1028,8 @@ insert into atacc1 (test2, test) values (1, NULL);
ERROR: null value in column "test" violates not-null constraint
DETAIL: Failing row contains (null, 1).
drop table atacc1;
+-- we want check if not null was implied by constraint
+set client_min_messages to 'debug1';
-- alter table / alter column [set/drop] not null tests
-- try altering system catalogs, should fail
alter table pg_class alter column relname drop not null;
@@ -1043,15 +1045,19 @@ ERROR: relation "non_existent" does not exist
-- test checking for null values and primary key
create table atacc1 (test int not null);
alter table atacc1 add constraint "atacc1_pkey" primary key (test);
+DEBUG: ALTER TABLE / ADD PRIMARY KEY will create implicit index "atacc1_pkey" for table "atacc1"
+DEBUG: building index "atacc1_pkey" on table "atacc1" serially
alter table atacc1 alter column test drop not null;
ERROR: column "test" is in a primary key
alter table atacc1 drop constraint "atacc1_pkey";
alter table atacc1 alter column test drop not null;
insert into atacc1 values (null);
alter table atacc1 alter test set not null;
+DEBUG: verifying table "atacc1"
ERROR: column "test" contains null values
delete from atacc1;
alter table atacc1 alter test set not null;
+DEBUG: verifying table "atacc1"
-- 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
@@ -1070,36 +1076,49 @@ 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);
+DEBUG: verifying table "atacc1"
alter table atacc1 alter test_a set not null;
+DEBUG: verifying table "atacc1"
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;
+DEBUG: verifying table "atacc1"
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);
+DEBUG: verifying table "atacc1"
alter table atacc1 alter test_a set not null;
+DEBUG: verifying table "atacc1" NOT NULL constraint on test_a attribute by existed constraints
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;
+DEBUG: verifying table "atacc1" NOT NULL constraint on test_a attribute by existed constraints
+DEBUG: verifying table "atacc1"
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;
+DEBUG: verifying table "atacc1"
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;
+DEBUG: verifying table "atacc1"
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);
+DEBUG: verifying table "atacc1"
alter table atacc1 alter test_b set not null, alter test_a set not null;
+DEBUG: verifying table "atacc1" NOT NULL constraint on test_b attribute by existed constraints
+DEBUG: verifying table "atacc1" NOT NULL constraint on test_a attribute by existed constraints
drop table atacc1;
+reset client_min_messages;
-- 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 94c6a43a10..aa678ff6b8 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -776,6 +776,9 @@ insert into atacc1 (test2, test) values (2, 3);
insert into atacc1 (test2, test) values (1, NULL);
drop table atacc1;
+-- we want check if not null was implied by constraint
+set client_min_messages to 'debug1';
+
-- alter table / alter column [set/drop] not null tests
-- try altering system catalogs, should fail
alter table pg_class alter column relname drop not null;
@@ -844,6 +847,8 @@ 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;
+reset client_min_messages;
+
-- test inheritance
create table parent (a int);
create table child (b varchar(255)) inherits (parent);
On Mon, 11 Mar 2019 at 00:13, Sergei Kornilov <sk@zsrv.org> wrote:
Providing I'm imagining it correctly, I do think the patch is slightly
cleaner with that change.Ok, sounds reasonable. I changed patch this way.
Looks good to me. Good idea to keep the controversial setting of
client_min_messages to debug1 in the tests in a separate patch.
Apart from a few lines that need to be wrapped at 80 chars and some
comment lines that seem to have been wrapped early, I'm happy for it
to be marked as ready for committer, but I'll defer to Ildar in case
he wants to have another look.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On Sun, Mar 10, 2019 at 11:30 PM David Rowley
<david.rowley@2ndquadrant.com> wrote:
Looks good to me. Good idea to keep the controversial setting of
client_min_messages to debug1 in the tests in a separate patch.Apart from a few lines that need to be wrapped at 80 chars and some
comment lines that seem to have been wrapped early, I'm happy for it
to be marked as ready for committer, but I'll defer to Ildar in case
he wants to have another look.
Dispatches from the department of grammatical nitpicking...
+ entire table, however if a valid <literal>CHECK</literal> constraint is
I think this should be:
entire table; however, if...
+ * are set NOT NULL, however, if we can find a constraint which proves
similarly here
+ ereport(DEBUG1,
+ (errmsg("verifying table \"%s\" NOT NULL constraint "
+ "on %s attribute by existed constraints",
+ RelationGetRelationName(rel), NameStr(attr->attname))));Ugh, that doesn't read well at all. How about:
existing constraints on column "%s"."%s" are sufficient to prove that
it does not contain nulls
- * in implicit-AND form.
+ * in implicitly-AND form, must only contain immutable clauses
+ * and all Vars must be varno=1.I think you should leave the existing sentence alone (implicit-AND is
correct, implicitly-AND is not) and add a new sentence that says the
stuff you want to add.
+ * "Existing constraints" include its check constraints and optional
+ * caller-provided existConstraint list. existConstraint list is modified
+ * during ConstraintImpliedByRelConstraint call and would represent all
+ * assumed conditions. testConstraint describes the constraint to validate.
+ * Both existConstraint and testConstraint must be in implicitly-AND form,
+ * must only contain immutable clauses and all Vars must be varno=1.I think that it might be better to copy the list rather than to have
the comment note that it gets mutated, but regardless the grammar
needs improvement here. Maybe: On entry, existConstraint is a
caller-provided list of conditions which this function may assume to
be true; on exit, it will have been destructively modified by the
addition of the table's CHECK constraints. testConstraint is the
constraint to validate. Both existConstraint and testConstraint must
be in implicit-AND form, must only contain immutable clauses, and must
contain only Vars with varno = 1.
- * not-false and try to prove the same for partConstraint.
- *
- * 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.
+ * not-false and try to prove the same for testConstraint.I object to removing this.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Hello
Dispatches from the department of grammatical nitpicking...
Thank you!
+ entire table, however if a valid <literal>CHECK</literal> constraint is
I think this should be:
entire table; however, if...
+ * are set NOT NULL, however, if we can find a constraint which proves
similarly here
Changed
+ ereport(DEBUG1, + (errmsg("verifying table \"%s\" NOT NULL constraint " + "on %s attribute by existed constraints", + RelationGetRelationName(rel), NameStr(attr->attname))));Ugh, that doesn't read well at all. How about:
existing constraints on column "%s"."%s" are sufficient to prove that
it does not contain nulls
Changed
- * in implicit-AND form. + * in implicitly-AND form, must only contain immutable clauses + * and all Vars must be varno=1.I think you should leave the existing sentence alone (implicit-AND is
correct, implicitly-AND is not) and add a new sentence that says the
stuff you want to add.
Ok
+ * "Existing constraints" include its check constraints and optional + * caller-provided existConstraint list. existConstraint list is modified + * during ConstraintImpliedByRelConstraint call and would represent all + * assumed conditions. testConstraint describes the constraint to validate. + * Both existConstraint and testConstraint must be in implicitly-AND form, + * must only contain immutable clauses and all Vars must be varno=1.I think that it might be better to copy the list rather than to have
the comment note that it gets mutated, but regardless the grammar
needs improvement here.
Agreed, in attached new version I copy the list and do not modify parameter.
I object to removing this.
Okay, I revert this David's change
regards, Sergei
Attachments:
0001_alter_table_set_not_null_v12.patchtext/x-diff; name=0001_alter_table_set_not_null_v12.patchDownload
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 890b23afd6..cda0ea8972 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -214,8 +214,17 @@ 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>
+ <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 5ed560b02f..15407778b4 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,9 @@ 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 bool ConstraintImpliedByRelConstraint(Relation scanrel,
+ List *partConstraint, List *existedConstraints);
static ObjectAddress ATExecColumnDefault(Relation rel, const char *colName,
Node *newDefault, LOCKMODE lockmode);
static ObjectAddress ATExecAddIdentity(Relation rel, const char *colName,
@@ -4550,10 +4553,11 @@ 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->new_notnull ||
+ if (tab->constraints != NIL || tab->verify_new_notnull ||
tab->partition_constraint != NULL)
ATRewriteTable(tab, InvalidOid, lockmode);
@@ -4714,13 +4718,13 @@ 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
- * 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++)
{
@@ -5749,11 +5753,9 @@ 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->new_notnull |= colDef->is_not_null;
+ tab->verify_new_notnull |= colDef->is_not_null;
}
}
@@ -6121,8 +6123,19 @@ 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;
+ /*
+ * 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 (!tab->verify_new_notnull &&
+ !NotNullImpliedByRelConstraints(rel, (Form_pg_attribute) GETSTRUCT(tuple)))
+ {
+ /* Tell Phase 3 it needs to test the constraint */
+ tab->verify_new_notnull = true;
+ }
ObjectAddressSubSet(address, RelationRelationId,
RelationGetRelid(rel), attnum);
@@ -6138,6 +6151,43 @@ ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
return address;
}
+/*
+ * NotNullImpliedByRelConstraints
+ * Does rel's existing constraints imply NOT NULL for the given attribute
+ */
+static bool
+NotNullImpliedByRelConstraints(Relation rel, Form_pg_attribute attr)
+{
+ NullTest *nnulltest = 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;
+
+ if (ConstraintImpliedByRelConstraint(rel, list_make1(nnulltest), NIL))
+ {
+ ereport(DEBUG1,
+ (errmsg("existing constraints on column \"%s\".\"%s\" are"
+ " sufficient to prove that it does not contain nulls",
+ RelationGetRelationName(rel), NameStr(attr->attname))));
+ return true;
+ }
+
+ return false;
+}
+
/*
* ALTER TABLE ALTER COLUMN SET/DROP DEFAULT
*
@@ -14416,8 +14466,7 @@ PartConstraintImpliedByRelConstraint(Relation scanrel,
{
List *existConstraint = NIL;
TupleConstr *constr = RelationGetDescr(scanrel)->constr;
- int num_check,
- i;
+ int i;
if (constr && constr->has_not_null)
{
@@ -14451,6 +14500,27 @@ PartConstraintImpliedByRelConstraint(Relation scanrel,
}
}
+ return ConstraintImpliedByRelConstraint(scanrel, partConstraint, existConstraint);
+}
+
+/*
+ * ConstraintImpliedByRelConstraint
+ * Do scanrel's existing constraints imply the given constraint?
+ *
+ * testConstraint is the constraint to validate. provenConstraint is a
+ * caller-provided list of conditions which this function may assume
+ * to be true. Both provenConstraint and testConstraint must be in
+ * implicit-AND form, must only contain immutable clauses, and must
+ * contain only Vars with varno = 1.
+ */
+bool
+ConstraintImpliedByRelConstraint(Relation scanrel, List *testConstraint, List *provenConstraint)
+{
+ List *existConstraint = list_copy(provenConstraint);
+ TupleConstr *constr = RelationGetDescr(scanrel)->constr;
+ int num_check,
+ i;
+
num_check = (constr != NULL) ? constr->num_check : 0;
for (i = 0; i < num_check; i++)
{
@@ -14481,13 +14551,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);
}
/*
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index fd14c4b4f2..a76bc0dd6a 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -1065,6 +1065,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 298bde179b..5bda7febde 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -809,6 +809,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);
0002_alter_table_set_not_null_v12_debuglevel_tests.patchtext/x-diff; name=0002_alter_table_set_not_null_v12_debuglevel_tests.patchDownload
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index a76bc0dd6a..d0dc671c47 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -1028,6 +1028,8 @@ insert into atacc1 (test2, test) values (1, NULL);
ERROR: null value in column "test" violates not-null constraint
DETAIL: Failing row contains (null, 1).
drop table atacc1;
+-- we want check if not null was implied by constraint
+set client_min_messages to 'debug1';
-- alter table / alter column [set/drop] not null tests
-- try altering system catalogs, should fail
alter table pg_class alter column relname drop not null;
@@ -1043,15 +1045,19 @@ ERROR: relation "non_existent" does not exist
-- test checking for null values and primary key
create table atacc1 (test int not null);
alter table atacc1 add constraint "atacc1_pkey" primary key (test);
+DEBUG: ALTER TABLE / ADD PRIMARY KEY will create implicit index "atacc1_pkey" for table "atacc1"
+DEBUG: building index "atacc1_pkey" on table "atacc1" serially
alter table atacc1 alter column test drop not null;
ERROR: column "test" is in a primary key
alter table atacc1 drop constraint "atacc1_pkey";
alter table atacc1 alter column test drop not null;
insert into atacc1 values (null);
alter table atacc1 alter test set not null;
+DEBUG: verifying table "atacc1"
ERROR: column "test" contains null values
delete from atacc1;
alter table atacc1 alter test set not null;
+DEBUG: verifying table "atacc1"
-- 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
@@ -1070,36 +1076,49 @@ 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);
+DEBUG: verifying table "atacc1"
alter table atacc1 alter test_a set not null;
+DEBUG: verifying table "atacc1"
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;
+DEBUG: verifying table "atacc1"
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);
+DEBUG: verifying table "atacc1"
alter table atacc1 alter test_a set not null;
+DEBUG: existing constraints on column "atacc1"."test_a" are sufficient to prove that it does not contain nulls
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;
+DEBUG: existing constraints on column "atacc1"."test_a" are sufficient to prove that it does not contain nulls
+DEBUG: verifying table "atacc1"
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;
+DEBUG: verifying table "atacc1"
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;
+DEBUG: verifying table "atacc1"
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);
+DEBUG: verifying table "atacc1"
alter table atacc1 alter test_b set not null, alter test_a set not null;
+DEBUG: existing constraints on column "atacc1"."test_b" are sufficient to prove that it does not contain nulls
+DEBUG: existing constraints on column "atacc1"."test_a" are sufficient to prove that it does not contain nulls
drop table atacc1;
+reset client_min_messages;
-- 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 5bda7febde..b8714e27d3 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -776,6 +776,9 @@ insert into atacc1 (test2, test) values (2, 3);
insert into atacc1 (test2, test) values (1, NULL);
drop table atacc1;
+-- we want check if not null was implied by constraint
+set client_min_messages to 'debug1';
+
-- alter table / alter column [set/drop] not null tests
-- try altering system catalogs, should fail
alter table pg_class alter column relname drop not null;
@@ -844,6 +847,8 @@ 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;
+reset client_min_messages;
+
-- test inheritance
create table parent (a int);
create table child (b varchar(255)) inherits (parent);
On Tue, Mar 12, 2019 at 4:27 PM Sergei Kornilov <sk@zsrv.org> wrote:
Agreed, in attached new version ...
Committed with a little more nitpicking.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Wow, thank you!
13.03.2019, 15:57, "Robert Haas" <robertmhaas@gmail.com>:
Show quoted text
On Tue, Mar 12, 2019 at 4:27 PM Sergei Kornilov <sk@zsrv.org> wrote:
Agreed, in attached new version ...
Committed with a little more nitpicking.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
On Tue, Mar 12, 2019 at 4:27 PM Sergei Kornilov <sk@zsrv.org> wrote:
Agreed, in attached new version ...
Committed with a little more nitpicking.
The buildfarm thinks additional nitpicking is needed.
regards, tom lane
Hi
The buildfarm thinks additional nitpicking is needed.
hm. Patch was committed with debug1 level tests and many animals uses log_statement = 'all'. Therefore they have additional line in result: LOG: statement: alter table pg_class alter column relname drop not null; and similar for other queries.
I think we better would be to revert "set client_min_messages to 'debug1';" part.
regards, Sergei
On Wed, Mar 13, 2019 at 11:17 AM Sergei Kornilov <sk@zsrv.org> wrote:
The buildfarm thinks additional nitpicking is needed.
hm. Patch was committed with debug1 level tests and many animals uses log_statement = 'all'. Therefore they have additional line in result: LOG: statement: alter table pg_class alter column relname drop not null; and similar for other queries.
I think we better would be to revert "set client_min_messages to 'debug1';" part.
Ugh, I guess so. Or how about changing the message itself to use
INFO, like we already do in QueuePartitionConstraintValidation?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Hi
Ugh, I guess so. Or how about changing the message itself to use
INFO, like we already do in QueuePartitionConstraintValidation?
Fine for me. But year ago this was implemented in my patch and Tom voted against using INFO level for such purpose: /messages/by-id/1142.1520362313@sss.pgh.pa.us
regards, Sergei
Sergei Kornilov <sk@zsrv.org> writes:
Ugh, I guess so. Or how about changing the message itself to use
INFO, like we already do in QueuePartitionConstraintValidation?
Fine for me. But year ago this was implemented in my patch and Tom voted against using INFO level for such purpose: /messages/by-id/1142.1520362313@sss.pgh.pa.us
What I thought then was that you didn't need the message at all,
at any debug level. I still think that. It might have been useful
for development purposes but it does not belong in committed code.
INFO (making it impossible for anybody to not have the message
in-their-face) is right out.
regards, tom lane
I wrote:
Sergei Kornilov <sk@zsrv.org> writes:
Ugh, I guess so. Or how about changing the message itself to use
INFO, like we already do in QueuePartitionConstraintValidation?
Fine for me. But year ago this was implemented in my patch and Tom voted against using INFO level for such purpose: /messages/by-id/1142.1520362313@sss.pgh.pa.us
What I thought then was that you didn't need the message at all,
at any debug level. I still think that.
Oh, and yes, I think QueuePartitionConstraintValidation's usage
is an unacceptable abuse of INFO level. I'm surprised we haven't
gotten complaints about it yet.
regards, tom lane
On Wed, Mar 13, 2019 at 1:09 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Sergei Kornilov <sk@zsrv.org> writes:
Ugh, I guess so. Or how about changing the message itself to use
INFO, like we already do in QueuePartitionConstraintValidation?Fine for me. But year ago this was implemented in my patch and Tom voted against using INFO level for such purpose: /messages/by-id/1142.1520362313@sss.pgh.pa.us
What I thought then was that you didn't need the message at all,
at any debug level. I still think that. It might have been useful
for development purposes but it does not belong in committed code.
INFO (making it impossible for anybody to not have the message
in-their-face) is right out.
I find that position entirely wrong-headed. If you think users have
no interest in a message telling them whether their gigantic table is
getting scanned or not, you're wrong. Maybe you're right that
everyone doesn't want it, but I'm positive some do. We've had
requests for very similar things on this very mailing list more than
one.
And if you think that it's not useful to have regression tests that
verify that the behavior is correct, well, that's not a question with
one objectively right answer, but I emphatically disagree with that
position. This behavior is often quite subtle, especially in
combination with partitioned tables where different partitions may get
different treatment. It would not be at all difficult to break it
without realizing.
I do understand that we seem to have no good way of doing this that
lets users have the option of seeing this message and also makes it
something that can be regression-tested. INFO is out because there's
no option, and DEBUG1 is out because it doesn't work in the regression
tests. However, I think giving up and saying "ok, well that's just
how it is, too bad" is, one, letting the tail wag the dog, and two, a
terribly disappointing attitude.
I've just reverted the 0002 portion of the patch, which should
hopefully fix this problem for now. But I strongly encourage you
think of something to which you could say "yes" instead of just
shooting everything people want to do in this area down.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Wed, Mar 13, 2019 at 1:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Oh, and yes, I think QueuePartitionConstraintValidation's usage
is an unacceptable abuse of INFO level. I'm surprised we haven't
gotten complaints about it yet.
Perhaps that's because users aren't as direly opposed to informational
messages from DDL commands as you seem to believe...
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Hi,
On Wed, Mar 13, 2019 at 01:25:11PM -0400, Robert Haas wrote:
On Wed, Mar 13, 2019 at 1:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Oh, and yes, I think QueuePartitionConstraintValidation's usage
is an unacceptable abuse of INFO level. I'm surprised we haven't
gotten complaints about it yet.Perhaps that's because users aren't as direly opposed to informational
messages from DDL commands as you seem to believe...
On Wed, Mar 13, 2019 at 01:09:32PM -0400, Tom Lane wrote:
Sergei Kornilov <sk@zsrv.org> writes:
Ugh, I guess so. Or how about changing the message itself to use
INFO, like we already do in QueuePartitionConstraintValidation?Fine for me. But year ago this was implemented in my patch and Tom voted against using INFO level for such purpose: /messages/by-id/1142.1520362313@sss.pgh.pa.us
What I thought then was that you didn't need the message at all,
at any debug level. I still think that. It might have been useful
for development purposes but it does not belong in committed code.
INFO (making it impossible for anybody to not have the message
in-their-face) is right out.
I'm writing to provide a datapoint for the existing message regarding "implied
by existing constraints" in QueuePartitionConstraintValidation.
On the one hand, I agree that message at INFO is more verbose than other
commands, and I'm 20% surprised by it.
On the other hand, I *like* the output (including at INFO level). It alerted
me to 1) deficiency with some of our tables (due to column not marked NOT
NULL); and, 2) helped me to understand how to improve some of our dynamic DDL.
If the message weren't visible at LOG, I'd miss it, at least sometimes, and
look for how to re-enable it (but statement logging of DEBUG is beyond
excessive).
For context, I'm someone who configures servers with log_min_messages=info and
who sometimes SETs client_min_messages='DEBUG1' ; I have $toomany warnings in
our monitoring system, because I'd rather see twice as much stuff that may or
may not be interesting than miss 10% of things I needed to see.
Justin
Import Notes
Reply to msg id not found: CA+TgmoZMDn_9xHBeJVSsgLrRpDk85y_Gpccts9x1CHaSb3DKA@mail.gmail.com7254.1552496972@sss.pgh.pa.us | Resolved by subject fallback
On Thu, 14 Mar 2019 at 06:24, Robert Haas <robertmhaas@gmail.com> wrote:
On Wed, Mar 13, 2019 at 1:09 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Sergei Kornilov <sk@zsrv.org> writes:
Ugh, I guess so. Or how about changing the message itself to use
INFO, like we already do in QueuePartitionConstraintValidation?Fine for me. But year ago this was implemented in my patch and Tom voted against using INFO level for such purpose: /messages/by-id/1142.1520362313@sss.pgh.pa.us
What I thought then was that you didn't need the message at all,
at any debug level. I still think that. It might have been useful
for development purposes but it does not belong in committed code.
INFO (making it impossible for anybody to not have the message
in-their-face) is right out.I find that position entirely wrong-headed. If you think users have
no interest in a message telling them whether their gigantic table is
getting scanned or not, you're wrong. Maybe you're right that
everyone doesn't want it, but I'm positive some do. We've had
requests for very similar things on this very mailing list more than
one.
If we don't have this for SET NOT NULL then we should either add it or
remove the one from ATTACH PARTITION. I don't think we need to decide
which it is now, so I've added an item to the open items list that
this is out for debate.
https://wiki.postgresql.org/wiki/PostgreSQL_12_Open_Items#Open_Issues
I think we can mark this patch as committed now as I don't think the
lack of a way to test it is likely to cause it to be reverted.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Hello
If we don't have this for SET NOT NULL then we should either add it or
remove the one from ATTACH PARTITION.
I proposed to lower report level to debug1 in a separate thread: /messages/by-id/4859321552643736@myt5-02b80404fd9e.qloud-c.yandex.net
Possible better would be link it.
regards, Sergei
Hi Robert,
On 3/17/19 3:40 PM, David Rowley wrote:
On Thu, 14 Mar 2019 at 06:24, Robert Haas <robertmhaas@gmail.com> wrote:
I think we can mark this patch as committed now as I don't think the
lack of a way to test it is likely to cause it to be reverted.
Should we mark this as committed now or is there more tweaking to be done?
Regards,
--
-David
david@pgmasters.net
On Mon, Mar 25, 2019 at 3:51 AM David Steele <david@pgmasters.net> wrote:
On 3/17/19 3:40 PM, David Rowley wrote:
On Thu, 14 Mar 2019 at 06:24, Robert Haas <robertmhaas@gmail.com> wrote:
I think we can mark this patch as committed now as I don't think the
lack of a way to test it is likely to cause it to be reverted.Should we mark this as committed now or is there more tweaking to be done?
I have now marked it as Committed.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 2019-03-25 11:09:47 -0400, Robert Haas wrote:
On Mon, Mar 25, 2019 at 3:51 AM David Steele <david@pgmasters.net> wrote:
On 3/17/19 3:40 PM, David Rowley wrote:
On Thu, 14 Mar 2019 at 06:24, Robert Haas <robertmhaas@gmail.com> wrote:
I think we can mark this patch as committed now as I don't think the
lack of a way to test it is likely to cause it to be reverted.Should we mark this as committed now or is there more tweaking to be done?
I have now marked it as Committed.
There's still an open item "Debate INFO messages in ATTACH PARTITION and
SET NOT NULL" for this thread. Is there anything further to be done? Or
are we content with this for v12?
- Andres
Andres Freund <andres@anarazel.de> writes:
There's still an open item "Debate INFO messages in ATTACH PARTITION and
SET NOT NULL" for this thread. Is there anything further to be done? Or
are we content with this for v12?
IIRC, that's based on my complaint that these messages have no business
being INFO level. I'm definitely not content with the current state.
I proposed that we didn't need those messages at all, which Robert pushed
back on, but I'd be willing to compromise by reducing them to NOTICE or
DEBUG level.
The actually intended use of INFO level is to mark messages that the user
actively asked for (via VERBOSE or some morally-equivalent option), which
is why that level has such high priority. If we had something like a
VERBOSE option for ALTER TABLE, it'd make plenty of sense to follow
VACUUM's precedent:
if (params->options & VACOPT_VERBOSE)
elevel = INFO;
else
elevel = DEBUG2;
It seems a bit late to be inventing such a thing for v12 though.
In the meantime, I'm not very content with random subsets of
ALTER TABLE acting as if they have been granted permission
to be VERBOSE. It's unfriendly, and it's inconsistent because
there are so many other expensive ALTER TABLE actions that don't
do this.
regards, tom lane
Hi
I proposed that we didn't need those messages at all, which Robert pushed
back on, but I'd be willing to compromise by reducing them to NOTICE or
DEBUG level.
I posted patch with such change in a separate topic: https://commitfest.postgresql.org/23/2076/
PS: not think this is blocker for v12
regards, Sergei
On Thu, 2 May 2019 at 06:18, Sergei Kornilov <sk@zsrv.org> wrote:
I proposed that we didn't need those messages at all, which Robert pushed
back on, but I'd be willing to compromise by reducing them to NOTICE or
DEBUG level.I posted patch with such change in a separate topic: https://commitfest.postgresql.org/23/2076/
PS: not think this is blocker for v12
Probably not a blocker, but now does not seem like an unreasonable
time to lower these INFO messages down to DEBUG. If not, then we can
just remove the item from the open items list. I just put it there as
I didn't want it getting forgotten about as it wasn't going to be
anybody's priority to think hard about it on the final few days of the
last commitfest of the release.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
David Rowley <david.rowley@2ndquadrant.com> writes:
On Thu, 2 May 2019 at 06:18, Sergei Kornilov <sk@zsrv.org> wrote:
PS: not think this is blocker for v12
Probably not a blocker, but now does not seem like an unreasonable
time to lower these INFO messages down to DEBUG.
Not a blocker perhaps, but it's better if we can get new behavior to
be more or less right the first time.
regards, tom lane
On Thu, 2 May 2019 at 13:08, Tom Lane <tgl@sss.pgh.pa.us> wrote:
David Rowley <david.rowley@2ndquadrant.com> writes:
On Thu, 2 May 2019 at 06:18, Sergei Kornilov <sk@zsrv.org> wrote:
PS: not think this is blocker for v12
Probably not a blocker, but now does not seem like an unreasonable
time to lower these INFO messages down to DEBUG.Not a blocker perhaps, but it's better if we can get new behavior to
be more or less right the first time.
It's not really new behaviour though. The code in question is for
ATTACH PARTITION and was added in c31e9d4bafd (back in 2017).
bbb96c3704c is the commit for using constraints to speed up SET NOT
NULL. The mention of using the constraint for proof was made DEBUG1 in
the initial commit. What we need to decide is if we want to make
ATTACH PARTITION's INFO message a DEBUG1 in pg12 or not.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On Thu, 2 May 2019 at 14:22, David Rowley <david.rowley@2ndquadrant.com> wrote:
On Thu, 2 May 2019 at 13:08, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Not a blocker perhaps, but it's better if we can get new behavior to
be more or less right the first time.It's not really new behaviour though. The code in question is for
ATTACH PARTITION and was added in c31e9d4bafd (back in 2017).
bbb96c3704c is the commit for using constraints to speed up SET NOT
NULL. The mention of using the constraint for proof was made DEBUG1 in
the initial commit. What we need to decide is if we want to make
ATTACH PARTITION's INFO message a DEBUG1 in pg12 or not.
FWIW, I'm in favour of changing ATTACH PARTITION's INFO messages to
DEBUG1 for PG12 to align it to what bbb96c3704c did.
Anyone else?
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On Fri, 3 May 2019 at 19:06, David Rowley <david.rowley@2ndquadrant.com> wrote:
FWIW, I'm in favour of changing ATTACH PARTITION's INFO messages to
DEBUG1 for PG12 to align it to what bbb96c3704c did.Anyone else?
Does anyone think we shouldn't change the INFO message in ATTACH
PARTITION to a DEBUG1 in PG12?
I think it makes sense make this change so that it matches the
behaviour of the output of ALTER TABLE .. ALTER COLUMN .. SET NOT NULL
when it uses a CHECK constraint.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Hello
Does anyone think we shouldn't change the INFO message in ATTACH
PARTITION to a DEBUG1 in PG12?
Seems no one wants to vote against this change.
My proposed patch was here: https://commitfest.postgresql.org/23/2076/
regards, Sergei
On Tue, 11 Jun 2019 at 03:35, Sergei Kornilov <sk@zsrv.org> wrote:
Does anyone think we shouldn't change the INFO message in ATTACH
PARTITION to a DEBUG1 in PG12?Seems no one wants to vote against this change.
I'm concerned about two things:
1. The patch reduces the test coverage of ATTACH PARTITION. We now
have no way to ensure the constraint was used to validate the rows in
the partition.
2. We're inconsistent with what we do for SET NOT NULL and ATTACH
PARTITION. We raise an INFO message when we use a constraint for
ATTACH PARTITION and only a DEBUG1 for SET NOT NULL.
Unfortunately, my two concerns conflict with each other.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On Wed, Jun 12, 2019 at 08:34:57AM +1200, David Rowley wrote:
On Tue, 11 Jun 2019 at 03:35, Sergei Kornilov <sk@zsrv.org> wrote:
Does anyone think we shouldn't change the INFO message in ATTACH
PARTITION to a DEBUG1 in PG12?Seems no one wants to vote against this change.
I'm concerned about two things:
1. The patch reduces the test coverage of ATTACH PARTITION. We now
have no way to ensure the constraint was used to validate the rows in
the partition.
2. We're inconsistent with what we do for SET NOT NULL and ATTACH
PARTITION. We raise an INFO message when we use a constraint for
ATTACH PARTITION and only a DEBUG1 for SET NOT NULL.Unfortunately, my two concerns conflict with each other.
We're getting close to beta3/rc1, and this thread was idle for ~1 month.
I think there's a consensus to change INFO to DEBUG1 in pg12, and then
maybe imlpement something like VERBOSE mode in the future. Objections?
As for the reduction of test coverage, can't we deduce whether a
constraint was used from data in pg_stats or something like that?
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
I think there's a consensus to change INFO to DEBUG1 in pg12, and then
maybe imlpement something like VERBOSE mode in the future. Objections?
ISTM the consensus is "we'd rather reduce the verbosity, but we don't
want to give up test coverage". So what's blocking this is lack of
a patch to show that there's another way to verify what code path
was taken.
As for the reduction of test coverage, can't we deduce whether a
constraint was used from data in pg_stats or something like that?
Not sure how exactly ... and we've already learned that pg_stats
isn't too reliable.
regards, tom lane