Feedback-ID: i0fe9450f:Fastmail
Date: Fri, 19 Sep 2025 15:36:30 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Greg Burd <greg@burd.me>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
	Robert Haas <robertmhaas@gmail.com>,
	Nathan Bossart <nathandbossart@gmail.com>,
	Masahiko Sawada <sawada.mshk@gmail.com>
Subject: Re: [PATCH] Add tests for Bitmapset
Message-ID: <aMz57nHgkLozddYu@paquier.xyz>
References: <aMs1p9qnutgFPGLI@paquier.xyz>
 <1D776AE2-6027-4494-846F-B50B68B4356C@greg.burd.me>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="wd2hMNxk6cP7U9Rn"
Content-Disposition: inline
In-Reply-To: <1D776AE2-6027-4494-846F-B50B68B4356C@greg.burd.me>
Archived-At: 
 <https://www.postgresql.org/message-id/aMz57nHgkLozddYu%40paquier.xyz>
Precedence: bulk


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

On Thu, Sep 18, 2025 at 02:50:45PM -0400, Greg Burd wrote:
> Thanks for all the feedback and time spent reviewing this patch.  I
> switched out the encode/decode functions to use the nodeToString() and
> stringToNode() functions and change all the SQL testing function
> signatures to TEXT from BYTEA.  This exercises more code and that's good
> for testing purposes.  I took out the code that checks the hash values,
> I can't think of a reason that should cause a future failure should that
> change and output different values.  I re-integrated the earlier random
> testing function along with the newer code.  I'm not sure this buys us
> much if anything.
>=20
> coverage:      HEAD      v7
> lines......:   87.1    90.5   +3.4%
> functions..:  100.0   100.0   +0.0%
> branches...:   63.5    75.9  +12.4%

Nathan has given me his blessing, so as I could look at this patch and
put my hands on it.  I have spotted one bug, and things that could be
done a bit better.  Please see below.

As far as I am concerned there is a mistake with bms_overlap_list().
This API checks a Bitmapset with a list of integers, but you have
coded things so as it checks for a list of Bitmapset.  I think that
test_bms_overlap_list() should take in input a int4 array, then the
code in charge of the conversion from the array to the list needs to
be tweaked a little bit.  With the current code, we could get random
failures with negative members included in a Bitmapset, depending on
what's on the stack.

I am not really convinced about the value brought by bms_values() now
that the output is generated for all the SQL functions using the
"decode" and "encode" routines (which may be actually less confusing
if renamed as "bms_to_text" and "text_to_bms", but that's a personal
take), because this results in doing an extra round-trip between text
and Bitmapset.  The tests don't rely much on NULL as input to
translate that as "(b)".  bmsToString() is just a direct call to what
the decode and encode routines do.

Same remark about test_bms_from_array(), that mimicks nodeRead(), in
charge of doing a int4[] -> Bitmapset conversion.  Wouldn't it be
simpler to just use "(b 1 2 3)" in input and let the decode/encode
routines do the job?

Let's replace the test descriptions in the queries that we know have
an individual result by comments.  For example all the "Range
operations".  These descriptions are useful in the UNION queries,
where we want to rely on a set of Bitmapsets for multiple operations,
to be able to know the description of the result.  For individual
queries, this bloats the output.

The numbering in the tests are not that useful to have.  If the test
suite is updated, that would mean more updates required if we add a
new block in the middle.

+SELECT 'overlapping ranges' as test,
+       bms_values(test_bms_union(
+           test_bms_add_range(NULL, 50, 150),
+           test_bms_add_range(NULL, 100, 200)

The output generated by this query leads to a bitmapset with 200
members.  It would be simpler and more readable to reduce these
ranges, or is there a reason for these to be large?  Same remark for
"difference subtraction edge case" with its 50 members.

+SELECT 'add_range large range' as test, bms_values(test_bms_add_range(NULL=
, 0, 1000)) as result;

This one also is funky with its output of 1000 elements.  Any reason
this is justified in terms of coverage?  If yes, we could use some
substr() calls to get the end and the beginning of the output
generated, perhaps to reduce the output of the whole, or the length,
or something like that relevant enough to validate the output.  There
is a second test a bit down that uses a range of [1000,1100], as well.
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmjM+e4ACgkQnvQgOdby
QH2A/w//aHaQSpmRCqiTNouxqEvICQHNxcXgCTUdYJfyd88pzlOQ1LX/0vHGssuw
q8t13BtsiCiE5RWUaU5LzrdIgKxqdGpQr9tZgN5XIVohCffI2kO5psDAP6k5rNUJ
WHAx7XNrGUqqeibzFthSBPud3/BVH7c/Zb0mTRmQLJFlbGQvSs7+u2MTLwJfcktM
ovNJ7rITLgwT+ZT5Uz4WYOGaIEaMww3voF0xgNxEhbW+GhLc/xYK9tMObMlk8Amg
lEls0mSc0lEppsaxtJZ+691dRupyVx8Q3rSq5LpMqVzV739I3QfNZSaQ2F/4zitS
6IOaon6lgkEuL5XWnoCSW9MGmjIQiwqXgHT2VjurdNHkTN0yr89db8AtBQ57FICY
WhsYbo8Y6WFaD15XsHrJn1j82RhJxUsgyE0QaXCf8n621j3qq0qpy0bcu7uY+yyh
HY9vWG4D5wkUrUdBAfFX80uXHNlcidPgTzlU+x4AoABbix/x91gxp1tXpXAD1gA6
3iK7pEh7d2ilZjN2WHu1h8pzQAgqoyGLePXvZYI4JqXBq05jOD/yT/MO08Z+Wj5U
F+nKYMMJUyebVZ2Znu7sBFbfq4HE0oW7OPUqw78gw5V6+FscMnB+IlQTkiNQ+T1e
bWDAHR1nAwH4OCjT9GaixvxoczyiWmoJts6PgiIMlO9t++rP1ew=
=KKaY
-----END PGP SIGNATURE-----

--wd2hMNxk6cP7U9Rn--