On 2017/08/05 11:05, Robert Haas wrote:
> On Thu, Aug 3, 2017 at 8:45 PM, Amit Langote
> <langote_amit...@lab.ntt.co.jp> wrote:
>>> 0003 needs a rebase.
>>
>> Rebased patch attached.
>
> Committed.
Thank you.
> I think 0004 is a new feature, so I'm leaving that for v11.
Sure.
By the way, bulk of 0004 is refactoring which it seems is what Jeevan's
default partition patch set also includes as one of the patches [1]. It
got a decent amount review from Ashutosh. I broke it down into a separate
patch, so that the patch to add the new feature is its own tiny patch.
I also spotted a couple of comments referring to attachRel that we just
recently renamed.
So, attached are:
0001: s/attachRel/attachrel/g
0002: Refactoring to introduce a PartConstraintImpliedByRelConstraint
0003: Add the feature to skip the scan of individual leaf partitions
Totally fine if you postpone 0002 and 0003 to when the tree opens up for
PG 11.
Thanks,
Amit
[1]
https://www.postgresql.org/message-id/CAOgcT0OARciE2X%2BU0rjSKp9VuC279dYcCGkc3nCWKhHQ1_m2rw%40mail.gmail.com
From 26e7205fd7c35c0b497c1a7c31152393d3551b23 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Mon, 7 Aug 2017 10:45:39 +0900
Subject: [PATCH 1/3] Typo: attachRel is now attachrel
---
src/backend/commands/tablecmds.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 1b8d4b3d17..d27c43bdc7 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13491,7 +13491,7 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
* Prevent circularity by seeing if rel is a partition of attachrel. (In
* particular, this disallows making a rel a partition of itself.)
*
- * We do that by checking if rel is a member of the list of attachRel's
+ * We do that by checking if rel is a member of the list of attachrel's
* partitions provided the latter is partitioned at all. We want to
avoid
* having to construct this list again, so we request the strongest lock
* on all partitions. We need the strongest lock, because we may decide
@@ -13746,7 +13746,7 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
{
/*
* Adjust the constraint that we constructed
above for
- * attachRel so that it matches this
partition's attribute
+ * attachrel so that it matches this
partition's attribute
* numbers.
*/
my_partconstr =
map_partition_varattnos(my_partconstr, 1,
--
2.11.0
From fc6de6ab2800d509ec2af35c96722672e47e99cc Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Thu, 15 Jun 2017 19:22:31 +0900
Subject: [PATCH 2/3] Some refactoring of code in ATExecAttachPartition()
Take the code to check using table's constraints if the partition
constraint validation can be skipped and put it into a separate
function PartConstraintImpliedByRelConstraint().
---
src/backend/commands/tablecmds.c | 187 +++++++++++++++++++++------------------
1 file changed, 101 insertions(+), 86 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d27c43bdc7..818335ffe9 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -473,6 +473,8 @@ static void CreateInheritance(Relation child_rel, Relation
parent_rel);
static void RemoveInheritance(Relation child_rel, Relation parent_rel);
static ObjectAddress ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd);
+static bool PartConstraintImpliedByRelConstraint(Relation partrel,
+ List *partConstraint);
static ObjectAddress ATExecDetachPartition(Relation rel, RangeVar *name);
@@ -13422,15 +13424,12 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
Relation attachrel,
catalog;
List *attachrel_children;
- TupleConstr *attachrel_constr;
- List *partConstraint,
- *existConstraint;
+ List *partConstraint;
SysScanDesc scan;
ScanKeyData skey;
AttrNumber attno;
int natts;
TupleDesc tupleDesc;
- bool skip_validate = false;
ObjectAddress address;
const char *trigger_name;
bool found_whole_row;
@@ -13625,88 +13624,10 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
elog(ERROR, "unexpected whole-row reference found in partition
key");
/*
- * Check if we can do away with having to scan the table being attached
to
- * validate the partition constraint, by *proving* that the existing
- * constraints of the table *imply* the partition predicate. We include
- * the table's check constraints and NOT NULL constraints in the list of
- * clauses passed to predicate_implied_by().
- *
- * There is a case in which we cannot rely on just the result of the
- * proof.
+ * Based on the table's existing constraints, determine if we can skip
+ * scanning the table to validate the partition constraint.
*/
- attachrel_constr = tupleDesc->constr;
- existConstraint = NIL;
- if (attachrel_constr != NULL)
- {
- int num_check = attachrel_constr->num_check;
- int i;
-
- if (attachrel_constr->has_not_null)
- {
- int natts =
attachrel->rd_att->natts;
-
- for (i = 1; i <= natts; i++)
- {
- Form_pg_attribute att =
attachrel->rd_att->attrs[i - 1];
-
- if (att->attnotnull && !att->attisdropped)
- {
- NullTest *ntest = makeNode(NullTest);
-
- ntest->arg = (Expr *) makeVar(1,
-
i,
-
att->atttypid,
-
att->atttypmod,
-
att->attcollation,
-
0);
- ntest->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.
- */
- ntest->argisrow = false;
- ntest->location = -1;
- existConstraint =
lappend(existConstraint, ntest);
- }
- }
- }
-
- for (i = 0; i < num_check; i++)
- {
- Node *cexpr;
-
- /*
- * If this constraint hasn't been fully validated yet,
we must
- * ignore it here.
- */
- if (!attachrel_constr->check[i].ccvalid)
- continue;
-
- cexpr = stringToNode(attachrel_constr->check[i].ccbin);
-
- /*
- * Run each expression through const-simplification and
- * canonicalization. It is necessary, because we will
be
- * comparing it to similarly-processed qual clauses,
and may fail
- * to detect valid matches without this.
- */
- cexpr = eval_const_expressions(NULL, cexpr);
- cexpr = (Node *) canonicalize_qual((Expr *) cexpr);
-
- existConstraint = list_concat(existConstraint,
-
make_ands_implicit((Expr *) cexpr));
- }
-
- existConstraint =
list_make1(make_ands_explicit(existConstraint));
-
- /* And away we go ... */
- if (predicate_implied_by(partConstraint, existConstraint, true))
- skip_validate = true;
- }
-
- if (skip_validate)
+ if (PartConstraintImpliedByRelConstraint(attachrel, partConstraint))
{
/* No need to scan the table after all. */
ereport(INFO,
@@ -13715,9 +13636,18 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
}
else
{
- /* Constraints proved insufficient, so we need to scan the
table. */
ListCell *lc;
+ /*
+ * Constraints proved insufficient, so we need to scan the
table.
+ * However, if the table is partitioned, validation scans of the
+ * individual leaf partitions may still be skipped if they have
+ * constraints that would make scanning them unnecessary.
+ *
+ * Note that attachrel's OID is in the attachrel_children list.
Since
+ * we already determined above that its validation scan cannot
be
+ * skipped, we need not check that again in the loop below.
+ */
foreach(lc, attachrel_children)
{
AlteredTableInfo *tab;
@@ -13742,6 +13672,10 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
continue;
}
+ /*
+ * Check if the partition's existing constraints imply
the
+ * partition constraint and if so, skip the validation
scan.
+ */
if (part_rel != attachrel)
{
/*
@@ -13776,6 +13710,87 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
}
/*
+ * PartConstraintImpliedByRelConstraint
+ * Does partrel's existing constraints imply the partition
constraint?
+ *
+ * Existing constraints includes its check constraints and column-level
+ * NOT NULL constraints and partConstraint describes the partition constraint.
+ */
+static bool
+PartConstraintImpliedByRelConstraint(Relation partrel, List *partConstraint)
+{
+ List *existConstraint = NIL;
+ TupleConstr *constr = RelationGetDescr(partrel)->constr;
+ int num_check,
+ i;
+
+ if (constr && constr->has_not_null)
+ {
+ int natts = partrel->rd_att->natts;
+
+ for (i = 1; i <= natts; i++)
+ {
+ Form_pg_attribute att = partrel->rd_att->attrs[i - 1];
+
+ if (att->attnotnull && !att->attisdropped)
+ {
+ NullTest *ntest = makeNode(NullTest);
+
+ ntest->arg = (Expr *) makeVar(1,
+
i,
+
att->atttypid,
+
att->atttypmod,
+
att->attcollation,
+
0);
+ ntest->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.
+ */
+ ntest->argisrow = false;
+ ntest->location = -1;
+ existConstraint = lappend(existConstraint,
ntest);
+ }
+ }
+ }
+
+ num_check = (constr != NULL) ? constr->num_check : 0;
+ for (i = 0; i < num_check; i++)
+ {
+ Node *cexpr;
+
+ /*
+ * If this constraint hasn't been fully validated yet, we must
+ * ignore it here.
+ */
+ if (!constr->check[i].ccvalid)
+ continue;
+
+ cexpr = stringToNode(constr->check[i].ccbin);
+
+ /*
+ * Run each expression through const-simplification and
+ * canonicalization. It is necessary, because we will be
comparing
+ * it to similarly-processed partition constraint expressions,
and
+ * may fail to detect valid matches without this.
+ */
+ cexpr = eval_const_expressions(NULL, cexpr);
+ cexpr = (Node *) canonicalize_qual((Expr *) cexpr);
+
+ existConstraint = list_concat(existConstraint,
+
make_ands_implicit((Expr *) cexpr));
+ }
+
+ if (existConstraint != NIL)
+ existConstraint =
list_make1(make_ands_explicit(existConstraint));
+
+ /* And away we go ... */
+ return predicate_implied_by(partConstraint, existConstraint, true);
+}
+
+/*
* ALTER TABLE DETACH PARTITION
*
* Return the address of the relation that is no longer a partition of rel.
--
2.11.0
From f1905e74dcb5a4c26385fb4fb2558acf2b731ff1 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Mon, 7 Aug 2017 10:51:47 +0900
Subject: [PATCH 3/3] Teach ATExecAttachPartition to skip validation in more
cases
In cases where the table being attached is a partitioned table and
the table itself does not have constraints that would allow validation
on the whole table to be skipped, we can still skip the validations
of individual partitions if they each happen to have the requisite
constraints.
Per an idea of Robert Haas', with code refactoring suggestions from
Ashutosh Bapat.
---
src/backend/commands/tablecmds.c | 11 +++++++++++
src/test/regress/expected/alter_table.out | 12 ++++++++++++
src/test/regress/sql/alter_table.sql | 12 ++++++++++++
3 files changed, 35 insertions(+)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 818335ffe9..eae90dcf4a 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13689,6 +13689,17 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
/* There can never be a whole-row reference
here */
if (found_whole_row)
elog(ERROR, "unexpected whole-row
reference found in partition key");
+
+ if
(PartConstraintImpliedByRelConstraint(part_rel,
+
my_partconstr))
+ {
+ ereport(INFO,
+ (errmsg("partition
constraint for table \"%s\" is implied by existing constraints",
+
RelationGetRelationName(part_rel))));
+ if (part_rel != attachrel)
+ heap_close(part_rel, NoLock);
+ continue;
+ }
}
/* Grab a work queue entry. */
diff --git a/src/test/regress/expected/alter_table.out
b/src/test/regress/expected/alter_table.out
index 58192d2c6a..e636ff4561 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3392,6 +3392,18 @@ SELECT tableoid::regclass, a, b FROM part_7 order by a;
ALTER TABLE list_parted2 ATTACH PARTITION part_7 FOR VALUES IN (7);
ERROR: partition constraint is violated by some row
+-- If the partitioned table being attached does not have a constraint that
+-- would allow validation scan to be skipped, but an individual partition
+-- does, then the partition's validation scan is skipped. Note that the
+-- following leaf partition only allows rows that have a = 7 (and b = 'b' but
+-- that's irrelevant).
+CREATE TABLE part_7_b PARTITION OF part_7 (
+ CONSTRAINT check_a CHECK (a IS NOT NULL AND a = 7)
+) FOR VALUES IN ('b');
+-- The faulting row in part_7_a_null will still cause the command to fail
+ALTER TABLE list_parted2 ATTACH PARTITION part_7 FOR VALUES IN (7);
+INFO: partition constraint for table "part_7_b" is implied by existing
constraints
+ERROR: partition constraint is violated by some row
-- check that the table being attached is not already a partition
ALTER TABLE list_parted2 ATTACH PARTITION part_2 FOR VALUES IN (2);
ERROR: "part_2" is already a partition
diff --git a/src/test/regress/sql/alter_table.sql
b/src/test/regress/sql/alter_table.sql
index 9a20dd141a..1e1fe6f03d 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2216,6 +2216,18 @@ INSERT INTO part_7 (a, b) VALUES (8, null), (9, 'a');
SELECT tableoid::regclass, a, b FROM part_7 order by a;
ALTER TABLE list_parted2 ATTACH PARTITION part_7 FOR VALUES IN (7);
+-- If the partitioned table being attached does not have a constraint that
+-- would allow validation scan to be skipped, but an individual partition
+-- does, then the partition's validation scan is skipped. Note that the
+-- following leaf partition only allows rows that have a = 7 (and b = 'b' but
+-- that's irrelevant).
+CREATE TABLE part_7_b PARTITION OF part_7 (
+ CONSTRAINT check_a CHECK (a IS NOT NULL AND a = 7)
+) FOR VALUES IN ('b');
+-- The faulting row in part_7_a_null will still cause the command to fail
+ALTER TABLE list_parted2 ATTACH PARTITION part_7 FOR VALUES IN (7);
+
+
-- check that the table being attached is not already a partition
ALTER TABLE list_parted2 ATTACH PARTITION part_2 FOR VALUES IN (2);
--
2.11.0
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
