Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1v6Zlt-006i0E-3r for pgsql-hackers@arkaria.postgresql.org; Wed, 08 Oct 2025 19:24:21 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1v6Zlq-00Cajn-R8 for pgsql-hackers@arkaria.postgresql.org; Wed, 08 Oct 2025 19:24:19 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1v6Zlq-00CajT-Bd for pgsql-hackers@lists.postgresql.org; Wed, 08 Oct 2025 19:24:19 +0000 Received: from mail-pg1-x52d.google.com ([2607:f8b0:4864:20::52d]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.96) (envelope-from ) id 1v6Zlo-000ioG-0W for pgsql-hackers@lists.postgresql.org; Wed, 08 Oct 2025 19:24:18 +0000 Received: by mail-pg1-x52d.google.com with SMTP id 41be03b00d2f7-b5a631b9c82so92255a12.1 for ; Wed, 08 Oct 2025 12:24:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1759951456; x=1760556256; darn=lists.postgresql.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=HXR2S46vE991/QQgBAIF9+8E8QjHb2nZjxe0CCt7+LQ=; b=kzUJCKKpZZozZBp/7kXDMG512qbgQKyz4XhluDWlSAlXOsnSHduJH2hRBBa9xJH5nC b+EukBtuACnOOmYu1N2KJtEdBxr9XNFk+o5L4g1AE+SbVH9xRxAtkhwrvNyR1I0FiQr3 EVo3slF/WhvVKnBrqR45iSwHottsUfVNMPgTHepfl3tyqt8qtHnKYdjUO0+n0eGGoYx+ 3O1PNDkIFv0v3nEuIC9TYR/ijHKZLvm6PyFHuhQUhP99vaIDsJXqsqLr0qaYIirxf60P 9Pp79jvbHFazeNk8dEcxMq8ZeotGW+Dr40Q5aciGAdgIpV3rrit1rmNvwhpjcVkEiryX SJoA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1759951456; x=1760556256; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=HXR2S46vE991/QQgBAIF9+8E8QjHb2nZjxe0CCt7+LQ=; b=MpZKhC0wD92zgu1qc14iJBYD7Jfhk48OiT+i2Gp91RXTepGHKUtUYOplZ8D7ACR9t0 M8BLHrCnFb4Q++YIOx3BFrLuimXzDYbi5f+13yOJZyGR/YVRXFne/mZBFTQxBJ87JyWU VKDqMRtwAUWWSwy2DTNtD1R1nqKp7XWqQJBz/QPj8HLMQgPjHVwC2951ngHh94LZ72wb d8997UXoLqYlMgH9/urnr5kRlsqRILr8/X7++uhK8LdNq82Y1PUP7BlIzH6LnvIz7Oya ZSXq6te35e9jl91Fe83L6HSSlMIf1IOpYR0kbbyrzoGQSwXvrqmA/kRvDKCAcRa6d+3H e+JQ== X-Forwarded-Encrypted: i=1; AJvYcCVjqZJS/a6Yk/wpeq2C+UW4B0mstwM6cdtSOf4pEW4wSu8nwWTTDZMGNW1kg5r+huocyRYMKK5Yohk0tq52@lists.postgresql.org X-Gm-Message-State: AOJu0Yz4hukLqFYm8d7kcsqPXGrL2sxB4/N0KH3rsRTV0r0qd1wBvshD fqoOoiDMtvJqJ2Geoc4BnGIWKrpEhnnfpsmDNY5Pasc29wqxxOt+rtru3wN7qsrHQ23kAUjyXi6 l0N2x5zgDof/4KMJY349vpvMEtgtgCKs= X-Gm-Gg: ASbGncv53cLbtpDN6fUp/9nI8mf/NQxY149LT1oF91W0d/phVPlKaS0fU6+U7KJ+kw+ 16Br3rcqs5Au2KyV25JBClIwnn2JU+Bvl8S6pSR12nbCkhi1TpXsjy71VSOjohqpaQcQMLM62C0 m3Y2wu/ex2T+k0n0NXov0E04vTJ+wb/e3sfOG2Yug/WkcQL1LrnzaO/zmsgyQ1QuhVOep3eJJIQ a+XSBtI5pel3PbUqILj4xkIXikJ/NQ= X-Google-Smtp-Source: AGHT+IFE5comGLY9pSaYKDSr+i/C1GdhA4UfuU4UNDlzmOQcF6R3P1UEEnHLp7fvwIRdgmC8LyuUaJE1gT1jWLDjlXo= X-Received: by 2002:a17:902:d50f:b0:278:9051:8ea9 with SMTP id d9443c01a7336-290272dc4a7mr66676815ad.40.1759951456137; Wed, 08 Oct 2025 12:24:16 -0700 (PDT) MIME-Version: 1.0 References: <2D90FFB0-C80A-4189-A5BF-C37F05E271D7@greg.burd.me> In-Reply-To: <2D90FFB0-C80A-4189-A5BF-C37F05E271D7@greg.burd.me> From: Ranier Vilela Date: Wed, 8 Oct 2025 16:24:08 -0300 X-Gm-Features: AS18NWDw3vXbKCs-NrOyEmzE0HBkj-kAhYG_7brj0pa3DiSuEbR9TbmYYTcKZvw Message-ID: Subject: Re: [PATCH] Add tests for Bitmapset To: Greg Burd Cc: Daniel Gustafsson , David Rowley , Michael Paquier , PostgreSQL Hackers Content-Type: multipart/alternative; boundary="0000000000005d78830640aaa110" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --0000000000005d78830640aaa110 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Em qua., 8 de out. de 2025 =C3=A0s 15:48, Greg Burd escreveu= : > > On Oct 8 2025, at 8:10 am, Ranier Vilela wrote: > > > Hi. > > > > > >> Em sex., 3 de out. de 2025 =C3=A0s 09:13, Greg Burd esc= reveu: > >> > >>> > >>> On Oct 3 2025, at 4:25 am, Daniel Gustafsson wrote: > >>> > >>>>> On 3 Oct 2025, at 01:36, David Rowley wrote: > >>>>> > >>>>> On Fri, 3 Oct 2025 at 01:33, Daniel Gustafsson > 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_membe= r? > >> > >> diff --strip-trailing-cr -U3 > >> > C:/dll/postgres_dev/postgres_commit/src/test/modules/test_bitmapset/expec= ted/test_bitmapset.out > C:/dll/postgres_dev/postgres_commit/build/testrun/test_bitmapset/regress/= results/test_bitmapset.out > >> --- > >> > C:/dll/postgres_dev/postgres_commit/src/test/modules/test_bitmapset/expec= ted/test_bitmapset.out > >> 2025-10-02 21:17:53.169515700 -0300 > >> +++ > >> > C:/dll/postgres_dev/postgres_commit/build/testrun/test_bitmapset/regress/= results/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 > > 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: union 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. > Great. Thank you for their efforts. Tests pass here too (Windows). LGTM. best regards, Ranier Vilela --0000000000005d78830640aaa110 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


Em qua., 8 de o= ut. de 2025 =C3=A0s 15:48, Greg Burd <gr= eg@burd.me> escreveu:

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

> Hi.
>=C2=A0
>=C2=A0
>> Em sex., 3 de out. de 2025 =C3=A0s 09:13, Greg Burd <greg@burd.me> escreveu:
>>=C2=A0
>>>=C2=A0
>>> On Oct 3 2025, at 4:25 am, Daniel Gustafsson <daniel@yesql.se> wrote:
>>>=C2=A0
>>>>> On 3 Oct 2025, at 01:36, David Rowley <dgrowleyml@gmail.com> = wrote:
>>>>>=C2=A0
>>>>> On Fri, 3 Oct 2025 at 01:33, Daniel Gustafsson <daniel@yesql.se> w= rote:
>>>>>> Another nitpick would be to remove the test for NU= LL in test_bms_make_singleton
>>>>>> since that is a STRICT function, making the test f= or NULL
>>>>>> superfluous code:
>>>>>=C2=A0
>>>>> I see test_random_operations() is also strict. Is it w= orth getting rid
>>>>> of the SQL NULL checks on the inputs there too? Aka, t= he attached.
>>>>=C2=A0
>>>> Indeed, but reading the code I wonder if STRICT was a mist= ake and
>>>> the intention
>>>> was to allow NULL input?
>>>=C2=A0
>>> Yes, it was an oversight after I re-worked the random function= .
>>>=C2=A0
>>>> That being said, the function is never called with
>>>> NULL so that's mostly academic thinking.=C2=A0 +1 for = removing the NULL
>>>> checks and simplifying the code.
>>>=C2=A0
>>> I agree, and thank you both for the attention to detail and in= terest in
>>> this test suite.
>> With the patch attached, there are regression.
>> Is it intentional not to check the return of the function bms_is_m= ember?
>>=C2=A0
>> diff --strip-trailing-cr -U3
>> C:/dll/postgres_dev/postgres_commit/src/test/modules/test_bitmapse= t/expected/test_bitmapset.out C:/dll/postgres_dev/postgres_commit/build/tes= trun/test_bitmapset/regress/results/test_bitmapset.out
>> ---
>> C:/dll/postgres_dev/postgres_commit/src/test/modules/test_bitmapse= t/expected/test_bitmapset.out
>> 2025-10-02 21:17:53.169515700 -0300
>> +++
>> C:/dll/postgres_dev/postgres_commit/build/testrun/test_bitmapset/r= egress/results/test_bitmapset.out
>> 2025-10-07 21:24:00.534142300 -0300
>> @@ -1570,9 +1570,5 @@
>> =C2=A0
>> =C2=A0-- random operations
>> =C2=A0SELECT test_random_operations(-1, 10000, 81920, 0) > 0 AS= result;
>> - result
>> ---------
>> - t
>> -(1 row)
>> -
>> +ERROR: =C2=A0union missing member 63904
>> =C2=A0DROP EXTENSION test_bitmapset;
>> =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.=C2=A0 The err= or
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.=C2=A0 That err= or
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).=C2=A0 The add/del/test loop
wasn't accumulating the anticipated set of members and so was testing i= f
the latest random int generated was in the set or not, which makes no
sense.=C2=A0 I've updated that loop to retain the list of members and t= he
test to check against the retained list of members.=C2=A0 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.
Great. Thank you for their ef= forts.

Tests pass here too (Windows).
LGTM.

best regards,
Ranier = Vilela
--0000000000005d78830640aaa110--