Feedback-ID: i675e48f3:Fastmail
Date: Thu, 9 Oct 2025 09:11:43 -0400
From: Greg Burd <greg@burd.me>
To: Michael Paquier <michael@paquier.xyz>, David Rowley
 <dgrowleyml@gmail.com>
Cc: Ranier Vilela <ranier.vf@gmail.com>, Daniel Gustafsson
 <daniel@yesql.se>, PostgreSQL Hackers
 <pgsql-hackers@lists.postgresql.org>
Message-ID: <06A7959F-F0D3-4B2B-ACB0-19114E071B53@getmailspring.com>
In-Reply-To: <aOcwU_0-U3zbNDki@paquier.xyz>
References: <aOcwU_0-U3zbNDki@paquier.xyz>
Subject: Re: [PATCH] Add tests for Bitmapset
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: inline
Archived-At: 
 <https://www.postgresql.org/message-id/06A7959F-F0D3-4B2B-ACB0-19114E071B53%40getmailspring.com>
Precedence: bulk


On Oct 8 2025, at 11:47 pm, Michael Paquier <michael=40paquier.xyz> wrote=
:

> On Thu, Oct 09, 2025 at 04:35:55PM +1300, David Rowley wrote:
>> On Thu, 9 Oct 2025 at 15:13, Michael Paquier <michael=40paquier.xyz> w=
rote:
>> > What do you think about the attached=3F
>> =20
>> Thanks. Looks pretty good.
>> =20
>> > + members =3D palloc(sizeof(int) * num=5Fops);
>> =20
>> Any reason to pfree that and allocate that to the same size as it
>> already was=3F
> =20
> No reason.  We can shortcut that a bit.
> =20
>> Wondering if the =22members=5Bpos=5D =3D members=5B--num=5Fmembers=5D;=
=22 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 */
> =20
> Yes, a comment can be adapted here.  Sounds good to me.
> --
> Michael

Michael, David,

This patch LGTM.  I appreciate the additional work you've both put into i=
t.

You're right, of course, I didn=E2=80=99t need to preserve the order of e=
lements
when deleting.  This is where I'd started and I don't recall why I
changed it and didn't revert to this simpler solution.

+ members=5Bpos=5D =3D members=5B--num=5Fmembers=5D;

Phase 2 might record a value to members that was also recorded in phase
1, but that seems harmless.  That's why I had:

+ if (=21bms=5Fis=5Fmember(member, bms1) && =21bms=5Fis=5Fmember(member, =
bms2))

I hadn't thought to optimize this function, but given the amount of time
the tests run in aggregate globally, it makes sense.

best.

-greg