Feedback-ID: i675e48f3:Fastmail
Date: Wed, 8 Oct 2025 14:48:03 -0400
From: Greg Burd <greg@burd.me>
To: Ranier Vilela <ranier.vf@gmail.com>
Cc: Daniel Gustafsson <daniel@yesql.se>, David Rowley
 <dgrowleyml@gmail.com>, Michael Paquier <michael@paquier.xyz>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Message-ID: <2D90FFB0-C80A-4189-A5BF-C37F05E271D7@greg.burd.me>
In-Reply-To: 
 <CAEudQAq_zOSA2NUQSWePTGV_=90Uw0WcXxGOWnN-vwF046OOqA@mail.gmail.com>
References: 
 <CAEudQAq_zOSA2NUQSWePTGV_=90Uw0WcXxGOWnN-vwF046OOqA@mail.gmail.com>
Subject: Re: [PATCH] Add tests for Bitmapset
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="68e6b1e3_6bd364f1_41f9"
Archived-At: 
 <https://www.postgresql.org/message-id/2D90FFB0-C80A-4189-A5BF-C37F05E271D7%40greg.burd.me>
Precedence: bulk

--68e6b1e3_6bd364f1_41f9
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: inline


On Oct 8 2025, at 8:10 am, Ranier Vilela <ranier.vf=40gmail.com> wrote:

> Hi.
> =20
> =20
>> Em sex., 3 de out. de 2025 =C3=A0s 09:13, Greg Burd <greg=40burd.me> e=
screveu:
>> =20
>>> =20
>>> On Oct 3 2025, at 4:25 am, Daniel Gustafsson <daniel=40yesql.se> wrot=
e:
>>> =20
>>>>> On 3 Oct 2025, at 01:36, David Rowley <dgrowleyml=40gmail.com> wrot=
e:
>>>>> =20
>>>>> On =46ri, 3 Oct 2025 at 01:33, Daniel Gustafsson <daniel=40yesql.se=
> wrote:
>>>>>> Another nitpick would be to remove the test for NULL in test=5Fbms=
=5Fmake=5Fsingleton
>>>>>> since that is a STRICT function, making the test for NULL
>>>>>> superfluous code:
>>>>> =20
>>>>> I see test=5Frandom=5Foperations() is also strict. Is it worth gett=
ing rid
>>>>> of the SQL NULL checks on the inputs there too=3F Aka, the attached=
.
>>>> =20
>>>> Indeed, but reading the code I wonder if STRICT was a mistake and
>>>> the intention
>>>> was to allow NULL input=3F
>>> =20
>>> Yes, it was an oversight after I re-worked the random function.
>>> =20
>>>> That being said, the function is never called with
>>>> NULL so that's mostly academic thinking.=C2=A0 +1 for removing the N=
ULL
>>>> checks and simplifying the code.
>>> =20
>>> 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=5Fis=5Fm=
ember=3F
>> =20
>> diff --strip-trailing-cr -U3
>> C:/dll/postgres=5Fdev/postgres=5Fcommit/src/test/modules/test=5Fbitmap=
set/expected/test=5Fbitmapset.out C:/dll/postgres=5Fdev/postgres=5Fcommit=
/build/testrun/test=5Fbitmapset/regress/results/test=5Fbitmapset.out
>> ---
>> C:/dll/postgres=5Fdev/postgres=5Fcommit/src/test/modules/test=5Fbitmap=
set/expected/test=5Fbitmapset.out
>> 2025-10-02 21:17:53.169515700 -0300
>> +++
>> C:/dll/postgres=5Fdev/postgres=5Fcommit/build/testrun/test=5Fbitmapset=
/regress/results/test=5Fbitmapset.out
>> 2025-10-07 21:24:00.534142300 -0300
>> =40=40 -1570,9 +1570,5 =40=40
>> =C2=A0
>> =C2=A0-- random operations
>> =C2=A0SELECT test=5Frandom=5Foperations(-1, 10000, 81920, 0) > 0 AS re=
sult;
>> - result
>> ---------
>> - t
>> -(1 row)
>> -
>> +ERROR: =C2=A0union missing member 63904
>> =C2=A0DROP EXTENSION test=5Fbitmapset;
>> =C2=A0
>> best regards,
>> Ranier Vilela

Thanks Ranier for the report.

You're correct, the tests I wrote overlooked testing the return value
and so were not accomplishing the goal of testing at all.

Adding your small suggested patched turned over another flaw.  The error
message you were seeing was from later in that same function.

> +ERROR: =C2=A0union missing member 63904

This was emitted by the code in the second portion of the randomized
testing where there is a loop doing add/del/test operations.  That error
message was misleading as it matched the earlier error message near the
change you made.

But, thankfully that pointed to a second (face palm) flaw in that
function (again, I take full credit/blame).  The add/del/test loop
wasn't accumulating the anticipated set of members and so was testing if
the latest random int generated was in the set or not, which makes no
sense.  I've updated that loop to retain the list of members and the
test to check against the retained list of members.  I've updated the
log message to be different from those above to avoid similar confusion
in the future.

This seems to be passing now, apologies for not paying closer attention
to this code earlier on.

best.

-greg

--68e6b1e3_6bd364f1_41f9
Content-Type: application/octet-stream
Content-Transfer-Encoding: base64
Content-Disposition: attachment;
 filename="v1-0001-Correct-flaws-in-randomized-Bitmapset-tests.patch"

RnJvbSBlY2ZlNzcwZDRlZjgxMzlkNmY4NTJkZGU1MDM3YTdlYjUxNzE1ZTg5IE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBHcmVnIEJ1cmQgPGdyZWdAYnVyZC5tZT4KRGF0ZTogV2VkLCA4
IE9jdCAyMDI1IDE0OjI3OjMwIC0wNDAwClN1YmplY3Q6IFtQQVRDSCB2MV0gQ29ycmVjdCBmbGF3
cyBpbiByYW5kb21pemVkIEJpdG1hcHNldCB0ZXN0cwoKVGhlcmUgd2VyZSB0d28gb3ZlcnNpZ2h0
cyBpbiB0aGUgZnVuY3Rpb24gZGVzaWduYXRlZCB0byBwZXJmb3JtCnJhbmRvbWl6ZWQgdGVzdHMg
YWdhaW5zdCBCaXRtYXBzZXQuICBGaXJzdCwgdGhlIHRlc3QgdmFsaWRhdGluZwpibXNfdW5pb24o
KSB3YXMgbm90IGNoZWNraW5nIHRoZSByZXR1cm4gZnJvbSBibXNfaXNfbWVtYmVyKCkgYW5kIHNv
Cndhc24ndCB0ZXN0aW5nIG11Y2ggYXQgYWxsLiAgU2Vjb25kLCBpbiB0aGUgc2Vjb25kIGhhbGYg
b2YgdGhpcyBmdW5jdGlvbgpwZXJmb3JtaW5nIHJhbmRvbWl6ZWQgb3BlcmF0aW9ucyB0aGVyZSB3
YXMgbm8gdHJhY2tpbmcgb2Ygd2hhdCBtZW1iZXJzCnNob3VsZCBleGlzdCBhZnRlciBzb21lIG51
bWJlciBvZiByYW5kb20gYWRkL2RlbCBvcGVyYXRpb25zLiAgVGhpcyBwYXRjaAphZGRyZXNzZXMg
Ym90aCBvZiB0aG9zZSBpc3N1ZXMuCgpSZXBvcnRlZC1ieTogUmFuaWVyIFZpbGVsYTxyYW5pZXIu
dmZAZ21haWwuY29tPgpEaXNjdXNzaW9uOiBodHRwczovL3Bvc3Rnci5lcy9tL0NBQXBIRHZxZ2hN
bm1femdTTmVmdG85b2FFSjBTLTNDZ2IzZ2RzVjdYdkxDLWhNUzAyUUBtYWlsLmdtYWlsLmNvbQot
LS0KIC4uLi9tb2R1bGVzL3Rlc3RfYml0bWFwc2V0L3Rlc3RfYml0bWFwc2V0LmMgICB8IDMxICsr
KysrKysrKysrKysrLS0tLS0KIDEgZmlsZSBjaGFuZ2VkLCAyMyBpbnNlcnRpb25zKCspLCA4IGRl
bGV0aW9ucygtKQoKZGlmZiAtLWdpdCBhL3NyYy90ZXN0L21vZHVsZXMvdGVzdF9iaXRtYXBzZXQv
dGVzdF9iaXRtYXBzZXQuYyBiL3NyYy90ZXN0L21vZHVsZXMvdGVzdF9iaXRtYXBzZXQvdGVzdF9i
aXRtYXBzZXQuYwppbmRleCA4YmM5YjFmNDhlOS4uMDA2NTFjYjhkZjEgMTAwNjQ0Ci0tLSBhL3Ny
Yy90ZXN0L21vZHVsZXMvdGVzdF9iaXRtYXBzZXQvdGVzdF9iaXRtYXBzZXQuYworKysgYi9zcmMv
dGVzdC9tb2R1bGVzL3Rlc3RfYml0bWFwc2V0L3Rlc3RfYml0bWFwc2V0LmMKQEAgLTYyNCwxMCAr
NjI0LDggQEAgdGVzdF9yYW5kb21fb3BlcmF0aW9ucyhQR19GVU5DVElPTl9BUkdTKQogCQltZW1i
ZXIgPSBwZ19wcm5nX3VpbnQzMigmc3RhdGUpICUgbWF4X3JhbmdlICsgbWluX3ZhbHVlOwogCiAJ
CWlmICghYm1zX2lzX21lbWJlcihtZW1iZXIsIGJtczEpKQotCQl7CiAJCQltZW1iZXJzW251bV9t
ZW1iZXJzKytdID0gbWVtYmVyOwotCQkJYm1zMSA9IGJtc19hZGRfbWVtYmVyKGJtczEsIG1lbWJl
cik7Ci0JCX0KKwkJYm1zMSA9IGJtc19hZGRfbWVtYmVyKGJtczEsIG1lbWJlcik7CiAJfQogCiAJ
LyogUGhhc2UgMjogUmFuZG9tIHNldCBvcGVyYXRpb25zICovCkBAIC02MzUsNiArNjMzLDggQEAg
dGVzdF9yYW5kb21fb3BlcmF0aW9ucyhQR19GVU5DVElPTl9BUkdTKQogCXsKIAkJbWVtYmVyID0g
cGdfcHJuZ191aW50MzIoJnN0YXRlKSAlIG1heF9yYW5nZSArIG1pbl92YWx1ZTsKIAorCQlpZiAo
IWJtc19pc19tZW1iZXIobWVtYmVyLCBibXMxKSAmJiAhYm1zX2lzX21lbWJlcihtZW1iZXIsIGJt
czIpKQorCQkJbWVtYmVyc1tudW1fbWVtYmVycysrXSA9IG1lbWJlcjsKIAkJYm1zMiA9IGJtc19h
ZGRfbWVtYmVyKGJtczIsIG1lbWJlcik7CiAJfQogCkBAIC02ODMsMjQgKzY4MywzOSBAQCB0ZXN0
X3JhbmRvbV9vcGVyYXRpb25zKFBHX0ZVTkNUSU9OX0FSR1MpCiAJYm1zX2ZyZWUoYm1zMSk7CiAJ
Ym1zX2ZyZWUoYm1zMik7CiAKLQlmb3IgKGludCBpID0gMDsgaSA8IG51bV9vcHM7IGkrKykKKwlu
dW1fbWVtYmVycyA9IDA7CisJbWVtYmVycyA9IHBhbGxvYyhzaXplb2YoaW50KSAqIG51bV9vcHMp
OworCisJZm9yIChpbnQgb3AgPSAwOyBvcCA8IG51bV9vcHM7IG9wKyspCiAJewotCQltZW1iZXIg
PSBwZ19wcm5nX3VpbnQzMigmc3RhdGUpICUgbWF4X3JhbmdlICsgbWluX3ZhbHVlOwogCQlzd2l0
Y2ggKHBnX3BybmdfdWludDMyKCZzdGF0ZSkgJSAzKQogCQl7CiAJCQljYXNlIDA6CQkJCS8qIGFk
ZCAqLworCQkJCW1lbWJlciA9IHBnX3BybmdfdWludDMyKCZzdGF0ZSkgJSBtYXhfcmFuZ2UgKyBt
aW5fdmFsdWU7CisJCQkJaWYgKCFibXNfaXNfbWVtYmVyKG1lbWJlciwgYm1zKSkKKwkJCQkJbWVt
YmVyc1tudW1fbWVtYmVycysrXSA9IG1lbWJlcjsKIAkJCQlibXMgPSBibXNfYWRkX21lbWJlcihi
bXMsIG1lbWJlcik7CiAJCQkJYnJlYWs7CiAJCQljYXNlIDE6CQkJCS8qIGRlbGV0ZSAqLwotCQkJ
CWlmIChibXMgIT0gTlVMTCkKKwkJCQlpZiAobnVtX21lbWJlcnMgPiAwKQogCQkJCXsKKwkJCQkJ
aW50CQkJcG9zID0gcGdfcHJuZ191aW50MzIoJnN0YXRlKSAlIG51bV9tZW1iZXJzOworCisJCQkJ
CW1lbWJlciA9IG1lbWJlcnNbcG9zXTsKKwkJCQkJaWYgKCFibXNfaXNfbWVtYmVyKG1lbWJlciwg
Ym1zKSkKKwkJCQkJCWVsb2coRVJST1IsICJleHBlY3RlZCAlZCB0byBiZSBhIHZhbGlkIG1lbWJl
ciIsIG1lbWJlcik7CiAJCQkJCWJtcyA9IGJtc19kZWxfbWVtYmVyKGJtcywgbWVtYmVyKTsKKwkJ
CQkJbnVtX21lbWJlcnMtLTsKKwkJCQkJbWVtbW92ZShtZW1iZXJzICsgcG9zLCBtZW1iZXJzICsg
cG9zICsgMSwKKwkJCQkJCQkobnVtX21lbWJlcnMgLSBwb3MpICogc2l6ZW9mKGludCkpOwogCQkJ
CX0KIAkJCQlicmVhazsKIAkJCWNhc2UgMjoJCQkJLyogdGVzdCBtZW1iZXJzaGlwICovCi0JCQkJ
aWYgKGJtcyAhPSBOVUxMKQorCQkJCS8qIFZlcmlmeSBiaXRtYXAgY29udGFpbnMgYWxsIG1lbWJl
cnMgKi8KKwkJCQlmb3IgKGludCBpID0gMDsgaSA8IG51bV9tZW1iZXJzOyBpKyspCiAJCQkJewot
CQkJCQlibXNfaXNfbWVtYmVyKG1lbWJlciwgYm1zKTsKKwkJCQkJaWYgKCFibXNfaXNfbWVtYmVy
KG1lbWJlcnNbaV0sIGJtcykpCisJCQkJCQllbG9nKEVSUk9SLCAibWlzc2luZyBtZW1iZXIgJWQi
LCBtZW1iZXJzW2ldKTsKIAkJCQl9CiAJCQkJYnJlYWs7CiAJCX0KLS0gCjIuNDkuMAoK

--68e6b1e3_6bd364f1_41f9--