public inbox for [email protected]
help / color / mirror / Atom feedFrom: David Rowley <[email protected]>
To: Michael Paquier <[email protected]>
Cc: Greg Burd <[email protected]>
Cc: Ranier Vilela <[email protected]>
Cc: Daniel Gustafsson <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: [PATCH] Add tests for Bitmapset
Date: Sat, 18 Apr 2026 21:06:02 +1200
Message-ID: <CAApHDvrDW9W72vAr7h7XeCu7+Qz-_Vff02Q+RPPuVeM0Qf0MCw@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CAEudQAq_zOSA2NUQSWePTGV_=90Uw0WcXxGOWnN-vwF046OOqA@mail.gmail.com>
<[email protected]>
<CAApHDvqXdBWMwGuzJBSbq4eiNLDaR738-upd8NJpmhUq3a+XLQ@mail.gmail.com>
<[email protected]>
<CAApHDvpn3bshX+_2t0NbJ263r1etcBdmkROjuJw8DvyZkMcXqg@mail.gmail.com>
<[email protected]>
<CAApHDvp=pEkO6LKfXKDjRNeQ1LkV7A5fsTn29ahUBXxmjyWPHg@mail.gmail.com>
<[email protected]>
On Fri, 10 Oct 2025 at 11:30, Michael Paquier <[email protected]> wrote:
> Thanks for double-checking. Applied after running an indent.
I was working on test_bitmapset.c to add some tests for a new
bitmapset function. I noticed a few weird things.
1. test_random_operations() is coded to use GetCurrentTimestamp() as a
seed when the given seed is <= 0. Of course, it'll be a while before
the return value of that wraps beyond 2^63 (292250 years), but I still
can't help but think that NULL is a better value to use to have the
seed auto-generate.
2. Doing #1 means the function can't be STRICT. I do think it's wrong
that the function is marked as strict. That's normally reserved for
functions that we needn't call because NULL input(s) yield a NULL
output. That's not the case for this function.
3. There's no CHECK_FOR_INTERRUPTS() in test_random_operations(). If
someone uses a large num_ops, there's no way to cancel the query.
4. If there happened to be some rare bug in bitmapset.c that
test_random_operations() we might struggle to find it again, as we
don't report which seed we used in the ERROR message.
5. Couple of minor cosmetics; header order, whilespace.
I felt it was worth fixing these now as the function I plan to add
there does #1, #2, #3 and #4. If I add the new function for v20, the
discrepancy seems questionable.
David
Attachments:
[application/octet-stream] test_bitmapset_fixes.patch (6.4K, 2-test_bitmapset_fixes.patch)
download | inline diff:
diff --git a/src/test/modules/test_bitmapset/expected/test_bitmapset.out b/src/test/modules/test_bitmapset/expected/test_bitmapset.out
index f75cb46b869..0b72b91cd1f 100644
--- a/src/test/modules/test_bitmapset/expected/test_bitmapset.out
+++ b/src/test/modules/test_bitmapset/expected/test_bitmapset.out
@@ -1569,7 +1569,7 @@ SELECT test_bms_nonempty_difference('(b 1 2)', '(b 50 100)') AS result;
(1 row)
-- random operations
-SELECT test_random_operations(-1, 10000, 81920, 0) > 0 AS result;
+SELECT test_random_operations(NULL, 10000, 81920, 0) > 0 AS result;
result
--------
t
diff --git a/src/test/modules/test_bitmapset/sql/test_bitmapset.sql b/src/test/modules/test_bitmapset/sql/test_bitmapset.sql
index e75ab8f620a..c53232e0ada 100644
--- a/src/test/modules/test_bitmapset/sql/test_bitmapset.sql
+++ b/src/test/modules/test_bitmapset/sql/test_bitmapset.sql
@@ -401,6 +401,6 @@ SELECT test_bms_nonempty_difference('(b 100)', '(b 5)') AS result;
SELECT test_bms_nonempty_difference('(b 1 2)', '(b 50 100)') AS result;
-- random operations
-SELECT test_random_operations(-1, 10000, 81920, 0) > 0 AS result;
+SELECT test_random_operations(NULL, 10000, 81920, 0) > 0 AS result;
DROP EXTENSION test_bitmapset;
diff --git a/src/test/modules/test_bitmapset/test_bitmapset--1.0.sql b/src/test/modules/test_bitmapset/test_bitmapset--1.0.sql
index 227ecb5aa3b..e7b263e51f5 100644
--- a/src/test/modules/test_bitmapset/test_bitmapset--1.0.sql
+++ b/src/test/modules/test_bitmapset/test_bitmapset--1.0.sql
@@ -133,8 +133,8 @@ RETURNS int
AS 'MODULE_PATHNAME' LANGUAGE C;
-- Test utility functions
-CREATE FUNCTION test_random_operations(integer, integer, integer, integer)
-RETURNS integer STRICT
+CREATE FUNCTION test_random_operations(bigint, integer, integer, integer)
+RETURNS integer
AS 'MODULE_PATHNAME' LANGUAGE C;
COMMENT ON EXTENSION test_bitmapset IS 'Test code for Bitmapset';
diff --git a/src/test/modules/test_bitmapset/test_bitmapset.c b/src/test/modules/test_bitmapset/test_bitmapset.c
index 272bed390a6..3a185369651 100644
--- a/src/test/modules/test_bitmapset/test_bitmapset.c
+++ b/src/test/modules/test_bitmapset/test_bitmapset.c
@@ -1,7 +1,7 @@
/*-------------------------------------------------------------------------
*
* test_bitmapset.c
- * Test the Bitmapset data structure.
+ * Test the Bitmapset data structure.
*
* This module tests the Bitmapset implementation in PostgreSQL, covering
* all public API functions.
@@ -19,11 +19,12 @@
#include <stddef.h>
#include "catalog/pg_type.h"
#include "common/pg_prng.h"
-#include "utils/array.h"
#include "fmgr.h"
+#include "miscadmin.h"
#include "nodes/bitmapset.h"
#include "nodes/nodes.h"
#include "nodes/pg_list.h"
+#include "utils/array.h"
#include "utils/builtins.h"
#include "utils/timestamp.h"
@@ -587,7 +588,7 @@ test_bitmap_match(PG_FUNCTION_ARGS)
* "min_value" is the minimal value used for the members, that will stand
* up to a range of "max_range". "num_ops" defines the number of time each
* operation is done. "seed" is a random seed used to calculate the member
- * values. When "seed" is <= 0, a random seed will be chosen automatically.
+ * values. When "seed" is NULL, a random seed will be chosen automatically.
*
* The return value is the number of times all operations have been executed.
*/
@@ -608,13 +609,20 @@ test_random_operations(PG_FUNCTION_ARGS)
int num_members = 0;
int total_ops = 0;
- if (PG_GETARG_INT32(0) > 0)
- seed = PG_GETARG_INT32(0);
+ if (!PG_ARGISNULL(0))
+ seed = PG_GETARG_INT64(0);
num_ops = PG_GETARG_INT32(1);
max_range = PG_GETARG_INT32(2);
min_value = PG_GETARG_INT32(3);
+ if (PG_ARGISNULL(1) || num_ops <= 0)
+ elog(ERROR, "invalid number of operations");
+ if (PG_ARGISNULL(2) || max_range <= 0)
+ elog(ERROR, "invalid maximum range");
+ if (PG_ARGISNULL(3) || min_value < 0)
+ elog(ERROR, "invalid minimum value");
+
pg_prng_seed(&state, seed);
/*
@@ -627,6 +635,8 @@ test_random_operations(PG_FUNCTION_ARGS)
/* Phase 1: Random insertions in first set */
for (int i = 0; i < num_ops / 2; i++)
{
+ CHECK_FOR_INTERRUPTS();
+
member = pg_prng_uint32(&state) % max_range + min_value;
if (!bms_is_member(member, bms1))
@@ -637,6 +647,8 @@ test_random_operations(PG_FUNCTION_ARGS)
/* Phase 2: Random insertions in second set */
for (int i = 0; i < num_ops / 4; i++)
{
+ CHECK_FOR_INTERRUPTS();
+
member = pg_prng_uint32(&state) % max_range + min_value;
if (!bms_is_member(member, bms2))
@@ -651,8 +663,11 @@ test_random_operations(PG_FUNCTION_ARGS)
/* Verify union contains all members from first and second sets */
for (int i = 0; i < num_members; i++)
{
+ CHECK_FOR_INTERRUPTS();
+
if (!bms_is_member(members[i], result))
- elog(ERROR, "union missing member %d", members[i]);
+ elog(ERROR, "union missing member %d, seed " INT64_FORMAT,
+ members[i], seed);
}
bms_free(result);
@@ -667,8 +682,11 @@ test_random_operations(PG_FUNCTION_ARGS)
while ((member = bms_next_member(result, member)) >= 0)
{
+ CHECK_FOR_INTERRUPTS();
+
if (!bms_is_member(member, bms1) || !bms_is_member(member, bms2))
- elog(ERROR, "intersection contains invalid member %d", member);
+ elog(ERROR, "intersection contains invalid member %d, seed " INT64_FORMAT,
+ member, seed);
}
bms_free(result);
}
@@ -680,6 +698,8 @@ test_random_operations(PG_FUNCTION_ARGS)
int lower = pg_prng_uint32(&state) % 100;
int upper = lower + (pg_prng_uint32(&state) % 20);
+ CHECK_FOR_INTERRUPTS();
+
result = bms_add_range(result, lower, upper);
}
if (result != NULL)
@@ -699,6 +719,8 @@ test_random_operations(PG_FUNCTION_ARGS)
for (int op = 0; op < num_ops; op++)
{
+ CHECK_FOR_INTERRUPTS();
+
switch (pg_prng_uint32(&state) % 3)
{
case 0: /* add */
@@ -714,7 +736,8 @@ test_random_operations(PG_FUNCTION_ARGS)
member = members[pos];
if (!bms_is_member(member, bms))
- elog(ERROR, "expected %d to be a valid member", member);
+ elog(ERROR, "expected %d to be a valid member, seed " INT64_FORMAT,
+ member, seed);
bms = bms_del_member(bms, member);
@@ -730,7 +753,8 @@ test_random_operations(PG_FUNCTION_ARGS)
for (int i = 0; i < num_members; i++)
{
if (!bms_is_member(members[i], bms))
- elog(ERROR, "missing member %d", members[i]);
+ elog(ERROR, "missing member %d, seed " INT64_FORMAT,
+ members[i], seed);
}
break;
}
view thread (81+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [PATCH] Add tests for Bitmapset
In-Reply-To: <CAApHDvrDW9W72vAr7h7XeCu7+Qz-_Vff02Q+RPPuVeM0Qf0MCw@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox