Thanks for the review again.
On 2017/06/22 19:55, Ashutosh Bapat wrote:
> On Thu, Jun 15, 2017 at 4:06 PM, Amit Langote
> <langote_amit...@lab.ntt.co.jp> wrote:
>>
>> Anyway, I tried to implement the refactoring in patch 0002, which is not
>> all of the patch 0001 that Jeevan posted. Please take a look. I wondered
>> if we should emit a NOTICE when an individual leaf partition validation
>> can be skipped?
>
> Yes. We emit an INFO for the table being attached. We want to do the
> same for the child. Or NOTICE In both the places.
Actually, I meant INFO.
>> No point in adding a new test if the answer to that is
>> no, I'd think.
Updated the patch 0002 so that an INFO message is printed for each leaf
partition and a test for the same.
>> Attaching here 0001 which fixes the bug (unchanged from the previous
>> version) and 0002 which implements the refactoring (and the feature to
>> look at the individual leaf partitions' constraints to see if validation
>> can be skipped.)
>
> Comments on 0001 patch.
> + *
> + * We request an exclusive lock on all the partitions, because we may
> + * decide later in this function to scan them to validate the new
> + * partition constraint.
> Does that mean that we may not scan the partitions later, in which the
> stronger
> lock we took was not needed. Is that right?
Yes. I wrote that comment thinking only about the deadlock hazard which
had then occurred to me, so the text somehow ended up being reflective of
that thinking. Please see the new comment, which hopefully is more
informative.
> Don't we need an exclusive lock to
> make sure that the constraints are not changed while we are validating those?
If I understand your question correctly, you meant to ask if we don't need
the strongest lock on individual partitions while looking at their
constraints to prove that we don't need to scan them. We do and we do
take the strongest lock on individual partitions even today in the second
call to find_all_inheritors(). We're trying to eliminate the second call
here.
With the current code, we take AccessShareLock in the first call when
checking the circularity of inheritance. Then if attachRel doesn't have
the constraint to avoid the scan, we decide to look at individual
partitions (their rows, not constraints, as of now) when we take
AccessExclusiveLock. That might cause a deadlock (was able to reproduce
one using the debugger).
> if (!skip_validate)
> May be we should turn this into "else" condition of the "if" just above.
Yes, done.
> + /*
> + * We already collected the list of partitions, including the table
> + * named in the command itself, which should appear at the head of
> the
> + * list.
> + */
> + Assert(list_length(attachRel_children) >= 1 &&
> + linitial_oid(attachRel_children) == RelationGetRelid(attachRel));
> Probably the Assert should be moved next to find_all_inheritors(). But I don't
> see much value in this comment and the Assert at this place.
I agree, removed both.
>
> + /* Skip the original table if it's partitioned. */
> + if (part_relid == RelationGetRelid(attachRel) &&
> + attachRel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
> + continue;
> +
>
> There isn't much point in checking this for every child in the loop. Instead,
> we should remove attachRel from the attachRel_children if there are more than
> one elements in the list (which means the attachRel is partitioned, may be
> verify that by Asserting).
Rearranged code considering these comments.
> Comments on 0002 patch.
> Thanks for the refactoring. The code looks really good now.
Thanks.
> The name skipPartConstraintValidation() looks very specific to the case at
> hand. The function is really checking whether existing constraints on the
> table
> can imply the given constraints (which happen to be partition constraints).
> How
> about PartConstraintImpliedByRelConstraint()? The idea is to pick a general
> name so that the function can be used for purposes other than skipping
> validation scan in future.
I liked this idea, so done.
> * This basically returns if the partrel's existing constraints, which
> returns "true". Add "otherwise returns false".
>
> if (constr != NULL)
> {
> ...
> }
> return false;
> May be you should return false when constr == NULL (I prefer !constr, but
> others may have different preferences.). That should save us one level of
> indentation. At the end just return whatever predicate_implied_by() returns.
Good suggestion, done.
Attached updated patches.
Thanks,
Amit
From ee034a3bcb25a8b516220636134fe3ed38796cfe Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Wed, 14 Jun 2017 11:32:01 +0900
Subject: [PATCH 1/2] Cope with differing attnos in ATExecAttachPartition code
If the table being attached has different attnos from the parent for
the partitioning columns which are present in the partition constraint
expressions, then predicate_implied_by() will prematurely return false
due to the structural inequality of the corresponding Var expressions
in the partition constraint and those in the table's check constraint
expressions. Fix this by mapping the partition constraint's expressions
to bear the partition's attnos.
Further, if the validation scan needs to be performed after all and
the table being attached is a partitioned table, we will need to map
the constraint expression again to change the attnos to the individual
leaf partition's attnos from those of the table being attached.
Another minor fix:
Avoid creating an AT work queue entry for the table being attached if
it's partitioned. Current coding does not lead to that happening.
---
src/backend/commands/tablecmds.c | 72 ++++++++++++++++++++-----------
src/test/regress/expected/alter_table.out | 45 +++++++++++++++++++
src/test/regress/sql/alter_table.sql | 38 ++++++++++++++++
3 files changed, 131 insertions(+), 24 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7d9c769b06..683bbbc08f 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13407,7 +13407,7 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
{
Relation attachRel,
catalog;
- List *childrels;
+ List *attachRel_children;
TupleConstr *attachRel_constr;
List *partConstraint,
*existConstraint;
@@ -13472,12 +13472,26 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
heap_close(catalog, AccessShareLock);
/*
- * Prevent circularity by seeing if rel is a partition of attachRel. (In
+ * Prevent circularity by seeing if rel is a partition of attachRel, (In
* particular, this disallows making a rel a partition of itself.)
+ *
+ * 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 to scan them if we find out that the table being
attached
+ * (or its leaf partitions) may contain rows that violate the partition
+ * constraint due to lack of a constraint that would have prevented them
+ * in the first place. If such a constraint is present (which by
+ * definition is present in all partitions), we are able to skip the
scan.
+ * But we cannot risk a deadlock by taking the strongest lock only when
+ * needed by taking a weaker one now.
+ *
+ * XXX - Do we need to lock the partitions here if we already have the
+ * strongest lock on attachRel? The information we need here to check
+ * for circularity cannot change without taking a lock on attachRel.
*/
- childrels = find_all_inheritors(RelationGetRelid(attachRel),
-
AccessShareLock, NULL);
- if (list_member_oid(childrels, RelationGetRelid(rel)))
+ attachRel_children = find_all_inheritors(RelationGetRelid(attachRel),
+
AccessExclusiveLock, NULL);
+ if (list_member_oid(attachRel_children, RelationGetRelid(rel)))
ereport(ERROR,
(errcode(ERRCODE_DUPLICATE_TABLE),
errmsg("circular inheritance not allowed"),
@@ -13575,6 +13589,13 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
partConstraint = list_make1(make_ands_explicit(partConstraint));
/*
+ * Adjust the generated constraint to match this partition's attribute
+ * numbers.
+ */
+ partConstraint = map_partition_varattnos(partConstraint, 1, attachRel,
+
rel);
+
+ /*
* 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
@@ -13661,25 +13682,24 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
ereport(INFO,
(errmsg("partition constraint for table \"%s\"
is implied by existing constraints",
RelationGetRelationName(attachRel))));
-
- /*
- * Set up to have the table be scanned to validate the partition
- * constraint (see partConstraint above). If it's a partitioned table,
we
- * instead schedule its leaf partitions to be scanned.
- */
- if (!skip_validate)
+ else
{
- List *all_parts;
ListCell *lc;
- /* Take an exclusive lock on the partitions to be checked */
+ /*
+ * Schedule the table (or leaf partitions if partitioned) to be
scanned
+ * later.
+ *
+ * Note that attachRel's OID is in this list. If it's
partitioned, we
+ * we don't need to schedule it to be scanned (would be a noop
anyway
+ * even if we did), so just remove it from the list.
+ */
+ Assert(linitial_oid(attachRel_children) ==
+
RelationGetRelid(attachRel));
if (attachRel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- all_parts =
find_all_inheritors(RelationGetRelid(attachRel),
-
AccessExclusiveLock, NULL);
- else
- all_parts = list_make1_oid(RelationGetRelid(attachRel));
+ attachRel_children =
list_delete_first(attachRel_children);
- foreach(lc, all_parts)
+ foreach(lc, attachRel_children)
{
AlteredTableInfo *tab;
Oid part_relid = lfirst_oid(lc);
@@ -13696,21 +13716,25 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
* Skip if it's a partitioned table. Only
RELKIND_RELATION
* relations (ie, leaf partitions) need to be scanned.
*/
- if (part_rel != attachRel &&
- part_rel->rd_rel->relkind ==
RELKIND_PARTITIONED_TABLE)
+ if (part_rel->rd_rel->relkind ==
RELKIND_PARTITIONED_TABLE)
{
- heap_close(part_rel, NoLock);
+ if (part_rel != attachRel)
+ heap_close(part_rel, NoLock);
continue;
}
/* Grab a work queue entry */
tab = ATGetQueueEntry(wqueue, part_rel);
- /* Adjust constraint to match this partition */
+ /*
+ * Adjust the constraint that we constructed above for
the table
+ * being attached so that it matches this partition's
attribute
+ * numbers.
+ */
constr = linitial(partConstraint);
tab->partition_constraint = (Expr *)
map_partition_varattnos((List *) constr, 1,
-
part_rel, rel);
+
part_rel, attachRel);
/* keep our lock until commit */
if (part_rel != attachRel)
heap_close(part_rel, NoLock);
diff --git a/src/test/regress/expected/alter_table.out
b/src/test/regress/expected/alter_table.out
index 13d6a4b747..3ec5080fd6 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3347,6 +3347,51 @@ ALTER TABLE part_5 DROP CONSTRAINT check_a;
ALTER TABLE part_5 ADD CONSTRAINT check_a CHECK (a IN (5)), ALTER a SET NOT
NULL;
ALTER TABLE list_parted2 ATTACH PARTITION part_5 FOR VALUES IN (5);
INFO: partition constraint for table "part_5" is implied by existing
constraints
+-- Check the case where attnos of the partitioning columns in the table being
+-- attached differs from the parent. It should not affect the constraint-
+-- checking logic that allows to skip the scan.
+CREATE TABLE part_6 (
+ c int,
+ LIKE list_parted2,
+ CONSTRAINT check_a CHECK (a IS NOT NULL AND a = 6)
+);
+ALTER TABLE part_6 DROP c;
+ALTER TABLE list_parted2 ATTACH PARTITION part_6 FOR VALUES IN (6);
+INFO: partition constraint for table "part_6" is implied by existing
constraints
+-- Similar to above, but the table being attached is a partitioned table
+-- whose partition has still different attnos for the root partitioning
+-- columns.
+CREATE TABLE part_7 (
+ LIKE list_parted2,
+ CONSTRAINT check_a CHECK (a IS NOT NULL AND a = 7)
+) PARTITION BY LIST (b);
+CREATE TABLE part_7_a_null (
+ c int,
+ d int,
+ e int,
+ LIKE list_parted2, -- 'a' will have attnum = 4
+ CONSTRAINT check_b CHECK (b IS NULL OR b = 'a'),
+ CONSTRAINT check_a CHECK (a IS NOT NULL AND a = 7)
+);
+ALTER TABLE part_7_a_null DROP c, DROP d, DROP e;
+ALTER TABLE part_7 ATTACH PARTITION part_7_a_null FOR VALUES IN ('a', null);
+INFO: partition constraint for table "part_7_a_null" is implied by existing
constraints
+ALTER TABLE list_parted2 ATTACH PARTITION part_7 FOR VALUES IN (7);
+INFO: partition constraint for table "part_7" is implied by existing
constraints
+-- Same example, but check this time that the constraint correctly detects
+-- violating rows
+ALTER TABLE list_parted2 DETACH PARTITION part_7;
+ALTER TABLE part_7 DROP CONSTRAINT check_a; -- thusly, scan won't be skipped
+INSERT INTO part_7 (a, b) VALUES (8, null), (9, 'a');
+SELECT tableoid::regclass, a, b FROM part_7 order by a;
+ tableoid | a | b
+---------------+---+---
+ part_7_a_null | 8 |
+ part_7_a_null | 9 | a
+(2 rows)
+
+ALTER TABLE list_parted2 ATTACH PARTITION part_7 FOR VALUES IN (7);
+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 5dd1402ea6..e0b7b37278 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2178,6 +2178,44 @@ ALTER TABLE part_5 DROP CONSTRAINT check_a;
ALTER TABLE part_5 ADD CONSTRAINT check_a CHECK (a IN (5)), ALTER a SET NOT
NULL;
ALTER TABLE list_parted2 ATTACH PARTITION part_5 FOR VALUES IN (5);
+-- Check the case where attnos of the partitioning columns in the table being
+-- attached differs from the parent. It should not affect the constraint-
+-- checking logic that allows to skip the scan.
+CREATE TABLE part_6 (
+ c int,
+ LIKE list_parted2,
+ CONSTRAINT check_a CHECK (a IS NOT NULL AND a = 6)
+);
+ALTER TABLE part_6 DROP c;
+ALTER TABLE list_parted2 ATTACH PARTITION part_6 FOR VALUES IN (6);
+
+-- Similar to above, but the table being attached is a partitioned table
+-- whose partition has still different attnos for the root partitioning
+-- columns.
+CREATE TABLE part_7 (
+ LIKE list_parted2,
+ CONSTRAINT check_a CHECK (a IS NOT NULL AND a = 7)
+) PARTITION BY LIST (b);
+CREATE TABLE part_7_a_null (
+ c int,
+ d int,
+ e int,
+ LIKE list_parted2, -- 'a' will have attnum = 4
+ CONSTRAINT check_b CHECK (b IS NULL OR b = 'a'),
+ CONSTRAINT check_a CHECK (a IS NOT NULL AND a = 7)
+);
+ALTER TABLE part_7_a_null DROP c, DROP d, DROP e;
+ALTER TABLE part_7 ATTACH PARTITION part_7_a_null FOR VALUES IN ('a', null);
+ALTER TABLE list_parted2 ATTACH PARTITION part_7 FOR VALUES IN (7);
+
+-- Same example, but check this time that the constraint correctly detects
+-- violating rows
+ALTER TABLE list_parted2 DETACH PARTITION part_7;
+ALTER TABLE part_7 DROP CONSTRAINT check_a; -- thusly, scan won't be skipped
+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);
+
-- 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
From 16036f23e166bf0d5c3c03a70e92cf0bf13f63ef Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Thu, 15 Jun 2017 19:22:31 +0900
Subject: [PATCH 2/2] 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 | 223 ++++++++++++++++--------------
src/test/regress/expected/alter_table.out | 12 ++
src/test/regress/sql/alter_table.sql | 11 ++
3 files changed, 145 insertions(+), 101 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 683bbbc08f..0d954e7e2d 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);
@@ -13408,15 +13410,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;
attachRel = heap_openrv(cmd->name, AccessExclusiveLock);
@@ -13596,89 +13595,10 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
rel);
/*
- * 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
the
+ * partition constraint validation scan.
*/
- 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;
- }
-
- /* It's safe to skip the validation scan after all */
- if (skip_validate)
+ if (PartConstraintImpliedByRelConstraint(attachRel, partConstraint))
ereport(INFO,
(errmsg("partition constraint for table \"%s\"
is implied by existing constraints",
RelationGetRelationName(attachRel))));
@@ -13687,12 +13607,15 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
ListCell *lc;
/*
- * Schedule the table (or leaf partitions if partitioned) to be
scanned
- * later.
+ * For each leaf partition, check if it we can skip the
validation
+ * scan because it has constraints that allows to do so. If
not,
+ * schedule it to be scanned later.
*
- * Note that attachRel's OID is in this list. If it's
partitioned, we
+ * Note that attachRel's OID is in this list. Since we already
+ * determined above that its validation scan cannot be skipped,
we
+ * need not check that again in the loop below. If it's
partitioned,
* we don't need to schedule it to be scanned (would be a noop
anyway
- * even if we did), so just remove it from the list.
+ * even if we did) either, so just remove it from the list.
*/
Assert(linitial_oid(attachRel_children) ==
RelationGetRelid(attachRel));
@@ -13704,7 +13627,7 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
AlteredTableInfo *tab;
Oid part_relid = lfirst_oid(lc);
Relation part_rel;
- Expr *constr;
+ List *my_partconstr;
/* Lock already taken */
if (part_relid != RelationGetRelid(attachRel))
@@ -13713,6 +13636,23 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
part_rel = attachRel;
/*
+ * Adjust the constraint that we constructed above for
attachRel
+ * so that it matches this partition's attribute
numbers.
+ */
+ my_partconstr = map_partition_varattnos(partConstraint,
1,
+
part_rel,
+
attachRel);
+ 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;
+ }
+
+ /*
* Skip if it's a partitioned table. Only
RELKIND_RELATION
* relations (ie, leaf partitions) need to be scanned.
*/
@@ -13723,18 +13663,10 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
continue;
}
- /* Grab a work queue entry */
+ /* Nope. So grab a work queue entry. */
tab = ATGetQueueEntry(wqueue, part_rel);
+ tab->partition_constraint = (Expr *)
linitial(my_partconstr);
- /*
- * Adjust the constraint that we constructed above for
the table
- * being attached so that it matches this partition's
attribute
- * numbers.
- */
- constr = linitial(partConstraint);
- tab->partition_constraint = (Expr *)
- map_partition_varattnos((List *) constr, 1,
-
part_rel, attachRel);
/* keep our lock until commit */
if (part_rel != attachRel)
heap_close(part_rel, NoLock);
@@ -13750,6 +13682,95 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
}
/*
+ * skipPartConstraintValidation
+ * Can we skip partition constraint validation?
+ *
+ * This basically returns if the partrel's existing constraints, which
+ * includes its check constraints and column-level NOT NULL constraints,
+ * imply the partition constraint as described in partConstraint.
+ */
+static bool
+PartConstraintImpliedByRelConstraint(Relation partrel, List *partConstraint)
+{
+ List *existConstraint = NIL;
+ TupleConstr *constr = RelationGetDescr(partrel)->constr;
+ int num_check,
+ i;
+
+ if (constr == NULL)
+ return false;
+
+ num_check = constr->num_check;
+
+ if (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);
+ }
+ }
+ }
+
+ 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 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))
+ return true;
+
+ /* Tough luck. */
+ return false;
+}
+
+/*
* ALTER TABLE DETACH PARTITION
*
* Return the address of the relation that is no longer a partition of rel.
diff --git a/src/test/regress/expected/alter_table.out
b/src/test/regress/expected/alter_table.out
index 3ec5080fd6..03571f0e7c 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 e0b7b37278..7e270b77ca 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2216,6 +2216,17 @@ 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
