On 30 June 2017 at 09:06, Amit Langote <langote_amit...@lab.ntt.co.jp> wrote:
> When testing the patch, I realized that the current code in
> check_new_partition_bound() that checks for range partition overlap had a
> latent bug that resulted in false positives for the new cases that the new
> less restrictive syntax allowed. I spent some time simplifying that code
> while also fixing the aforementioned bug. It's implemented in the
> attached patch 0001.
>
I haven't had time to look at 0002 yet, but looking at 0001, I'm not
convinced that this really represents much of a simplification, but I
do prefer the way it now consistently reports the first overlapping
partition in the error message.
I'm not entirely convinced by this change either:
- if (equal || off1 != off2)
+ if (off2 > off1 + 1 || ((off2 == off1 + 1) && !equal))
Passing probe_is_bound = true to partition_bound_bsearch() will I
think cause it to return equal = false when the upper bound of one
partition equals the lower bound of another, so relying on the "equal"
flag here seems a bit dubious. I think I can just about convince
myself that it works, but not for the reasons stated in the comments.
It also seems unnecessary for this code to be doing 2 binary searches.
I think a better simplification would be to just do one binary search
to find the gap that the lower bound fits in, and then test the upper
bound of the new partition against the lower bound of the next
partition (if there is one), as in the attached patch.
Regards,
Dean
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
new file mode 100644
index 7da2058..96760a0
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -745,78 +745,62 @@ check_new_partition_bound(char *relname,
if (partdesc->nparts > 0)
{
PartitionBoundInfo boundinfo = partdesc->boundinfo;
- int off1,
- off2;
- bool equal = false;
+ int offset;
+ bool equal;
Assert(boundinfo && boundinfo->ndatums > 0 &&
boundinfo->strategy == PARTITION_STRATEGY_RANGE);
/*
- * Firstly, find the greatest range bound that is less
- * than or equal to the new lower bound.
+ * Test whether the new lower bound (which is treated
+ * inclusively as part of the new partition) lies inside an
+ * existing partition, or in a gap.
+ *
+ * If it's in a gap, the next index value will be -1 (the
+ * lower bound of the next partition). This is also true
+ * if there is no next partition, since the index array is
+ * initialised with an extra -1 at the end.
+ *
+ * Note that this also allows for the possibility that the
+ * new lower bound equals an existing upper bound.
*/
- off1 = partition_bound_bsearch(key, boundinfo, lower, true,
- &equal);
+ offset = partition_bound_bsearch(key, boundinfo, lower,
+ true, &equal);
- /*
- * off1 == -1 means that all existing bounds are greater
- * than the new lower bound. In that case and the case
- * where no partition is defined between the bounds at
- * off1 and off1 + 1, we have a "gap" in the range that
- * could be occupied by the new partition. We confirm if
- * so by checking whether the new upper bound is confined
- * within the gap.
- */
- if (!equal && boundinfo->indexes[off1 + 1] < 0)
+ if (boundinfo->indexes[offset + 1] < 0)
{
- off2 = partition_bound_bsearch(key, boundinfo, upper,
- true, &equal);
-
/*
- * If the new upper bound is returned to be equal to
- * the bound at off2, the latter must be the upper
- * bound of some partition with which the new
- * partition clearly overlaps.
- *
- * Also, if bound at off2 is not same as the one
- * returned for the new lower bound (IOW, off1 !=
- * off2), then the new partition overlaps at least one
- * partition.
+ * Check that the new partition will fit in the gap.
+ * For it to fit, the new upper bound must be less than
+ * or equal to the lower bound of the next partition,
+ * if there is one.
*/
- if (equal || off1 != off2)
+ if (offset + 1 < boundinfo->ndatums)
{
- overlap = true;
+ int32 cmpval;
- /*
- * The bound at off2 could be the lower bound of
- * the partition with which the new partition
- * overlaps. In that case, use the upper bound
- * (that is, the bound at off2 + 1) to get the
- * index of that partition.
- */
- if (boundinfo->indexes[off2] < 0)
- with = boundinfo->indexes[off2 + 1];
- else
- with = boundinfo->indexes[off2];
+ cmpval = partition_bound_cmp(key, boundinfo,
+ offset + 1, upper,
+ true);
+ if (cmpval < 0)
+ {
+ /*
+ * The new partition overlaps with the existing
+ * partition between offset + 1 and offset + 2.
+ */
+ overlap = true;
+ with = boundinfo->indexes[offset + 2];
+ }
}
}
else
{
/*
- * Equal has been set to true and there is no "gap"
- * between the bound at off1 and that at off1 + 1, so
- * the new partition will overlap some partition. In
- * the former case, the new lower bound is found to be
- * equal to the bound at off1, which could only ever
- * be true if the latter is the lower bound of some
- * partition. It's clear in such a case that the new
- * partition overlaps that partition, whose index we
- * get using its upper bound (that is, using the bound
- * at off1 + 1).
+ * The new partition overlaps with the existing
+ * partition between offset and offset + 1.
*/
overlap = true;
- with = boundinfo->indexes[off1 + 1];
+ with = boundinfo->indexes[offset + 1];
}
}
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
new file mode 100644
index fb8745b..b6f794e
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -589,7 +589,7 @@ CREATE TABLE part3 PARTITION OF range_pa
CREATE TABLE fail_part PARTITION OF range_parted2 FOR VALUES FROM (10) TO (30);
ERROR: partition "fail_part" would overlap partition "part2"
CREATE TABLE fail_part PARTITION OF range_parted2 FOR VALUES FROM (10) TO (50);
-ERROR: partition "fail_part" would overlap partition "part3"
+ERROR: partition "fail_part" would overlap partition "part2"
-- now check for multi-column range partition key
CREATE TABLE range_parted3 (
a int,
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
