From: Sergei Kornilov <sk@zsrv.org>
To: Stephen Frost <sfrost@snowman.net>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: using index or check in ALTER TABLE SET NOT NULL
Date: Tue, 23 Jan 2018 22:06:07 +0300
Message-ID: <354171516734367@web46g.yandex.ru> (raw)
In-Reply-To: <20180123153711.GI2416@tamriel.snowman.net>
References: <81911511895540@web58j.yandex.ru>
<CA+TgmoZKR4dTR7KyoM4jfJN2J-eNoBRG2_XHdZa1f0VUbK1xXQ@mail.gmail.com>
<20171129152135.GN4628@tamriel.snowman.net>
<9878.1511970441@sss.pgh.pa.us>
<370481511970756@web3j.yandex.ru>
<491651511974381@web27j.yandex.ru>
<CA+TgmoahVfu0dUmvM1bO3csn=DtFp1fG92995rjYD9u_kO-Y1g@mail.gmail.com>
<369811512396584@web6o.yandex.ru>
<20180123153711.GI2416@tamriel.snowman.net>
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:
[text/x-diff] alter_table_set_not_null_by_constraints_only_v4.patch (13.2K, ../354171516734367@web46g.yandex.ru/2-alter_table_set_not_null_by_constraints_only_v4.patch)
download | inline diff:diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgmlindex 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.cindex 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.cindex 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.hindex 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.outindex 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.sqlindex 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);
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: sk@zsrv.org, sfrost@snowman.net, robertmhaas@gmail.com, tgl@sss.pgh.pa.us
Subject: Re: using index or check in ALTER TABLE SET NOT NULL
In-Reply-To: <354171516734367@web46g.yandex.ru>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox