Thanks for reviewing.
On 2017/08/02 2:54, Robert Haas wrote:
> On Mon, Jul 31, 2017 at 11:10 PM, Amit Langote
> <langote_amit...@lab.ntt.co.jp> wrote:
>> OK, these cosmetic changes are now in attached patch 0001.
>
> Regarding 0001:
>
> - List *childrels;
> + List *attachRel_children;
>
> I sorta don't see why this is necessary, or better.
Since ATExecAttachPartition() deals with the possibility that the table
being attached itself might be partitioned, someone reading the code might
find it helpful to get some clue about whose partitions/children a
particular piece of code is dealing with - AT's target table's (rel's) or
those of the table being attached (attachRel's)? IMHO, attachRel_children
makes it abundantly clear that it is in fact the partitions of the table
being attached that are being manipulated.
> /* It's safe to skip the validation scan after all */
> if (skip_validate)
> + {
> + /* No need to scan the table after all. */
>
> The existing comment should be removed along with adding the new one, I think.
Oops, fixed.
> - 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);
>
> This works out to a cosmetic change, I guess, but it makes it worse...
Not sure what you mean by "makes it worse". The comment above says that
we should skip partitioned tables from being scheduled for heap scan. The
new code still does that. We should close part_rel before continuing to
consider the next partition, but mustn't do that if part_rel is really
attachRel. The new code does that too. Stylistically worse?
Thanks,
Amit
From d67fac574abb085db33e00711ded0515d71d99eb Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Tue, 1 Aug 2017 10:12:39 +0900
Subject: [PATCH 1/4] Cosmetic fixes for code in ATExecAttachPartition
---
src/backend/commands/tablecmds.c | 33 +++++++++++++++------------------
1 file changed, 15 insertions(+), 18 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index bb00858ad1..1307dc5893 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13421,7 +13421,7 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
{
Relation attachRel,
catalog;
- List *childrels;
+ List *attachRel_children;
TupleConstr *attachRel_constr;
List *partConstraint,
*existConstraint;
@@ -13490,9 +13490,9 @@ 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.)
*/
- childrels = find_all_inheritors(RelationGetRelid(attachRel),
-
AccessShareLock, NULL);
- if (list_member_oid(childrels, RelationGetRelid(rel)))
+ attachRel_children = find_all_inheritors(RelationGetRelid(attachRel),
+
AccessShareLock, NULL);
+ if (list_member_oid(attachRel_children, RelationGetRelid(rel)))
ereport(ERROR,
(errcode(ERRCODE_DUPLICATE_TABLE),
errmsg("circular inheritance not allowed"),
@@ -13684,19 +13684,16 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
skip_validate = true;
}
- /* It's safe to skip the validation scan after all */
if (skip_validate)
+ {
+ /* No need to scan the table after all. */
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
{
+ /* Constraints proved insufficient, so we need to scan the
table. */
List *all_parts;
ListCell *lc;
@@ -13721,17 +13718,17 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
part_rel = attachRel;
/*
- * Skip if it's a partitioned table. Only
RELKIND_RELATION
- * relations (ie, leaf partitions) need to be scanned.
+ * Skip if the partition is itself a partitioned table.
We can
+ * only ever scan RELKIND_RELATION relations.
*/
- 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 */
+ /* Grab a work queue entry. */
tab = ATGetQueueEntry(wqueue, part_rel);
/* Adjust constraint to match this partition */
--
2.11.0
From 925b450cc69e8ccdbb191cb6b6d29f97aad2945f Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Tue, 1 Aug 2017 10:31:00 +0900
Subject: [PATCH 2/4] Fix lock-upgrade deadlock hazard in ATExecAttachPartition
Currently, we needless call find_all_inheritors twice to get the
partitions of the table being attached, with a share lock request
during the first call and exclusive lock in the second.
Fix to call find_all_inheritors() only once and request exclusive
lock on children. We need the exclusive lock, because might have
to scan the individual partitions to validate the partition
constraint being added.
---
src/backend/commands/tablecmds.c | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 1307dc5893..1d879fa61a 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13489,9 +13489,20 @@ 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
+ * 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
+ * to scan them if we find out that the table being attached (or its
leaf
+ * partitions) may contain rows that violate the partition constraint.
+ * If the table has a constraint that would prevent such rows, which by
+ * definition is present in all the partitions, we need not scan the
+ * table, nor its partitions. But we cannot risk a deadlock by taking a
+ * weaker lock now and the stronger one only when needed.
*/
attachRel_children = find_all_inheritors(RelationGetRelid(attachRel),
-
AccessShareLock, NULL);
+
AccessExclusiveLock, NULL);
if (list_member_oid(attachRel_children, RelationGetRelid(rel)))
ereport(ERROR,
(errcode(ERRCODE_DUPLICATE_TABLE),
@@ -13694,17 +13705,9 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
else
{
/* Constraints proved insufficient, so we need to scan the
table. */
- List *all_parts;
ListCell *lc;
- /* Take an exclusive lock on the partitions to be checked */
- 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));
-
- foreach(lc, all_parts)
+ foreach(lc, attachRel_children)
{
AlteredTableInfo *tab;
Oid part_relid = lfirst_oid(lc);
--
2.11.0
From 5478b98edcca677c3967e39c6de3869c94042dcb Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Tue, 1 Aug 2017 10:58:38 +0900
Subject: [PATCH 3/4] Cope with differing attnos in ATExecAttachPartition code
If the table being attached has attnos different 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 structural inequality of the corresponding Var expressions in the
the partition constraint and those in the table's check constraint
expressions. Fix this by changing 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.
Reported by: Ashutosh Bapat
Report:
https://postgr.es/m/CAFjFpReT_kq_uwU_B8aWDxR7jNGE%3DP0iELycdq5oupi%3DxSQTOw%40mail.gmail.com
---
src/backend/commands/tablecmds.c | 27 +++++++++++++++----
src/test/regress/expected/alter_table.out | 45 +++++++++++++++++++++++++++++++
src/test/regress/sql/alter_table.sql | 38 ++++++++++++++++++++++++++
3 files changed, 105 insertions(+), 5 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 1d879fa61a..3299ae5b5b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13614,6 +13614,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
@@ -13712,7 +13719,7 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
AlteredTableInfo *tab;
Oid part_relid = lfirst_oid(lc);
Relation part_rel;
- Expr *constr;
+ List *my_partconstr = partConstraint;
/* Lock already taken */
if (part_relid != RelationGetRelid(attachRel))
@@ -13731,14 +13738,24 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
continue;
}
+ if (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(my_partconstr, 1,
+
part_rel,
+
attachRel);
+ }
+
/* Grab a work queue entry. */
tab = ATGetQueueEntry(wqueue, part_rel);
/* Adjust constraint to match this partition */
- constr = linitial(partConstraint);
- tab->partition_constraint = (Expr *)
- map_partition_varattnos((List *) constr, 1,
-
part_rel, rel);
+ tab->partition_constraint = (Expr *)
linitial(my_partconstr);
+
/* 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..b727f4bcde 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..9a20dd141a 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 22e3f785efe854400ae3dc5fea6a5f7c01b6ce13 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Thu, 15 Jun 2017 19:22:31 +0900
Subject: [PATCH 4/4] 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 | 198 +++++++++++++++++-------------
src/test/regress/expected/alter_table.out | 12 ++
src/test/regress/sql/alter_table.sql | 11 ++
3 files changed, 135 insertions(+), 86 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3299ae5b5b..787bb840a5 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;
@@ -13621,88 +13620,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
+ * 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,
@@ -13711,9 +13632,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;
@@ -13738,6 +13668,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)
{
/*
@@ -13748,6 +13682,17 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
my_partconstr =
map_partition_varattnos(my_partconstr, 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;
+ }
}
/* Grab a work queue entry. */
@@ -13771,6 +13716,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.
diff --git a/src/test/regress/expected/alter_table.out
b/src/test/regress/expected/alter_table.out
index b727f4bcde..568fb3b2b6 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..d7dd3b8984 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
