From: Sergei Kornilov <sk@zsrv.org>
To: PostgreSQL mailing lists <pgsql-hackers@postgresql.org>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Stephen Frost <sfrost@snowman.net>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Ildar Musin <i.musin@postgrespro.ru>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Subject: Re: using index or check in ALTER TABLE SET NOT NULL
Date: Wed, 11 Apr 2018 21:55:46 +0300
Message-ID: <247201523472946@web58j.yandex.ru> (raw)
In-Reply-To: <979461523301587@web39j.yandex.ru>
References: <880511520351578@web5j.yandex.ru>
<20180306172528.xnelm3xcq2qeyd2g@alvherre.pgsql>
<1715451520361128@web26o.yandex.ru>
<1142.1520362313@sss.pgh.pa.us>
<082cf493-a989-dcd6-7baf-a94a40d07ea0@postgrespro.ru>
<2947361520607085@web26g.yandex.ru>
<5376321520674521@web1j.yandex.ru>
<979461523301587@web39j.yandex.ru>
Hi
I noticed new merge conflict, updated version attached.
regards, Sergei
Attachments:
[text/x-diff] alter_table_set_not_null_by_constraints_only_v9.patch (13.4K, ../247201523472946@web58j.yandex.ru/2-alter_table_set_not_null_by_constraints_only_v9.patch)
download | inline diff:diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgmlindex 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.cindex 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.cindex 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.hindex 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.outindex 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.sqlindex 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);
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, alvherre@alvh.no-ip.org, sfrost@snowman.net, robertmhaas@gmail.com, i.musin@postgrespro.ru, tgl@sss.pgh.pa.us
Subject: Re: using index or check in ALTER TABLE SET NOT NULL
In-Reply-To: <247201523472946@web58j.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