bms_prev_member won't work correctly if bitmapword is 64-bits

Started by David Rowleyover 7 years ago3 messages
#1David Rowley
david.rowley@2ndquadrant.com
1 attachment(s)

bms_prev_member mistakenly has a hardcoded 24 in the function. This
should really be BITS_PER_BITMAPWORD - 8 so that it is properly set to
56 if someone compiles with 64-bit bitmapwords.

The attached fixes this and also adds a test to exercise the function
a bit. [1]https://coverage.postgresql.org/src/backend/nodes/bitmapset.c.gcov.html indicates there's currently no coverage of this function at
all.

[1]: https://coverage.postgresql.org/src/backend/nodes/bitmapset.c.gcov.html

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Attachments:

0001-Make-bms_prev_member-work-correctly-with-a-64-bit-bi.patchapplication/octet-stream; name=0001-Make-bms_prev_member-work-correctly-with-a-64-bit-bi.patchDownload
From 35aa7596b6d239af432b4caafc6dde4f5aaaa25c Mon Sep 17 00:00:00 2001
From: "dgrowley@gmail.com" <dgrowley@gmail.com>
Date: Sun, 22 Apr 2018 19:17:08 +1200
Subject: [PATCH] Make bms_prev_member work correctly with a 64 bit bitmapword

5c067521 erroneously had coded bms_prev_member assuming that a bitmapword
would always hold 32 bits and started it's search on what it thought was the
highest 8-bits of the word.  This was not the case if bitmapwords were 64
bits.

In passing add a test to exercise this function a little. Previously there was
no coverage at all.
---
 src/backend/nodes/bitmapset.c                 |  2 +-
 src/test/regress/expected/partition_prune.out | 23 +++++++++++++++++++++++
 src/test/regress/sql/partition_prune.sql      | 25 +++++++++++++++++++++++++
 3 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/src/backend/nodes/bitmapset.c b/src/backend/nodes/bitmapset.c
index 9341bf579e..81182f2518 100644
--- a/src/backend/nodes/bitmapset.c
+++ b/src/backend/nodes/bitmapset.c
@@ -1167,7 +1167,7 @@ bms_prev_member(const Bitmapset *a, int prevbit)
 		if (w != 0)
 		{
 			int			result;
-			int			shift = 24;
+			int			shift = BITS_PER_BITMAPWORD - 8;
 			result = wordnum * BITS_PER_BITMAPWORD;
 
 			while ((w >> shift) == 0)
diff --git a/src/test/regress/expected/partition_prune.out b/src/test/regress/expected/partition_prune.out
index 9c65ee001d..3e818edd33 100644
--- a/src/test/regress/expected/partition_prune.out
+++ b/src/test/regress/expected/partition_prune.out
@@ -1747,6 +1747,29 @@ explain (analyze, costs off, summary off, timing off) execute ab_q3 (2, 2);
          Filter: ((b >= $1) AND (b <= $2) AND (a < $0))
 (10 rows)
 
+-- Test a backwards Append scan
+create table list_part (a int) partition by list (a);
+create table list_part1 partition of list_part for values in (1);
+create table list_part2 partition of list_part for values in (2);
+create table list_part3 partition of list_part for values in (3);
+create table list_part4 partition of list_part for values in (4);
+insert into list_part select generate_series(1,4);
+begin;
+-- Don't select an actual value out of the table as the order of the Append's
+-- subnodes may not be stable.
+declare cur SCROLL CURSOR for select 1 from list_part where a > (select 1) and a < (select 4);
+-- move beyond the final row
+move 3 from cur;
+-- Ensure we get two rows.
+fetch backward all from cur;
+ ?column? 
+----------
+        1
+        1
+(2 rows)
+
+commit;
+drop table list_part;
 -- Parallel append
 -- Suppress the number of loops each parallel node runs for.  This is because
 -- more than one worker may run the same parallel node if timing conditions
diff --git a/src/test/regress/sql/partition_prune.sql b/src/test/regress/sql/partition_prune.sql
index b38b39c71e..d8d3e3c47d 100644
--- a/src/test/regress/sql/partition_prune.sql
+++ b/src/test/regress/sql/partition_prune.sql
@@ -359,6 +359,31 @@ execute ab_q3 (1, 8);
 
 explain (analyze, costs off, summary off, timing off) execute ab_q3 (2, 2);
 
+-- Test a backwards Append scan
+create table list_part (a int) partition by list (a);
+create table list_part1 partition of list_part for values in (1);
+create table list_part2 partition of list_part for values in (2);
+create table list_part3 partition of list_part for values in (3);
+create table list_part4 partition of list_part for values in (4);
+
+insert into list_part select generate_series(1,4);
+
+begin;
+
+-- Don't select an actual value out of the table as the order of the Append's
+-- subnodes may not be stable.
+declare cur SCROLL CURSOR for select 1 from list_part where a > (select 1) and a < (select 4);
+
+-- move beyond the final row
+move 3 from cur;
+
+-- Ensure we get two rows.
+fetch backward all from cur;
+
+commit;
+
+drop table list_part;
+
 -- Parallel append
 
 -- Suppress the number of loops each parallel node runs for.  This is because
-- 
2.16.2.windows.1

#2Teodor Sigaev
teodor@sigaev.ru
In reply to: David Rowley (#1)
Re: bms_prev_member won't work correctly if bitmapword is 64-bits

Thank you, pushed

David Rowley wrote:

bms_prev_member mistakenly has a hardcoded 24 in the function. This
should really be BITS_PER_BITMAPWORD - 8 so that it is properly set to
56 if someone compiles with 64-bit bitmapwords.

The attached fixes this and also adds a test to exercise the function
a bit. [1] indicates there's currently no coverage of this function at
all.

[1] https://coverage.postgresql.org/src/backend/nodes/bitmapset.c.gcov.html

--
Teodor Sigaev E-mail: teodor@sigaev.ru
WWW: http://www.sigaev.ru/

#3David Rowley
david.rowley@2ndquadrant.com
In reply to: Teodor Sigaev (#2)
Re: bms_prev_member won't work correctly if bitmapword is 64-bits

On 24 April 2018 at 03:00, Teodor Sigaev <teodor@sigaev.ru> wrote:

Thank you, pushed

Thanks!

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services