Feedback-ID: i0fe9450f:Fastmail
Date: Thu, 9 Oct 2025 12:47:31 +0900
From: Michael Paquier <michael@paquier.xyz>
To: David Rowley <dgrowleyml@gmail.com>
Cc: Greg Burd <greg@burd.me>, Ranier Vilela <ranier.vf@gmail.com>,
	Daniel Gustafsson <daniel@yesql.se>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: [PATCH] Add tests for Bitmapset
Message-ID: <aOcwU_0-U3zbNDki@paquier.xyz>
References: 
 <CAEudQAq_zOSA2NUQSWePTGV_=90Uw0WcXxGOWnN-vwF046OOqA@mail.gmail.com>
 <2D90FFB0-C80A-4189-A5BF-C37F05E271D7@greg.burd.me>
 <CAApHDvqXdBWMwGuzJBSbq4eiNLDaR738-upd8NJpmhUq3a+XLQ@mail.gmail.com>
 <aOcaIr8QKMvbIlnA@paquier.xyz>
 <CAApHDvpn3bshX+_2t0NbJ263r1etcBdmkROjuJw8DvyZkMcXqg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="9tMm0UIKUEFKV99t"
Content-Disposition: inline
In-Reply-To: 
 <CAApHDvpn3bshX+_2t0NbJ263r1etcBdmkROjuJw8DvyZkMcXqg@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/aOcwU_0-U3zbNDki%40paquier.xyz>
Precedence: bulk


--9tMm0UIKUEFKV99t
Content-Type: multipart/mixed; boundary="2UxbqkDRN+/uQwJo"
Content-Disposition: inline


--2UxbqkDRN+/uQwJo
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Thu, Oct 09, 2025 at 04:35:55PM +1300, David Rowley wrote:
> On Thu, 9 Oct 2025 at 15:13, Michael Paquier <michael@paquier.xyz> wrote:
> > What do you think about the attached?
>=20
> Thanks. Looks pretty good.
>=20
> > + members =3D palloc(sizeof(int) * num_ops);
>=20
> Any reason to pfree that and allocate that to the same size as it already=
 was?

No reason.  We can shortcut that a bit.

> Wondering if the "members[pos] =3D members[--num_members];" is worth a
> short comment. Maybe something like: /* zap this member by moving the
> final array member into its place and shrinking the array by 1 */

Yes, a comment can be adapted here.  Sounds good to me.
--
Michael

--2UxbqkDRN+/uQwJo
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v3-0001-test_bitmapset-Improve-random-function.patch"
Content-Transfer-Encoding: quoted-printable

=46rom d7ba7df53ad8d1c45dadb7e01c47b4c30f5bfb75 Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Thu, 9 Oct 2025 12:46:59 +0900
Subject: [PATCH v3] test_bitmapset: Improve random function

---
 .../modules/test_bitmapset/test_bitmapset.c   | 57 ++++++++++++++-----
 1 file changed, 44 insertions(+), 13 deletions(-)

diff --git a/src/test/modules/test_bitmapset/test_bitmapset.c b/src/test/mo=
dules/test_bitmapset/test_bitmapset.c
index 8bc9b1f48e9a..abbdde85e0b3 100644
--- a/src/test/modules/test_bitmapset/test_bitmapset.c
+++ b/src/test/modules/test_bitmapset/test_bitmapset.c
@@ -616,25 +616,31 @@ test_random_operations(PG_FUNCTION_ARGS)
 	min_value =3D PG_GETARG_INT32(3);
=20
 	pg_prng_seed(&state, seed);
+
+	/*
+	 * There can be up to "num_ops" members added.  This is very unlikely
+	 * still possible if all the operations hit the "0" case during phase 4
+	 * where multiple operation types are mixed together.
+	 */
 	members =3D palloc(sizeof(int) * num_ops);
=20
-	/* Phase 1: Random insertions */
+	/* Phase 1: Random insertions in first set */
 	for (int i =3D 0; i < num_ops / 2; i++)
 	{
 		member =3D pg_prng_uint32(&state) % max_range + min_value;
=20
 		if (!bms_is_member(member, bms1))
-		{
 			members[num_members++] =3D member;
-			bms1 =3D bms_add_member(bms1, member);
-		}
+		bms1 =3D bms_add_member(bms1, member);
 	}
=20
-	/* Phase 2: Random set operations */
+	/* Phase 2: Random insertions in second set */
 	for (int i =3D 0; i < num_ops / 4; i++)
 	{
 		member =3D pg_prng_uint32(&state) % max_range + min_value;
=20
+		if (!bms_is_member(member, bms2))
+			members[num_members++] =3D member;
 		bms2 =3D bms_add_member(bms2, member);
 	}
=20
@@ -642,7 +648,7 @@ test_random_operations(PG_FUNCTION_ARGS)
 	result =3D bms_union(bms1, bms2);
 	EXPECT_NOT_NULL(result);
=20
-	/* Verify union contains all members from first set */
+	/* Verify union contains all members from first and second sets */
 	for (int i =3D 0; i < num_members; i++)
 	{
 		if (!bms_is_member(members[i], result))
@@ -650,7 +656,10 @@ test_random_operations(PG_FUNCTION_ARGS)
 	}
 	bms_free(result);
=20
-	/* Test intersection */
+	/*
+	 * Test intersection, checking that all the members in the result are from
+	 * both the first and second sets.
+	 */
 	result =3D bms_intersect(bms1, bms2);
 	if (result !=3D NULL)
 	{
@@ -679,28 +688,49 @@ test_random_operations(PG_FUNCTION_ARGS)
 		bms_free(result);
 	}
=20
-	pfree(members);
 	bms_free(bms1);
 	bms_free(bms2);
=20
-	for (int i =3D 0; i < num_ops; i++)
+	/*
+	 * Phase 4: mix of operations on a single set, cross-checking a bitmap
+	 * with a secondary state, "members".
+	 */
+	num_members =3D 0;
+
+	for (int op =3D 0; op < num_ops; op++)
 	{
-		member =3D pg_prng_uint32(&state) % max_range + min_value;
 		switch (pg_prng_uint32(&state) % 3)
 		{
 			case 0:				/* add */
+				member =3D pg_prng_uint32(&state) % max_range + min_value;
+				if (!bms_is_member(member, bms))
+					members[num_members++] =3D member;
 				bms =3D bms_add_member(bms, member);
 				break;
 			case 1:				/* delete */
-				if (bms !=3D NULL)
+				if (num_members > 0)
 				{
+					int			pos =3D pg_prng_uint32(&state) % num_members;
+
+					member =3D members[pos];
+					if (!bms_is_member(member, bms))
+						elog(ERROR, "expected %d to be a valid member", member);
+
 					bms =3D bms_del_member(bms, member);
+
+					/*
+					 * Move the final array member at the position of the member
+					 * just deleted, reducing the array size by one.
+					 */
+					members[pos] =3D members[--num_members];
 				}
 				break;
 			case 2:				/* test membership */
-				if (bms !=3D NULL)
+				/* Verify that bitmap contains all members */
+				for (int i =3D 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;
 		}
@@ -708,6 +738,7 @@ test_random_operations(PG_FUNCTION_ARGS)
 	}
=20
 	bms_free(bms);
+	pfree(members);
=20
 	PG_RETURN_INT32(total_ops);
 }
--=20
2.51.0


--2UxbqkDRN+/uQwJo--

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmjnMFMACgkQnvQgOdby
QH0XAA//bAUJwveaA7BR+a84IDJQF+bk81kra2/S7ydzQyJFc7Gev4bQKngV0a4o
B6PYKp4raTH6fijKWTISsV8jLswjT9y2Vz2eDZHVEIorPtgXjJ0XL1XLv6iH3N5q
/PFcMzZzgBhDDanpWdJTU7aUOcN1GdhWgIYeLrumpkK2UvyGlL2r13/R80aRK5lM
2t/iX1QjdjbeimCOkjqAOLAAag66yAf2sHKLEvS0LHWaHwDNEJGA6BXk+dpDR5S6
TEONZHvfz2dPkf7gqODf5ib14/0BONmtiQlJvRw5iCSbF/wj+rJMGUo8EKEPLnoi
anw3QJ0Yz1chBgmTVKKtvNaGJzd6XJ/MEEnqn3XHealVcAc1C7uQtZDYf/t8sDSc
omWJ/UYLe+PC6YuGDj7BqQOKXWQCY/kJMNS1Z0MlcNmx+z26uxf2hQnKo2mxiywk
pnHbaszpz7hO+vOrSg773IFue+9mvyU+ULsh3/6Pqjo4l01dt0cW34e7jiKRlCra
YAtVXRqxqv/6VjHF/Os95EGnu2/r/eVgU7Xj4muCboNlkXbwamN4b86bmDuY0mFU
nsBp7OghURtxmix83zGor3fxeFDLo6XDizDv7UippaU/TtEngvpkEo8gBI/acnTj
GFfRFkUtKS5yw1g5VsgYYs8uYTcYtf46xa6MLNig7cIaQ4KCcjw=
=b++7
-----END PGP SIGNATURE-----

--9tMm0UIKUEFKV99t--