Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 29 Sep 2025 15:20:18 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Greg Burd <greg@burd.me>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: [PATCH] Add tests for Bitmapset
Message-ID: <aNolInqSzt1d1338@paquier.xyz>
References: <aNR-gsGmLnMaNT5i@paquier.xyz>
 <837A4747-B56C-49FD-B6D0-1C40D79A5C47@greg.burd.me>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="Bz0Vg62wYCF9Tz9S"
Content-Disposition: inline
In-Reply-To: <837A4747-B56C-49FD-B6D0-1C40D79A5C47@greg.burd.me>
Archived-At: 
 <https://www.postgresql.org/message-id/aNolInqSzt1d1338%40paquier.xyz>
Precedence: bulk


--Bz0Vg62wYCF9Tz9S
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Sat, Sep 27, 2025 at 10:03:14AM -0400, Greg Burd wrote:
>=20
> On Sep 24 2025, at 7:28 pm, Michael Paquier <michael@paquier.xyz> wrote:
>=20
> > 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 t=
ree.
> >=20
> > By the way, Greg, do you think that we should aim for a state where we
> > are closer to completion?
>=20
> Hi Michael,
>=20
> We've come this far, why not go all the way? :)
>=20
> > 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.
>=20
> Agreed.
>=20
> > 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
>=20
> Thanks for the detailed analysis, I tried to address each of these in
> the patch.
>=20
> > 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_bi=
tmapset.c.gcov.html
>=20
> Agreed.
>=20
> > The coverage of the latter matters less than the coverage of the
> > former, of course.
>=20
> Agreed.
> test_bitmapset.c
> lines......: 98.9%
> functions..: 100%
> branches...: 82.6%
>=20
> 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.
>=20
>=20
> bitmapset.c
> lines......: 99.8%
> functions..: 100%
> branches...: 89.4%

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.

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

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

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

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.

> 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 set=
s.
>=20
> test_bms_subset_compare('(b 1 64 65)', '(b 1 2 64)') -> BMS_DIFFERENT
>=20
> 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.  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
--
Michael

--Bz0Vg62wYCF9Tz9S
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmjaJSIACgkQnvQgOdby
QH0XSg//YQdSNdKynT+KVHlJjnfbtwXbDrDPR5o3PYaAm/5/UV+CkDo/hNhLqDeX
k+jqK9H+3u9gjFqNLLbBXs0CoI6NXdp90/L/76DOtbNEGQy8ITLrJ9XTwPAcRaLY
7LPch09PffgNjgG7xOTSE9qA4xJCgc+BuEcLHJre7BVVnE/udntQGQof/vsKH+KZ
PJVFSCr2nWsKYQDoNC8kp+jQDtuS/DxopiBjUdNXGmGuvclMGj5SzXfHdc3dZWrE
zk4ipJFA4kZm8d15TTndXymKvajEADA7GFk8lHoh1Q4+hGvfrN0nvLjKG/InIUtv
fD5THG1gjqTSxnES1lN1bYuttBqh8+n3TRxNYMGWWXUvaE3g1ax+7gkf60c10SWU
aF6EBji7dVkzelFXOTDg7VK2ERRm7iG7gbbdiDTlT7YLhuyC9azo8lzZRlxqhLbr
qByx3Dpr3JufDMTEyPx+b2uu0PJmO1io+HK+rb2p05NivqXFSdg/e1c7jltufx8q
52+EiwvRodYVRXQQbLsqNs9nJw+WnGmZGglTwKjlpklPN0yn1kzFt5lG42EPXyVf
i8RmqivYHs7bJdSVzSGWTm02la+ueW6uI9LyHU/pZLGMCStTBgsyexMYteyZCxi5
yStFTnP4n3qulGdN3Lug0gWP34Wec+liMP6rrCBgoZaF/ICtjzg=
=rdPT
-----END PGP SIGNATURE-----

--Bz0Vg62wYCF9Tz9S--