From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Amit Langote <amitlangote09@gmail.com>
Cc: yuzuko <yuzukohosoya@gmail.com>
Cc: shawn wang <shawn.wang.pg@gmail.com>
Cc: Shawn Wang <shawn.wang@highgo.ca>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Problem with default partition pruning
Date: Sun, 4 Aug 2019 02:29:25 -0400
Message-ID: <20190804062925.GA5861@alvherre.pgsql> (raw)
In-Reply-To: <CA+HiwqG+nSD0vcJacArYgYcFVtpTJQ0fx1gBgoZkA_isKd6Z2w@mail.gmail.com>
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
Attachments:
[text/x-diff] v4-delta.patch (6.9K, ../20190804062925.GA5861@alvherre.pgsql/2-v4-delta.patch)
download | inline diff:diff --git a/src/backend/partitioning/partprune.c b/src/backend/partitioning/partprune.cindex 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.hindex 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? */
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: alvherre@2ndquadrant.com, amitlangote09@gmail.com, yuzukohosoya@gmail.com, shawn.wang.pg@gmail.com, shawn.wang@highgo.ca, pgsql-hackers@lists.postgresql.org
Subject: Re: Problem with default partition pruning
In-Reply-To: <20190804062925.GA5861@alvherre.pgsql>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox