public inbox for [email protected]
help / color / mirror / Atom feedFrom: Greg Burd <[email protected]>
To: Ranier Vilela <[email protected]>
Cc: Daniel Gustafsson <[email protected]>
Cc: David Rowley <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: [PATCH] Add tests for Bitmapset
Date: Wed, 8 Oct 2025 14:48:03 -0400
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAEudQAq_zOSA2NUQSWePTGV_=90Uw0WcXxGOWnN-vwF046OOqA@mail.gmail.com>
References: <CAEudQAq_zOSA2NUQSWePTGV_=90Uw0WcXxGOWnN-vwF046OOqA@mail.gmail.com>
On Oct 8 2025, at 8:10 am, Ranier Vilela <[email protected]> wrote:
> Hi.
>
>
>> Em sex., 3 de out. de 2025 às 09:13, Greg Burd <[email protected]> escreveu:
>>
>>>
>>> On Oct 3 2025, at 4:25 am, Daniel Gustafsson <[email protected]> wrote:
>>>
>>>>> On 3 Oct 2025, at 01:36, David Rowley <[email protected]> wrote:
>>>>>
>>>>> On Fri, 3 Oct 2025 at 01:33, Daniel Gustafsson <[email protected]> wrote:
>>>>>> Another nitpick would be to remove the test for NULL in test_bms_make_singleton
>>>>>> since that is a STRICT function, making the test for NULL
>>>>>> superfluous code:
>>>>>
>>>>> I see test_random_operations() is also strict. Is it worth getting rid
>>>>> of the SQL NULL checks on the inputs there too? Aka, the attached.
>>>>
>>>> Indeed, but reading the code I wonder if STRICT was a mistake and
>>>> the intention
>>>> was to allow NULL input?
>>>
>>> Yes, it was an oversight after I re-worked the random function.
>>>
>>>> That being said, the function is never called with
>>>> NULL so that's mostly academic thinking. +1 for removing the NULL
>>>> checks and simplifying the code.
>>>
>>> I agree, and thank you both for the attention to detail and interest in
>>> this test suite.
>> With the patch attached, there are regression.
>> Is it intentional not to check the return of the function bms_is_member?
>>
>> diff --strip-trailing-cr -U3
>> C:/dll/postgres_dev/postgres_commit/src/test/modules/test_bitmapset/expected/test_bitmapset.out C:/dll/postgres_dev/postgres_commit/build/testrun/test_bitmapset/regress/results/test_bitmapset.out
>> ---
>> C:/dll/postgres_dev/postgres_commit/src/test/modules/test_bitmapset/expected/test_bitmapset.out
>> 2025-10-02 21:17:53.169515700 -0300
>> +++
>> C:/dll/postgres_dev/postgres_commit/build/testrun/test_bitmapset/regress/results/test_bitmapset.out
>> 2025-10-07 21:24:00.534142300 -0300
>> @@ -1570,9 +1570,5 @@
>>
>> -- random operations
>> SELECT test_random_operations(-1, 10000, 81920, 0) > 0 AS result;
>> - result
>> ---------
>> - t
>> -(1 row)
>> -
>> +ERROR: union missing member 63904
>> DROP EXTENSION test_bitmapset;
>>
>> best regards,
>> Ranier Vilela
Thanks Ranier for the report.
You're correct, the tests I wrote overlooked testing the return value
and so were not accomplishing the goal of testing at all.
Adding your small suggested patched turned over another flaw. The error
message you were seeing was from later in that same function.
> +ERROR: union missing member 63904
This was emitted by the code in the second portion of the randomized
testing where there is a loop doing add/del/test operations. That error
message was misleading as it matched the earlier error message near the
change you made.
But, thankfully that pointed to a second (face palm) flaw in that
function (again, I take full credit/blame). The add/del/test loop
wasn't accumulating the anticipated set of members and so was testing if
the latest random int generated was in the set or not, which makes no
sense. I've updated that loop to retain the list of members and the
test to check against the retained list of members. I've updated the
log message to be different from those above to avoid similar confusion
in the future.
This seems to be passing now, apologies for not paying closer attention
to this code earlier on.
best.
-greg
Attachments:
[application/octet-stream] v1-0001-Correct-flaws-in-randomized-Bitmapset-tests.patch (3.1K, 2-v1-0001-Correct-flaws-in-randomized-Bitmapset-tests.patch)
download | inline diff:
From ecfe770d4ef8139d6f852dde5037a7eb51715e89 Mon Sep 17 00:00:00 2001
From: Greg Burd <[email protected]>
Date: Wed, 8 Oct 2025 14:27:30 -0400
Subject: [PATCH v1] Correct flaws in randomized Bitmapset tests
There were two oversights in the function designated to perform
randomized tests against Bitmapset. First, the test validating
bms_union() was not checking the return from bms_is_member() and so
wasn't testing much at all. Second, in the second half of this function
performing randomized operations there was no tracking of what members
should exist after some number of random add/del operations. This patch
addresses both of those issues.
Reported-by: Ranier Vilela<[email protected]>
Discussion: https://postgr.es/m/CAApHDvqghMnm_zgSNefto9oaEJ0S-3Cgb3gdsV7XvLC-hMS02Q@mail.gmail.com
---
.../modules/test_bitmapset/test_bitmapset.c | 31 ++++++++++++++-----
1 file changed, 23 insertions(+), 8 deletions(-)
diff --git a/src/test/modules/test_bitmapset/test_bitmapset.c b/src/test/modules/test_bitmapset/test_bitmapset.c
index 8bc9b1f48e9..00651cb8df1 100644
--- a/src/test/modules/test_bitmapset/test_bitmapset.c
+++ b/src/test/modules/test_bitmapset/test_bitmapset.c
@@ -624,10 +624,8 @@ test_random_operations(PG_FUNCTION_ARGS)
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 */
@@ -635,6 +633,8 @@ test_random_operations(PG_FUNCTION_ARGS)
{
member = pg_prng_uint32(&state) % max_range + min_value;
+ if (!bms_is_member(member, bms1) && !bms_is_member(member, bms2))
+ members[num_members++] = member;
bms2 = bms_add_member(bms2, member);
}
@@ -683,24 +683,39 @@ test_random_operations(PG_FUNCTION_ARGS)
bms_free(bms1);
bms_free(bms2);
- for (int i = 0; i < num_ops; i++)
+ num_members = 0;
+ members = palloc(sizeof(int) * num_ops);
+
+ 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);
+ num_members--;
+ memmove(members + pos, members + pos + 1,
+ (num_members - pos) * sizeof(int));
}
break;
case 2: /* test membership */
- if (bms != NULL)
+ /* Verify 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;
}
--
2.49.0
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