Thanks for the review.
On 2017/06/15 16:08, Ashutosh Bapat wrote:
> On Thu, Jun 15, 2017 at 10:46 AM, Amit Langote wrote:
>> If we end up having to perform the validation scan and the table being
>> attached is a partitioned table, we will scan its leaf partitions. Each
>> of those leaf partitions may have different attribute numbers for the
>> partitioning columns, so we will need to do the mapping, which actually we
>> do even today. With this patch however, we apply mapping to the generated
>> partition constraint so that it no longer bears the original parent's
>> attribute numbers but those of the table being attached. Down below where
>> we map to the leaf partition's attribute numbers, we still pass the root
>> partitioned table as the parent. But it may so happen that the attnos
>> appearing in the Vars can no longer be matched with any of the root
>> table's attribute numbers, resulting in the following code in
>> map_variable_attnos_mutator() to trigger an error:
>>
>> if (attno > context->map_length || context->attno_map[attno - 1] == 0)
>> elog(ERROR, "unexpected varattno %d in expression to be mapped",
>> attno);
>>
>> Consider this example:
>>
>> root: (a, b, c) partition by list (a)
>> intermediate: (b, c, ..dropped.., a) partition by list (b)
>> leaf: (b, c, a) partition of intermediate
>>
>> When attaching intermediate to root, we will generate the partition
>> constraint and after mapping, its Vars will have attno = 4. When trying
>> to map the same for leaf, we currently do map_partition_varattnos(expr, 1,
>> leaf, root). So, the innards of map_variable_attnos will try to look for
>> an attribute with attno = 4 in root which there isn't, so the above error
>> will occur. We should really be passing intermediate as parent to the
>> mapping routine. With the previous patch's approach, we would pass root
>> as the parent along with partConstraintOrig which would bear the root
>> parent's attnos.
>
> Thanks for the explanation. So, your earlier patch did map Vars
> correctly for the leaf partitions of the table being attached.
Yes I think.
>> Please find attached the updated patch. In addition to the already
>> described fixes, the patch also rearranges code so that a redundant AT
>> work queue entry is avoided. (Currently, we end up creating one for
>> attachRel even if it's partitioned, although it's harmless because
>> ATRewriteTables() knows to skip partitioned tables.)
>
> We are calling find_all_inheritors() on attachRel twice in this function, once
> to avoid circularity and second time for scheduling a scan. Why can't call it
> once and reuse the result?
Hmm, avoiding calling it twice would be a good idea.
Also, I noticed that there might be a deadlock hazard here due to the
lock-strength-upgrade thing happening here across the two calls. In the
first call to find_all_inheritors(), we request AccessShareLock, whereas
in the second, an AccessExclusiveLock. That ought to be fixed anyway I'd
think.
So, the first (and the only after this change) call will request an
AccessExclusiveLock, even though we may not scan the leaf partitions if a
suitable constraint exists on the table being attached. Note that
previously, we would not have exclusive-locked the leaf partitions in such
a case, although it was deadlock-prone/buggy anyway.
The updated patch includes this fix.
> On the default partitioning thread [1] Robert commented that we should try to
> avoid queueing the subpartitions which have constraints that imply the new
> partitioning constraint. I think that comment applies to this code, which the
> refactoring patch has moved into a function. If you do this, instead of
> duplicating the code to gather existing constraints, please create a function
> for gathering constraints of a given relation and use it for the table being
> attached as well as its partitions. Also, we should avoid matching
> constraints for the table being attached twice, when it's not
> partitioned.
I guess you are talking about the case where the table being attached
itself does not have a check constraint that would help avoid the scan,
but its individual leaf partitions (if any) may.
> 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? I tend to agree if that helps avoid muddying the
default partition discussion with this refactoring work.
>
> + /*
> + * Adjust the constraint to match this partition.
> + *
> + * Since partConstraint contains attachRel's attnos due to the
> + * mapping we did just before attempting the proof above, we pass
> + * attachRel as the parent to map_partition_varattnos, not 'rel'
> + * which is the root parent.
> + */
> May be reworded as "Adjust the partition constraints constructed for the table
> being attached for the leaf partition being validated."
Done, although worded slightly differently: Adjust the constraint that we
constructed above for the table being attached so that it matches this
partition's attribute numbers.
> +CREATE TABLE part_7 (
> + a int,
> + b char,
> + 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,
> + b char,
> + a int, -- '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)
> +);
> Why not to use LIKE list_parted as you have done for part_6?
Sure, done.
Please find the updated patch.
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] 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
