Feedback-ID: i675e48f3:Fastmail
Date: Mon, 29 Sep 2025 06:27:01 -0400
From: Greg Burd <greg@burd.me>
To: Michael Paquier <michael@paquier.xyz>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Message-ID: <791EFF06-DE5A-4E91-83E1-4C35550B55DA@getmailspring.com>
In-Reply-To: <aNolInqSzt1d1338@paquier.xyz>
References: <aNolInqSzt1d1338@paquier.xyz>
Subject: Re: [PATCH] Add tests for Bitmapset
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline
Archived-At: 
 <https://www.postgresql.org/message-id/791EFF06-DE5A-4E91-83E1-4C35550B55DA%40getmailspring.com>
Precedence: bulk


On Sep 29 2025, at 2:20 am, Michael Paquier <michael@paquier.xyz> wrote:

> On Sat, Sep 27, 2025 at 10:03:14AM -0400, Greg Burd wrote:
>> 
>> On Sep 24 2025, at 7:28 pm, Michael Paquier <michael@paquier.xyz> wrote:
>> 
>> > On Wed, Sep 24, 2025 at 07:39:59AM -0400, Greg Burd wrote:
>> >> Thanks Michael, Tom, for the help getting this into shape and in
>> the tree.
>> > 
>> > By the way, Greg, do you think that we should aim for a state where we
>> > are closer to completion?
>> 
>> Hi Michael,
>> 
>> We've come this far, why not go all the way? :)
>> 
>> > We have the module up to the point where we
>> > are in pretty good shape, with most things and the infrastructure done
>> > but it can be improved a bit more, as well.
>> 
>> Agreed.
>> 
>> > Based on the information provided by the coverage report at
>> > https://coverage.postgresql.org/src/backend/nodes/bitmapset.c.gcov.html,
>> > we still have the following things:
>> > - bms_equal for different word counts
>> > - bms_union, bms_nonempty_difference, bms_is_subset and bms_intersect
>> > with shorter word counts.
>> > - bms_different with different word counts
>> > - A couple more cases with bms_subset_compare
>> > - bms_member_index and word counts
>> > - bms_overlap_list with negative number in input list.
>> > - bms_singleton_member ERROR with empty input.
>> > - bms_get_singleton_member with NULL input
>> > - bms_del_member with word counts
>> > - bms_replace_members and repalloc case
>> > - bms_add_range, bms_join and bms_del_members, more word count cases
>> > - bms_prev_member and the prevbit business
>> 
>> Thanks for the detailed analysis, I tried to address each of these in
>> the patch.
>> 
>> > There is not much we can do with the random function, still we could
>> > do something about the NULL paths in the internal functions:
>> > https://coverage.postgresql.org/src/test/modules/test_bitmapset/test_bitmapset.c.gcov.html
>> 
>> Agreed.
>> 
>> > The coverage of the latter matters less than the coverage of the
>> > former, of course.
>> 
>> Agreed.
>> test_bitmapset.c
>> lines......: 98.9%
>> functions..: 100%
>> branches...: 82.6%
>> 
>> Here I removed a bit of code I thought was unnecessary and added a few
>> test cases.  The coverage is better overall, not much to say about it.
>> 
>> 
>> bitmapset.c
>> lines......: 99.8%
>> functions..: 100%
>> branches...: 89.4%

Hello Michael,

> No need for three tests for the word count case in bms_del_member(),
> only one is enough, but I've let them as they you have proposed, as
> you wanted to check more positioning with the members.  These are
> cheap, that's fine.

Yes, redundant.  Thanks for leaving them.

> Added a comment for bms_del_members() for the two cases that trigger
> word count changes.

Thanks, I considered adding comments to cases that were more subtle but
didn't find the time to review/add them.  I appreciate that you added that.

> The test changed with test_bms_difference() is indeed long, but not
> that bad with 140-ish characters, so left it as-is.

Thanks.

> Good catch with bms_del_members() that was missing.  There are so many
> APIs that it's easy to miss one.

Indeed, but it's covered now.

> The changes in test_bms_get_singleton_member() don't seem adapted,
> because we also want to test if the default value given by the
> function caller is changed, bms_get_singleton_member() returning a
> boolean status.  The patch did not use arg1 at all.  Also, if arg0 is
> NULL, let's return the input value.

Doh, I see what you mean.  Thanks for the adaptation.

>> A few branches were really tricky, my (least) favorite was the
>> bms_subset_compare().  To get to the first BMS_DIFFERENT return you have
>> to have a test case where the shorter bitmap is long enough to force two
>> or more iterations of the loop and in the first iteration the first word
>> in a needs to be a subset of b and then for the second word the opposite
>> where the second word of b is a subset of a and yet there can be no
>> overlapping bits in the two sets and the same number of words in both sets.
>> 
>> test_bms_subset_compare('(b 1 64 65)', '(b 1 2 64)') -> BMS_DIFFERENT
>> 
>> Technically the '(b 64 200)', '(b 1 201)' test case is also
>> BMS_DIFFERENT and so redundant, but I was so happy to find the test case
>> for this branch I kept them both.
> 
> I can see that you have expanded on these quite a bit, impressive.
> One thing that was confusing is that some of the tests already
> existed, so I've slightly reworded things to reduce the diffs.

Excellent, thanks.

> Per my
> measurements, we are at more than 99% in the module and bitmapset.c
> when only running make check in the module, which is nice

Yes, I think that does it.  Thanks so much for the guidance and help
taking this to its logical end.  I greatly appreciate your guidance and time.

> --
> Michael


best.

-greg