Horton

Thread

Commits

  1. Verify range bounds to bms_add_range when necessary

  2. Change bms_add_range to be a no-op for empty ranges

  1. negative bitmapset member not allowed Error with partition pruning

    Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com> — 2018-07-26T10:10:01Z

    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

  2. Re: negative bitmapset member not allowed Error with partition pruning

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-07-26T16:28:23Z

    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

  3. Re: negative bitmapset member not allowed Error with partition pruning

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-07-27T01:35:34Z

    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

  4. Re: negative bitmapset member not allowed Error with partition pruning

    David Rowley <david.rowley@2ndquadrant.com> — 2018-07-27T02:42:45Z

    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

  5. Re: negative bitmapset member not allowed Error with partition pruning

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-07-27T03:14:45Z

    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

  6. Re: negative bitmapset member not allowed Error with partition pruning

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-07-27T04:23:11Z

    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

  7. Re: negative bitmapset member not allowed Error with partition pruning

    David Rowley <david.rowley@2ndquadrant.com> — 2018-07-27T04:34:40Z

    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

  8. Re: negative bitmapset member not allowed Error with partition pruning

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-07-30T22:54:46Z

    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