Feedback-ID: i675e48f3:Fastmail
MIME-Version: 1.0
Date: Thu, 11 Sep 2025 06:56:07 -0400
From: "Greg Burd" <greg@burd.me>
To: "Masahiko Sawada" <sawada.mshk@gmail.com>
Cc: "Nathan Bossart" <nathandbossart@gmail.com>,
 "PostgreSQL Hackers" <pgsql-hackers@lists.postgresql.org>
Message-Id: <0978d5ba-e015-4889-a8b7-7a2891664492@app.fastmail.com>
In-Reply-To: 
 <CAD21AoDijQoXik+U8hf0QptPvNcpjCjfzO_Rp=eCLYSL1D6OOQ@mail.gmail.com>
References: <7BD1ABDB-B03A-464A-9BA9-A73B55AD8A1F@getmailspring.com>
 <aLpENihhKL4vOxeX@nathan> <02DB5E92-1E94-4617-AC11-836486F63BD5@burd.me>
 <aLsvPdSHJdRCEk7v@nathan> <9E1E0BA4-952C-412A-884A-6E700F26B0CA@burd.me>
 <CAD21AoDijQoXik+U8hf0QptPvNcpjCjfzO_Rp=eCLYSL1D6OOQ@mail.gmail.com>
Subject: Re: [PATCH] Add tests for Bitmapset
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/0978d5ba-e015-4889-a8b7-7a2891664492%40app.fastmail.com>
Precedence: bulk


On Thu, Sep 11, 2025, at 2:48 AM, Masahiko Sawada wrote:
> On Mon, Sep 8, 2025 at 11:21=E2=80=AFAM Burd, Greg <greg@burd.me> wrot=
e:
>>
>>
>> > On Sep 5, 2025, at 2:43=E2=80=AFPM, Nathan Bossart <nathandbossart@=
gmail.com> wrote:
>> >
>> > On Fri, Sep 05, 2025 at 10:48:21AM -0400, Burd, Greg wrote:
>> >> I looked at both radix tree and binary heap and how they use rando=
m sets when
>> >> testing.  Binary heap uses it to create different random sets of n=
umbers to
>> >> use across multiple tests while radix tree has a single function t=
hat focuses
>> >> on randomized data.  I decided not to add randomization into the t=
ests of
>> >> Bitmapset simply because I like avoiding non-deterministic behavio=
r.  But in
>> >> tests I guess that can be helpful finding future unknown corner ca=
ses.  I'm
>> >> on the fence as to the value, your call.  :)
>> >
>> > I'm not too concerned about it.  We've lived without a dedicated te=
st suite
>> > for Bitmapset for a very long time, so any amount of test coverage =
is an
>> > improvement.  Like you said, adding some randomization might be hel=
pful for
>> > finding weird bugs we wouldn't have thought to test.  And, given th=
e many,
>> > many machines that run the tests, IMHO it'd only help build even mo=
re
>> > confidence in the code.  If my suggestion inspires you to update th=
e patch,
>> > great, but I'm fine with proceeding with what you already wrote, to=
o.
>>
>> Nathan, thanks for considering the patch.  Honestly, I'm fine with it=
 as is.
>> We can revisit later if needed.  This does what I'd intended, test an=
d document
>> in code the API and implementation making future changes to that more
>> transparent.
>
> I appreciate your work on this. While I agree that adding more tests
> to  bitmapset.c is a good idea, I'm concerned about the minimal
> improvement in test coverage despite the addition of new test cases
> (only three lines of code are newly covered). Apart from adding some
> randomness to the tests we've discussed, given that we're implementing
> a dedicated test module for bitmapset.c, I would expect to see a more
> increase in test coverage.

Good point and thanks for taking the time to reply.  The only thing it i=
dentified
was a small issue fixed in b463288 so I'd not expect coverage to increas=
e much.

I'll take a stab at adding some randomness to the tests and check the de=
lta in
coverage.  Thanks for the nudge.  :)

>
> Regards,
>
> --=20
> Masahiko Sawada
> Amazon Web Services: https://aws.amazon.com

Just for reference I started this not to increase coverage, which is a g=
ood
goal just not the one I had.  I was reviewing the API and considering so=
me
changes based on other work I've done.  Now that I see how deeply baked =
in
this code is I think that's unlikely.  Maybe something else distinct for
bitmaps over 64-bit space at some point will be useful.  I wrote this co=
de
just to capture the API in test form.

best,

-greg