On 2017/06/15 18:05, Ashutosh Bapat wrote:
> On Thu, Jun 15, 2017 at 2:30 PM, Amit Langote
> <langote_amit...@lab.ntt.co.jp> wrote:
>> On 2017/06/15 17:53, Ashutosh Bapat wrote:
>>> On Thu, Jun 15, 2017 at 2:12 PM, Amit Langote wrote:
>>>>> Both of the above comments are not related to the bug that is being
>>>>> fixed, but
>>>>> they apply to the same code where the bug exists. So instead of fixing it
>>>>> twice, may be we should expand the scope of this work to cover other
>>>>> refactoring needed in this area. That might save us some rebasing and
>>>>> commits.
>>>>
>>>> Are you saying that the patch posted on that thread should be brought over
>>>> and discussed here?
>>>
>>> Not the whole patch, but that one particular comment, which applies to
>>> the existing code in ATExecAttachPartition(). If we fix the existing
>>> code in ATExecAttachPartition(), the refactoring patch there will
>>> inherit it when rebased.
>>
>> Yes, I too meant only the refactoring patch, which I see as patch 0001 in
>> the series of patches that Jeevan posted with the following message:
>>
>> https://www.postgresql.org/message-id/CAOgcT0NeR%3D%2BTMRTw6oq_5WrJF%2B_xG91k_nGUub29Lnv5-qmQHw%40mail.gmail.com
>
> I think we don't need to move that patch over to here, unless you see
> that some of that refactoring is useful here. I think, we should
> continue this thread and patch independent of what happens there. If
> and when this patch gets committed, that patch will need to be
> refactored.
I do see it as useful refactoring and a way to implement a feature (which
is perhaps something worth including into v10?) It's just that the patch
I have posted here fixes bugs, which it would be nice to get committed first.
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? No point in adding a new test if the answer to that is
no, I'd think.
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.)
Thanks,
Amit
From 941ef679635e6848d72edde4721c9d0ac4e9ff45 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 | 59 +++++++++++++++++++------------
src/test/regress/expected/alter_table.out | 45 +++++++++++++++++++++++
src/test/regress/sql/alter_table.sql | 38 ++++++++++++++++++++
3 files changed, 120 insertions(+), 22 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index b4425bc6af..981b7ae902 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13412,7 +13412,7 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
{
Relation attachRel,
catalog;
- List *childrels;
+ List *attachRel_children;
TupleConstr *attachRel_constr;
List *partConstraint,
*existConstraint;
@@ -13479,10 +13479,14 @@ 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 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.
*/
- 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"),
@@ -13580,6 +13584,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
@@ -13674,35 +13685,36 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
*/
if (!skip_validate)
{
- 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));
+ /*
+ * 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));
- foreach(lc, all_parts)
+ foreach(lc, attachRel_children)
{
AlteredTableInfo *tab;
Oid part_relid = lfirst_oid(lc);
Relation part_rel;
Expr *constr;
+ /* Skip the original table if it's partitioned. */
+ if (part_relid == RelationGetRelid(attachRel) &&
+ attachRel->rd_rel->relkind ==
RELKIND_PARTITIONED_TABLE)
+ continue;
+
/* Lock already taken */
- if (part_relid != RelationGetRelid(attachRel))
- part_rel = heap_open(part_relid, NoLock);
- else
- part_rel = attachRel;
+ part_rel = heap_open(part_relid, NoLock);
/*
* 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);
continue;
@@ -13711,14 +13723,17 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
/* 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);
+ 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 a7fe5ef75aaee54a2573232d0d27f81f3496efdd 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 | 209 ++++++++++++++++++++++-----------------
1 file changed, 117 insertions(+), 92 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 981b7ae902..b998dff35d 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 skipPartConstraintValidation(Relation partrel,
+ List *partConstraint);
static ObjectAddress ATExecDetachPartition(Relation rel, RangeVar *name);
@@ -13413,9 +13415,7 @@ 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;
@@ -13591,29 +13591,124 @@ 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.
+ */
+ if (skipPartConstraintValidation(attachRel, partConstraint))
+ {
+ ereport(INFO,
+ (errmsg("partition constraint for table \"%s\"
is implied by existing constraints",
+
RelationGetRelationName(attachRel))));
+ skip_validate = true;
+ }
+
+ /*
+ * 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.
*/
- attachRel_constr = tupleDesc->constr;
- existConstraint = NIL;
- if (attachRel_constr != NULL)
+ if (!skip_validate)
{
- int num_check = attachRel_constr->num_check;
+ ListCell *lc;
+
+ /*
+ * 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));
+
+ foreach(lc, attachRel_children)
+ {
+ AlteredTableInfo *tab;
+ Oid part_relid = lfirst_oid(lc);
+ Relation part_rel;
+ List *my_partconstr;
+
+ /* Skip the original table if it's partitioned. */
+ if (part_relid == RelationGetRelid(attachRel) &&
+ attachRel->rd_rel->relkind ==
RELKIND_PARTITIONED_TABLE)
+ continue;
+
+ /* Lock already taken */
+ part_rel = heap_open(part_relid, NoLock);
+
+ /*
+ * Skip if it's a partitioned table. Only
RELKIND_RELATION
+ * relations (ie, leaf partitions) need to be scanned.
+ */
+ if (part_rel->rd_rel->relkind ==
RELKIND_PARTITIONED_TABLE)
+ {
+ heap_close(part_rel, NoLock);
+ continue;
+ }
+
+ /*
+ * Can we skip validating this partition? We already
concluded
+ * negatively for attachRel.
+ */
+ my_partconstr = partConstraint;
+ if (part_relid != RelationGetRelid(attachRel))
+ {
+ /*
+ * Adjust the constraint that we constructed
above for the
+ * table being attached so that it matches this
partition's
+ * attribute numbers.
+ */
+ my_partconstr =
map_partition_varattnos(partConstraint, 1,
+
part_rel,
+
attachRel);
+ if (skipPartConstraintValidation(part_rel,
my_partconstr))
+ {
+ heap_close(part_rel, NoLock);
+ continue;
+ }
+ }
+
+ /* Nope. So grab a work queue entry. */
+ tab = ATGetQueueEntry(wqueue, part_rel);
+ tab->partition_constraint = (Expr *)
linitial(my_partconstr);
+
+ /* keep our lock until commit */
+ heap_close(part_rel, NoLock);
+ }
+ }
+
+ ObjectAddressSet(address, RelationRelationId,
RelationGetRelid(attachRel));
+
+ /* keep our lock until commit */
+ heap_close(attachRel, NoLock);
+
+ return address;
+}
+
+/*
+ * 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
+skipPartConstraintValidation(Relation partrel, List *partConstraint)
+{
+ List *existConstraint = NIL;
+ TupleConstr *constr = RelationGetDescr(partrel)->constr;
+
+ if (constr != NULL)
+ {
+ int num_check = constr->num_check;
int i;
- if (attachRel_constr->has_not_null)
+ if (constr->has_not_null)
{
- int natts =
attachRel->rd_att->natts;
+ int natts = partrel->rd_att->natts;
for (i = 1; i <= natts; i++)
{
- Form_pg_attribute att =
attachRel->rd_att->attrs[i - 1];
+ Form_pg_attribute att =
partrel->rd_att->attrs[i - 1];
if (att->attnotnull && !att->attisdropped)
{
@@ -13647,10 +13742,10 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
* If this constraint hasn't been fully validated yet,
we must
* ignore it here.
*/
- if (!attachRel_constr->check[i].ccvalid)
+ if (!constr->check[i].ccvalid)
continue;
- cexpr = stringToNode(attachRel_constr->check[i].ccbin);
+ cexpr = stringToNode(constr->check[i].ccbin);
/*
* Run each expression through const-simplification and
@@ -13669,80 +13764,10 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
/* 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)
- 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)
- {
- ListCell *lc;
-
- /*
- * 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));
-
- foreach(lc, attachRel_children)
- {
- AlteredTableInfo *tab;
- Oid part_relid = lfirst_oid(lc);
- Relation part_rel;
- Expr *constr;
-
- /* Skip the original table if it's partitioned. */
- if (part_relid == RelationGetRelid(attachRel) &&
- attachRel->rd_rel->relkind ==
RELKIND_PARTITIONED_TABLE)
- continue;
-
- /* Lock already taken */
- part_rel = heap_open(part_relid, NoLock);
-
- /*
- * Skip if it's a partitioned table. Only
RELKIND_RELATION
- * relations (ie, leaf partitions) need to be scanned.
- */
- if (part_rel->rd_rel->relkind ==
RELKIND_PARTITIONED_TABLE)
- {
- heap_close(part_rel, NoLock);
- continue;
- }
-
- /* Grab a work queue entry */
- tab = ATGetQueueEntry(wqueue, part_rel);
-
- /*
- * 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 */
- heap_close(part_rel, NoLock);
- }
+ return true;
}
- ObjectAddressSet(address, RelationRelationId,
RelationGetRelid(attachRel));
-
- /* keep our lock until commit */
- heap_close(attachRel, NoLock);
-
- return address;
+ return false;
}
/*
--
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
