MIME-Version: 1.0
References: <197CAFC2-963B-4414-9A51-08AF648146E4@yesql.se>
 <3E15C43D-8FDB-454F-A3E3-182210680C58@getmailspring.com>
In-Reply-To: <3E15C43D-8FDB-454F-A3E3-182210680C58@getmailspring.com>
From: Ranier Vilela <ranier.vf@gmail.com>
Date: Wed, 8 Oct 2025 09:10:16 -0300
Message-ID: 
 <CAEudQAq_zOSA2NUQSWePTGV_=90Uw0WcXxGOWnN-vwF046OOqA@mail.gmail.com>
Subject: Re: [PATCH] Add tests for Bitmapset
To: Greg Burd <greg@burd.me>
Cc: Daniel Gustafsson <daniel@yesql.se>, David Rowley <dgrowleyml@gmail.com>,
	Michael Paquier <michael@paquier.xyz>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: multipart/mixed; boundary="000000000000c50d320640a49158"
Archived-At: 
 <https://www.postgresql.org/message-id/CAEudQAq_zOSA2NUQSWePTGV_%3D90Uw0WcXxGOWnN-vwF046OOqA%40mail.gmail.com>
Precedence: bulk

--000000000000c50d320640a49158
Content-Type: multipart/alternative; boundary="000000000000c50d320640a49156"

--000000000000c50d320640a49156
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi.

Em sex., 3 de out. de 2025 =C3=A0s 09:13, Greg Burd <greg@burd.me> escreveu=
:

>
> On Oct 3 2025, at 4:25 am, Daniel Gustafsson <daniel@yesql.se> wrote:
>
> >> On 3 Oct 2025, at 01:36, David Rowley <dgrowleyml@gmail.com> wrote:
> >>
> >> On Fri, 3 Oct 2025 at 01:33, Daniel Gustafsson <daniel@yesql.se> wrote=
:
> >>> Another nitpick would be to remove the test for NULL in
> test_bms_make_singleton
> >>> since that is a STRICT function, making the test for NULL
> >>> superfluous code:
> >>
> >> I see test_random_operations() is also strict. Is it worth getting rid
> >> of the SQL NULL checks on the inputs there too? Aka, the attached.
> >
> > Indeed, but reading the code I wonder if STRICT was a mistake and the
> intention
> > was to allow NULL input?
>
> Yes, it was an oversight after I re-worked the random function.
>
> > That being said, the function is never called with
> > NULL so that's mostly academic thinking.  +1 for removing the NULL
> > checks and simplifying the code.
>
> I agree, and thank you both for the attention to detail and interest in
> this test suite.
>
With the patch attached, there are regression.
Is it intentional not to check the return of the function bms_is_member?

diff --strip-trailing-cr -U3
C:/dll/postgres_dev/postgres_commit/src/test/modules/test_bitmapset/expecte=
d/test_bitmapset.out
C:/dll/postgres_dev/postgres_commit/build/testrun/test_bitmapset/regress/re=
sults/test_bitmapset.out
---
C:/dll/postgres_dev/postgres_commit/src/test/modules/test_bitmapset/expecte=
d/test_bitmapset.out
2025-10-02 21:17:53.169515700 -0300
+++
C:/dll/postgres_dev/postgres_commit/build/testrun/test_bitmapset/regress/re=
sults/test_bitmapset.out
2025-10-07 21:24:00.534142300 -0300
@@ -1570,9 +1570,5 @@

 -- random operations
 SELECT test_random_operations(-1, 10000, 81920, 0) > 0 AS result;
- result
---------
- t
-(1 row)
-
+ERROR:  union missing member 63904
 DROP EXTENSION test_bitmapset;

best regards,
Ranier Vilela

--000000000000c50d320640a49156
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div dir=3D"ltr">Hi.</div><br><div class=3D"gmail_quote gm=
ail_quote_container"><div dir=3D"ltr" class=3D"gmail_attr">Em sex., 3 de ou=
t. de 2025 =C3=A0s 09:13, Greg Burd &lt;<a href=3D"mailto:greg@burd.me">gre=
g@burd.me</a>&gt; escreveu:<br></div><blockquote class=3D"gmail_quote" styl=
e=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);paddin=
g-left:1ex"><br>
On Oct 3 2025, at 4:25 am, Daniel Gustafsson &lt;<a href=3D"mailto:daniel@y=
esql.se" target=3D"_blank">daniel@yesql.se</a>&gt; wrote:<br>
<br>
&gt;&gt; On 3 Oct 2025, at 01:36, David Rowley &lt;<a href=3D"mailto:dgrowl=
eyml@gmail.com" target=3D"_blank">dgrowleyml@gmail.com</a>&gt; wrote:<br>
&gt;&gt; <br>
&gt;&gt; On Fri, 3 Oct 2025 at 01:33, Daniel Gustafsson &lt;<a href=3D"mail=
to:daniel@yesql.se" target=3D"_blank">daniel@yesql.se</a>&gt; wrote:<br>
&gt;&gt;&gt; Another nitpick would be to remove the test for NULL in test_b=
ms_make_singleton<br>
&gt;&gt;&gt; since that is a STRICT function, making the test for NULL<br>
&gt;&gt;&gt; superfluous code:<br>
&gt;&gt; <br>
&gt;&gt; I see test_random_operations() is also strict. Is it worth getting=
 rid<br>
&gt;&gt; of the SQL NULL checks on the inputs there too? Aka, the attached.=
<br>
&gt; <br>
&gt; Indeed, but reading the code I wonder if STRICT was a mistake and the =
intention<br>
&gt; was to allow NULL input?<br>
<br>
Yes, it was an oversight after I re-worked the random function.<br>
<br>
&gt; That being said, the function is never called with<br>
&gt; NULL so that&#39;s mostly academic thinking.=C2=A0 +1 for removing the=
 NULL<br>
&gt; checks and simplifying the code.<br>
<br>
I agree, and thank you both for the attention to detail and interest in<br>
this test suite.<br></blockquote><div>With the patch attached, there are re=
gression.</div><div>Is it intentional not to check the return of the functi=
on bms_is_member?</div><div><br></div><div>diff --strip-trailing-cr -U3 C:/=
dll/postgres_dev/postgres_commit/src/test/modules/test_bitmapset/expected/t=
est_bitmapset.out C:/dll/postgres_dev/postgres_commit/build/testrun/test_bi=
tmapset/regress/results/test_bitmapset.out<br>--- C:/dll/postgres_dev/postg=
res_commit/src/test/modules/test_bitmapset/expected/test_bitmapset.out	2025=
-10-02 21:17:53.169515700 -0300<br>+++ C:/dll/postgres_dev/postgres_commit/=
build/testrun/test_bitmapset/regress/results/test_bitmapset.out	2025-10-07 =
21:24:00.534142300 -0300<br>@@ -1570,9 +1570,5 @@<br>=C2=A0<br>=C2=A0-- ran=
dom operations<br>=C2=A0SELECT test_random_operations(-1, 10000, 81920, 0) =
&gt; 0 AS result;<br>- result <br>---------<br>- t<br>-(1 row)<br>-<br>+ERR=
OR: =C2=A0union missing member 63904<br>=C2=A0DROP EXTENSION test_bitmapset=
;<br>=C2=A0</div><div>best regards,</div><div>Ranier Vilela</div></div></di=
v>

--000000000000c50d320640a49156--

--000000000000c50d320640a49158
Content-Type: text/plain; charset="US-ASCII";
 name="check-function-return-bms_is_member.txt"
Content-Disposition: attachment;
	filename="check-function-return-bms_is_member.txt"
Content-Transfer-Encoding: base64
Content-ID: <f_mghy5bha0>
X-Attachment-Id: f_mghy5bha0

ZGlmZiAtLWdpdCBhL3NyYy90ZXN0L21vZHVsZXMvdGVzdF9iaXRtYXBzZXQvdGVzdF9iaXRtYXBz
ZXQuYyBiL3NyYy90ZXN0L21vZHVsZXMvdGVzdF9iaXRtYXBzZXQvdGVzdF9iaXRtYXBzZXQuYwpp
bmRleCA4YmM5YjFmNDhlLi45ZTc1NzczMmM1IDEwMDY0NAotLS0gYS9zcmMvdGVzdC9tb2R1bGVz
L3Rlc3RfYml0bWFwc2V0L3Rlc3RfYml0bWFwc2V0LmMKKysrIGIvc3JjL3Rlc3QvbW9kdWxlcy90
ZXN0X2JpdG1hcHNldC90ZXN0X2JpdG1hcHNldC5jCkBAIC03MDAsNyArNzAwLDggQEAgdGVzdF9y
YW5kb21fb3BlcmF0aW9ucyhQR19GVU5DVElPTl9BUkdTKQogCQkJY2FzZSAyOgkJCQkvKiB0ZXN0
IG1lbWJlcnNoaXAgKi8KIAkJCQlpZiAoYm1zICE9IE5VTEwpCiAJCQkJewotCQkJCQlibXNfaXNf
bWVtYmVyKG1lbWJlciwgYm1zKTsKKwkJCQkJaWYgKCFibXNfaXNfbWVtYmVyKG1lbWJlciwgYm1z
KSkKKwkJCQkJCWVsb2coRVJST1IsICJ1bmlvbiBtaXNzaW5nIG1lbWJlciAlZCIsIG1lbWJlcik7
CiAJCQkJfQogCQkJCWJyZWFrOwogCQl9Cg==
--000000000000c50d320640a49158--