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 likeif (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
Attachments:
partprune-check-zero-bounds-fix.patchtext/plain; charset=UTF-8; name=partprune-check-zero-bounds-fix.patchDownload
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;
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
Attachments:
partprune-check-zero-bounds-fix-v2.patchtext/plain; charset=UTF-8; name=partprune-check-zero-bounds-fix-v2.patchDownload
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;
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