Thread: negative bitmapset member not allowed Error with partition pruning
Hi,
I am getting "ERROR: negative bitmapset member not allowed" when enable_partition_pruning set to true with below test case.
[edb@localhost bin]$ ./psql postgres
psql (12devel)
Type "help" for help.postgres=# SET enable_partition_pruning TO on;
SET
postgres=# CREATE TABLE part (a INT, b INT) PARTITION BY LIST(a);
CREATE TABLE
postgres=# CREATE TABLE part_p1 PARTITION OF part FOR VALUES IN (-2,-1,0,1,2);
CREATE TABLE
postgres=# CREATE TABLE part_p2 PARTITION OF part DEFAULT PARTITION BY RANGE(a);
CREATE TABLE
postgres=# CREATE TABLE part_p2_p1 PARTITION OF part_p2 DEFAULT;
CREATE TABLE
postgres=# INSERT INTO part VALUES (-1,-1),(1,1),(2,NULL),(NULL,-2),(NULL,NULL);
INSERT 0 5
postgres=# EXPLAIN (COSTS OFF)
postgres-# SELECT tableoid::regclass as part, a, b FROM part WHERE a IS NULL ORDER BY 1, 2, 3;
ERROR: negative bitmapset member not allowed
postgres=# SET enable_partition_pruning TO off;
SET
postgres=# EXPLAIN (COSTS OFF)
SELECT tableoid::regclass as part, a, b FROM part WHERE a IS NULL ORDER BY 1, 2, 3;
QUERY PLAN
------------------------------------------------------------------------
Sort
Sort Key: ((part_p1.tableoid)::regclass), part_p1.a, part_p1.b
-> Append
-> Seq Scan on part_p1
Filter: (a IS NULL)
-> Seq Scan on part_p2_p1
Filter: (a IS NULL)
(7 rows)
postgres=#
Thanks & Regards,
Rajkumar Raghuwanshi
QMG, EnterpriseDB Corporation
Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com> writes:
> I am getting "ERROR: negative bitmapset member not allowed" when
> enable_partition_pruning set to true with below test case.
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.
(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?)
regards, tom lane
On 2018/07/27 1:28, Tom Lane wrote: > Rajkumar Raghuwanshi <rajkumar.raghuwanshi@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
Attachment
On 27 July 2018 at 13:35, Amit Langote <Langote_Amit_f8@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. Partition pruning is complex code so I think additional caution is warranted. People are more likely to notice the error and complain. It's likely especially useful with tools like sqlsmith, as I imagine it does not validate the actual results of queries (does it?). but I'm pretty sure that the ERROR would get flagged up. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
David Rowley <david.rowley@2ndquadrant.com> writes:
> On 27 July 2018 at 13:35, Amit Langote <Langote_Amit_f8@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.
regards, tom lane
On 2018/07/27 12:14, Tom Lane wrote: > David Rowley <david.rowley@2ndquadrant.com> writes: >> On 27 July 2018 at 13:35, Amit Langote <Langote_Amit_f8@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
Attachment
On 27 July 2018 at 15:14, Tom Lane <tgl@sss.pgh.pa.us> wrote: > David Rowley <david.rowley@2ndquadrant.com> writes: >> On 27 July 2018 at 13:35, Amit Langote <Langote_Amit_f8@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. Okay, that's a fair point. I agree, adding Asserts at the current call sites seems better. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On 2018-Jul-27, David Rowley wrote: > On 27 July 2018 at 15:14, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > 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. > > Okay, that's a fair point. I agree, adding Asserts at the current > call sites seems better. Given the discussion, I pushed two commits: first, bms_add_range returns the input bms if the range is empty, also adding Rajkumar's test case, which I also verified to reproduce the bug, and passes (for me) with the bms_add_range change. The second commit includes the proposed asserts, but not the change to avoid calling bms_add_range when the range is empty. Thanks! -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services