Re: negative bitmapset member not allowed Error with partition pruning

[Amit Langote]

On 2018/07/27 1:28, Tom Lane wrote:
> Rajkumar Raghuwanshi <rajkumar.raghuwan...@enterprisedb.com> writes:
>> I am getting "ERROR:  negative bitmapset member not allowed" when
>> enable_partition_pruning set to true with below test case.

Thanks Rajkumar.

> Confirmed here.  It's failing in perform_pruning_combine_step,
> which reaches this:
> 
>         result->bound_offsets = bms_add_range(NULL, 0, boundinfo->ndatums - 
> 1);
> 
> with boundinfo->ndatums == 0.  It's not clear to me whether that situation
> should be impossible or not.  If it is valid, perhaps all we need is
> something like
> 
>         if (boundinfo->ndatums > 0)
>             result->bound_offsets = bms_add_range(NULL, 0, boundinfo->ndatums 
> - 1);
>         else
>             result->bound_offsets = NULL;
> 
> although that then opens the question of whether downstream code is
> OK with bound_offsets being empty.

Yeah, this seems to be the only possible fix and I checked that downstream
code is fine with bound_offsets being NULL/empty.  Actually, the code
that's concerned with bound offsets is limited to partprune.c, because we
don't propagate bound offsets themselves outside this file.

I found one more place in get_matching_hash_bounds where I thought maybe
it'd be a good idea to add this check (if ndatums > 0), but then realized
that that would become dead code as the upstream code takes care of the 0
hash partitions case.  So, maybe an Assert (ndatums > 0) would be better.

Attached find a patch that does both.

> (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 (?).

Thanks,
Amit

diff --git a/src/backend/partitioning/partprune.c 
b/src/backend/partitioning/partprune.c
index 354eb0d4e6..8c024773af 100644
--- a/src/backend/partitioning/partprune.c
+++ b/src/backend/partitioning/partprune.c
@@ -2048,8 +2048,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
@@ -2995,7 +2999,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;