On 2018/07/27 12:14, Tom Lane wrote:
> David Rowley <david.row...@2ndquadrant.com> writes:
>> On 27 July 2018 at 13:35, Amit Langote <langote_amit...@lab.ntt.co.jp> wrote:
>>> On 2018/07/27 1:28, Tom Lane wrote:
>>>> (BTW, I'm not sure that it was wise to design bms_add_range to fail for
>>>> empty ranges. Maybe it'd be better to redefine it as a no-op for
>>>> upper < lower?)
>
>>> FWIW, I was thankful that David those left those checks there, because it
>>> helped expose quite a few bugs when writing this code or perhaps that was
>>> his intention to begin with, but maybe he thinks differently now (?).
>
>> I think it's more useful to keep as a bug catcher, although I do
>> understand the thinking behind just having it be a no-op.
>
> Well, my thinking is that it helps nobody if call sites have to have
> explicit workarounds for a totally-arbitrary refusal to handle edge
> cases in the primitive functions. I do not think that is good software
> design. If you want to have assertions that particular call sites are
> specifying nonempty ranges, put those in the call sites where it's
> important. But as-is, this seems like, say, defining foreach() to
> blow up on an empty list.
How about adding Asserts to that effect in partprune.c and execPartition.c
where they're not present? These are the only two files where it's
currently being used, given that it was invented only recently and exactly
for use by the new pruning code. I updated the patch that I posted in the
last email to add those Asserts.
FWIW, I can see at least the guard against < 0 values in number of other
bms_* functions, but I won't be the one to make a call one way or the
other about whether to change bms_add_range() to cope with erroneous inputs.
Thanks,
Amit
diff --git a/src/backend/executor/nodeAppend.c
b/src/backend/executor/nodeAppend.c
index 5ce4fb43e1..86a68d3020 100644
--- a/src/backend/executor/nodeAppend.c
+++ b/src/backend/executor/nodeAppend.c
@@ -171,6 +171,7 @@ ExecInitAppend(Append *node, EState *estate, int eflags)
{
/* We'll need to initialize all subplans */
nplans = list_length(node->appendplans);
+ Assert(nplans > 0);
validsubplans = bms_add_range(NULL, 0, nplans - 1);
}
@@ -179,7 +180,10 @@ ExecInitAppend(Append *node, EState *estate, int eflags)
* immediately, preventing later calls to
ExecFindMatchingSubPlans.
*/
if (!prunestate->do_exec_prune)
+ {
+ Assert(nplans > 0);
appendstate->as_valid_subplans = bms_add_range(NULL, 0,
nplans - 1);
+ }
}
else
{
@@ -189,6 +193,7 @@ ExecInitAppend(Append *node, EState *estate, int eflags)
* When run-time partition pruning is not enabled we can just
mark all
* subplans as valid; they must also all be initialized.
*/
+ Assert(nplans > 0);
appendstate->as_valid_subplans = validsubplans =
bms_add_range(NULL, 0, nplans - 1);
appendstate->as_prune_state = NULL;
diff --git a/src/backend/executor/nodeMergeAppend.c
b/src/backend/executor/nodeMergeAppend.c
index 64025733de..be43014cb8 100644
--- a/src/backend/executor/nodeMergeAppend.c
+++ b/src/backend/executor/nodeMergeAppend.c
@@ -131,6 +131,7 @@ ExecInitMergeAppend(MergeAppend *node, EState *estate, int
eflags)
{
/* We'll need to initialize all subplans */
nplans = list_length(node->mergeplans);
+ Assert(nplans > 0);
validsubplans = bms_add_range(NULL, 0, nplans - 1);
}
@@ -139,7 +140,10 @@ ExecInitMergeAppend(MergeAppend *node, EState *estate, int
eflags)
* immediately, preventing later calls to
ExecFindMatchingSubPlans.
*/
if (!prunestate->do_exec_prune)
+ {
+ Assert(nplans > 0);
mergestate->ms_valid_subplans = bms_add_range(NULL, 0,
nplans - 1);
+ }
}
else
{
@@ -149,6 +153,7 @@ ExecInitMergeAppend(MergeAppend *node, EState *estate, int
eflags)
* When run-time partition pruning is not enabled we can just
mark all
* subplans as valid; they must also all be initialized.
*/
+ Assert(nplans > 0);
mergestate->ms_valid_subplans = validsubplans =
bms_add_range(NULL, 0, nplans - 1);
mergestate->ms_prune_state = NULL;
diff --git a/src/backend/partitioning/partprune.c
b/src/backend/partitioning/partprune.c
index 354eb0d4e6..630b6d58e9 100644
--- a/src/backend/partitioning/partprune.c
+++ b/src/backend/partitioning/partprune.c
@@ -486,7 +486,10 @@ get_matching_partitions(PartitionPruneContext *context,
List *pruning_steps)
/* If there are no pruning steps then all partitions match. */
if (num_steps == 0)
+ {
+ Assert(context->nparts > 0);
return bms_add_range(NULL, 0, context->nparts - 1);
+ }
/*
* Allocate space for individual pruning steps to store its result.
Each
@@ -2048,8 +2051,12 @@ get_matching_hash_bounds(PartitionPruneContext *context,
bms_make_singleton(rowHash % greatest_modulus);
}
else
+ {
+ /* Getting here means at least one hash partition exists. */
+ Assert(boundinfo->ndatums > 0);
result->bound_offsets = bms_add_range(NULL, 0,
boundinfo->ndatums - 1);
+ }
/*
* There is neither a special hash null partition or the default hash
@@ -2128,6 +2135,7 @@ get_matching_list_bounds(PartitionPruneContext *context,
*/
if (nvalues == 0)
{
+ Assert(boundinfo->ndatums > 0);
result->bound_offsets = bms_add_range(NULL, 0,
boundinfo->ndatums - 1);
result->scan_default = partition_bound_has_default(boundinfo);
@@ -2140,6 +2148,7 @@ get_matching_list_bounds(PartitionPruneContext *context,
/*
* First match to all bounds. We'll remove any matching datums
below.
*/
+ Assert(boundinfo->ndatums > 0);
result->bound_offsets = bms_add_range(NULL, 0,
boundinfo->ndatums - 1);
@@ -2250,6 +2259,7 @@ get_matching_list_bounds(PartitionPruneContext *context,
break;
}
+ Assert(minoff >= 0 && maxoff >= 0);
result->bound_offsets = bms_add_range(NULL, minoff, maxoff);
return result;
}
@@ -2327,6 +2337,7 @@ get_matching_range_bounds(PartitionPruneContext *context,
maxoff--;
result->scan_default = partition_bound_has_default(boundinfo);
+ Assert(minoff >= 0 && maxoff >= 0);
result->bound_offsets = bms_add_range(NULL, minoff, maxoff);
return result;
@@ -2995,7 +3006,11 @@ perform_pruning_combine_step(PartitionPruneContext
*context,
{
PartitionBoundInfo boundinfo = context->boundinfo;
- result->bound_offsets = bms_add_range(NULL, 0,
boundinfo->ndatums - 1);
+ if (boundinfo->ndatums > 0)
+ result->bound_offsets = bms_add_range(NULL, 0,
+
boundinfo->ndatums - 1);
+ else
+ result->bound_offsets = NULL;
result->scan_default = partition_bound_has_default(boundinfo);
result->scan_null = partition_bound_accepts_nulls(boundinfo);
return result;
