Re: negative bitmapset member not allowed Error with partition pruning

On 2018/07/27 1:28, Tom Lane wrote:
> Rajkumar Raghuwanshi <rajkumar(dot)raghuwanshi(at)enterprisedb(dot)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