public inbox for [email protected]
help / color / mirror / Atom feedFrom: Michael Paquier <[email protected]>
To: David Rowley <[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: Thu, 9 Oct 2025 12:47:31 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAApHDvpn3bshX+_2t0NbJ263r1etcBdmkROjuJw8DvyZkMcXqg@mail.gmail.com>
References: <CAEudQAq_zOSA2NUQSWePTGV_=90Uw0WcXxGOWnN-vwF046OOqA@mail.gmail.com>
<[email protected]>
<CAApHDvqXdBWMwGuzJBSbq4eiNLDaR738-upd8NJpmhUq3a+XLQ@mail.gmail.com>
<[email protected]>
<CAApHDvpn3bshX+_2t0NbJ263r1etcBdmkROjuJw8DvyZkMcXqg@mail.gmail.com>
On Thu, Oct 09, 2025 at 04:35:55PM +1300, David Rowley wrote:
> On Thu, 9 Oct 2025 at 15:13, Michael Paquier <[email protected]> wrote:
> > What do you think about the attached?
>
> Thanks. Looks pretty good.
>
> > + members = palloc(sizeof(int) * num_ops);
>
> Any reason to pfree that and allocate that to the same size as it already was?
No reason. We can shortcut that a bit.
> Wondering if the "members[pos] = members[--num_members];" is worth a
> short comment. Maybe something like: /* zap this member by moving the
> final array member into its place and shrinking the array by 1 */
Yes, a comment can be adapted here. Sounds good to me.
--
Michael
Attachments:
[text/x-diff] v3-0001-test_bitmapset-Improve-random-function.patch (3.9K, 2-v3-0001-test_bitmapset-Improve-random-function.patch)
download | inline diff:
From d7ba7df53ad8d1c45dadb7e01c47b4c30f5bfb75 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Thu, 9 Oct 2025 12:46:59 +0900
Subject: [PATCH v3] test_bitmapset: Improve random function
---
.../modules/test_bitmapset/test_bitmapset.c | 57 ++++++++++++++-----
1 file changed, 44 insertions(+), 13 deletions(-)
diff --git a/src/test/modules/test_bitmapset/test_bitmapset.c b/src/test/modules/test_bitmapset/test_bitmapset.c
index 8bc9b1f48e9a..abbdde85e0b3 100644
--- a/src/test/modules/test_bitmapset/test_bitmapset.c
+++ b/src/test/modules/test_bitmapset/test_bitmapset.c
@@ -616,25 +616,31 @@ test_random_operations(PG_FUNCTION_ARGS)
min_value = PG_GETARG_INT32(3);
pg_prng_seed(&state, seed);
+
+ /*
+ * There can be up to "num_ops" members added. This is very unlikely
+ * still possible if all the operations hit the "0" case during phase 4
+ * where multiple operation types are mixed together.
+ */
members = palloc(sizeof(int) * num_ops);
- /* Phase 1: Random insertions */
+ /* Phase 1: Random insertions in first set */
for (int i = 0; i < num_ops / 2; i++)
{
member = pg_prng_uint32(&state) % max_range + min_value;
if (!bms_is_member(member, bms1))
- {
members[num_members++] = member;
- bms1 = bms_add_member(bms1, member);
- }
+ bms1 = bms_add_member(bms1, member);
}
- /* Phase 2: Random set operations */
+ /* Phase 2: Random insertions in second set */
for (int i = 0; i < num_ops / 4; i++)
{
member = pg_prng_uint32(&state) % max_range + min_value;
+ if (!bms_is_member(member, bms2))
+ members[num_members++] = member;
bms2 = bms_add_member(bms2, member);
}
@@ -642,7 +648,7 @@ test_random_operations(PG_FUNCTION_ARGS)
result = bms_union(bms1, bms2);
EXPECT_NOT_NULL(result);
- /* Verify union contains all members from first set */
+ /* Verify union contains all members from first and second sets */
for (int i = 0; i < num_members; i++)
{
if (!bms_is_member(members[i], result))
@@ -650,7 +656,10 @@ test_random_operations(PG_FUNCTION_ARGS)
}
bms_free(result);
- /* Test intersection */
+ /*
+ * Test intersection, checking that all the members in the result are from
+ * both the first and second sets.
+ */
result = bms_intersect(bms1, bms2);
if (result != NULL)
{
@@ -679,28 +688,49 @@ test_random_operations(PG_FUNCTION_ARGS)
bms_free(result);
}
- pfree(members);
bms_free(bms1);
bms_free(bms2);
- for (int i = 0; i < num_ops; i++)
+ /*
+ * Phase 4: mix of operations on a single set, cross-checking a bitmap
+ * with a secondary state, "members".
+ */
+ num_members = 0;
+
+ for (int op = 0; op < num_ops; op++)
{
- member = pg_prng_uint32(&state) % max_range + min_value;
switch (pg_prng_uint32(&state) % 3)
{
case 0: /* add */
+ member = pg_prng_uint32(&state) % max_range + min_value;
+ if (!bms_is_member(member, bms))
+ members[num_members++] = member;
bms = bms_add_member(bms, member);
break;
case 1: /* delete */
- if (bms != NULL)
+ if (num_members > 0)
{
+ int pos = pg_prng_uint32(&state) % num_members;
+
+ member = members[pos];
+ if (!bms_is_member(member, bms))
+ elog(ERROR, "expected %d to be a valid member", member);
+
bms = bms_del_member(bms, member);
+
+ /*
+ * Move the final array member at the position of the member
+ * just deleted, reducing the array size by one.
+ */
+ members[pos] = members[--num_members];
}
break;
case 2: /* test membership */
- if (bms != NULL)
+ /* Verify that bitmap contains all members */
+ for (int i = 0; i < num_members; i++)
{
- bms_is_member(member, bms);
+ if (!bms_is_member(members[i], bms))
+ elog(ERROR, "missing member %d", members[i]);
}
break;
}
@@ -708,6 +738,7 @@ test_random_operations(PG_FUNCTION_ARGS)
}
bms_free(bms);
+ pfree(members);
PG_RETURN_INT32(total_ops);
}
--
2.51.0
[application/pgp-signature] signature.asc (833B, 3-signature.asc)
download
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: <[email protected]>
* 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