From: Sergei Kornilov <sk@zsrv.org>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Stephen Frost <sfrost@snowman.net>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: using index or check in ALTER TABLE SET NOT NULL
Date: Mon, 04 Dec 2017 17:09:44 +0300
Message-ID: <369811512396584@web6o.yandex.ru> (raw)
In-Reply-To: <CA+TgmoahVfu0dUmvM1bO3csn=DtFp1fG92995rjYD9u_kO-Y1g@mail.gmail.com>
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>
Hello
I update patch and also rebase to current head. I hope now it is better aligned with project style
Attachments:
[text/x-diff] alter_table_set_not_null_by_constraints_only_v3.patch (7.4K, ../369811512396584@web6o.yandex.ru/2-alter_table_set_not_null_by_constraints_only_v3.patch)
download | inline diff:diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.cindex 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.cindex 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.hindex 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 */
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, robertmhaas@gmail.com, tgl@sss.pgh.pa.us, sfrost@snowman.net
Subject: Re: using index or check in ALTER TABLE SET NOT NULL
In-Reply-To: <369811512396584@web6o.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