On 2019-Jul-03, Amit Langote wrote:
> Hosoya-san,
>
> Thanks for updating the patches.
>
> I have no comment in particular about
> v4_default_partition_pruning.patch,
Cool, thanks. I spent some time reviewing this patch (the first one)
and I propose the attached cosmetic changes. Mostly they consist of a
few comment rewordings.
There is one Assert() that changed in a pretty significant way ...
innocent though the change looks. The original (not Hosoya-san's
patch's fault) had an assertion which is being changed thus:
minoff = 0;
maxoff = boundinfo->ndatums;
...
if (partindices[minoff] < 0)
minoff++;
if (partindices[maxoff] < 0)
maxoff--;
result->scan_default = partition_bound_has_default(boundinfo);
- Assert(minoff >= 0 && maxoff >= 0);
+ Assert(partindices[minoff] >= 0 &&
+ partindices[maxoff] >= 0);
Note that the original Assert() here was verifying whether minoff and
maxoff are both >= 0. But that seems pretty useless since it seems
almost impossible to have them become that given what we do to them.
What I think this code *really* wants to check is whether *the partition
indexes* that they point to are not negative: the transformation that
the two "if" lines do means to ignore bounds that correspond to value
ranges uncovered by any partition. And after the incr/decr operators,
we expect that the bounds will be those of existing partitions ... so
they shouldn't be -1.
Other changes are addition of braces to some one-line blocks that had
significant comments, and minor code rearrangements to make things look
more easily understandable.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/partitioning/partprune.c b/src/backend/partitioning/partprune.c
index 533818ce34..bfc16409dd 100644
--- a/src/backend/partitioning/partprune.c
+++ b/src/backend/partitioning/partprune.c
@@ -714,7 +714,7 @@ get_matching_partitions(PartitionPruneContext *context, List *pruning_steps)
PruneStepResult **results,
*final_result;
ListCell *lc;
- bool scan_default;
+ bool scan_default;
/* If there are no pruning steps then all partitions match. */
if (num_steps == 0)
@@ -771,21 +771,27 @@ get_matching_partitions(PartitionPruneContext *context, List *pruning_steps)
{
int partindex = context->boundinfo->indexes[i];
- /*
- * In range and hash partitioning cases, some index values may be -1,
- * indicating that no partition has been defined to accept a given
- * range of values (that is, the bound at given offset is the upper
- * bound of this unassigned range of values) or for a given remainder,
- * respectively. As it's still part of the queried range of values,
- * add the default partition, if any.
- */
- if (partindex >= 0)
- result = bms_add_member(result, partindex);
- else
+ if (partindex < 0)
+ {
+ /*
+ * In range partitioning cases, if a partition index is -1 it
+ * means that the bound at the offset is the upper bound for a
+ * range not covered by any partition (other than a possible
+ * default partition). In hash partitioning, the same means no
+ * partition has been defined for the corresponding remainder
+ * value.
+ *
+ * In either case, the value is still part of the queried range of
+ * values, so mark to scan the default partition if one exists.
+ */
scan_default |= partition_bound_has_default(context->boundinfo);
+ continue;
+ }
+
+ result = bms_add_member(result, partindex);
}
- /* Add the null and/or default partition if needed and if present. */
+ /* Add the null and/or default partition if needed and present. */
if (final_result->scan_null)
{
Assert(context->strategy == PARTITION_STRATEGY_LIST);
@@ -2623,17 +2629,13 @@ get_matching_list_bounds(PartitionPruneContext *context,
* Each datum whose offset is in result is to be treated as the upper bound of
* the partition that will contain the desired values.
*
- * scan_default will be set in the returned struct, if the default partition
- * needs to be scanned, provided one exists at all. Although note that we
- * intentionally don't set scan_default in this function if only because the
- * matching set of values, found by comparing the input values using the
- * specified opstrategy, contains unassigned portions of key space, which
- * we normally assume to belong to the default partition. We don't infer
- * that until all steps have been executed, including any combination steps,
- * because even if such unassinged portion of key space appears to be part of
- * the set of queried values based on comparisons in the individual OpExprs,
- * the final set of bounds obtained after combining multiple OpExprs may
- * exclude it.
+ * scan_default is set in the returned struct if a default partition exists
+ * and we're absolutely certain that it needs to be scanned. We do *not* set
+ * it just because values match portions of the key space uncovered by
+ * partitions other than default (space which we normally assume to belong to
+ * the default partition): the final set of bounds obtained after combining
+ * multiple pruning steps might exclude it, so we infer its inclusion
+ * elsewhere.
*
* 'opstrategy' if non-zero must be a btree strategy number.
*
@@ -2689,13 +2691,15 @@ get_matching_range_bounds(PartitionPruneContext *context,
*/
if (nvalues == 0)
{
+ /* ignore key space not covered by any partitions */
if (partindices[minoff] < 0)
minoff++;
if (partindices[maxoff] < 0)
maxoff--;
result->scan_default = partition_bound_has_default(boundinfo);
- Assert(minoff >= 0 && maxoff >= 0);
+ Assert(partindices[minoff] >= 0 &&
+ partindices[maxoff] >= 0);
result->bound_offsets = bms_add_range(NULL, minoff, maxoff);
return result;
@@ -2896,16 +2900,18 @@ get_matching_range_bounds(PartitionPruneContext *context,
minoff = inclusive ? off : off + 1;
}
-
- /*
- * lookup value falls in the range between some bounds in
- * boundinfo. off would be the offset of the greatest bound
- * that is <= lookup value, so add off + 1 to the result
- * instead as the offset of the upper bound of the smallest
- * partition that may contain the lookup value.
- */
else
+ {
+
+ /*
+ * lookup value falls in the range between some bounds in
+ * boundinfo. off would be the offset of the greatest
+ * bound that is <= lookup value, so add off + 1 to the
+ * result instead as the offset of the upper bound of the
+ * smallest partition that may contain the lookup value.
+ */
minoff = off + 1;
+ }
}
break;
@@ -2972,11 +2978,13 @@ get_matching_range_bounds(PartitionPruneContext *context,
maxoff = off;
}
else
+ {
/*
* 'off' is -1 indicating that all bounds are greater, so just
* set the first bound's offset as maxoff.
*/
maxoff = off + 1;
+ }
break;
default:
@@ -3265,23 +3273,24 @@ perform_pruning_combine_step(PartitionPruneContext *context,
/*
* A combine step without any source steps is an indication to not perform
- * any partition pruning, we just return all datum offsets.
+ * any partition pruning. Return all datum indexes in that case.
*/
result = (PruneStepResult *) palloc0(sizeof(PruneStepResult));
if (list_length(cstep->source_stepids) == 0)
{
PartitionBoundInfo boundinfo = context->boundinfo;
+ int rangemax;
/*
* Add all valid offsets into the boundinfo->indexes array. For range
* partitioning, boundinfo->indexes contains (boundinfo->ndatums + 1)
- * valid entries.
+ * valid entries; otherwise there are boundinfo->ndatums.
*/
- if (context->strategy == PARTITION_STRATEGY_RANGE)
- result->bound_offsets = bms_add_range(NULL, 0, boundinfo->ndatums);
- else
- result->bound_offsets = bms_add_range(NULL, 0,
- boundinfo->ndatums - 1);
+ rangemax = context->strategy == PARTITION_STRATEGY_RANGE ?
+ boundinfo->ndatums : boundinfo->ndatums - 1;
+
+ result->bound_offsets =
+ bms_add_range(result->bound_offsets, 0, rangemax);
result->scan_default = partition_bound_has_default(boundinfo);
result->scan_null = partition_bound_accepts_nulls(boundinfo);
return result;
diff --git a/src/include/partitioning/partbounds.h b/src/include/partitioning/partbounds.h
index c7535e32fc..160b319e0a 100644
--- a/src/include/partitioning/partbounds.h
+++ b/src/include/partitioning/partbounds.h
@@ -56,7 +56,6 @@
* pointed by remainder produced when hash value of the datum-tuple is divided
* by the greatest modulus.
*/
-
typedef struct PartitionBoundInfoData
{
char strategy; /* hash, list or range? */
