public inbox for [email protected]  

help / color / mirror / Atom feed

From: Greg Burd <[email protected]>
To: Michael Paquier <[email protected]>
To: David Rowley <[email protected]>
Cc: Ranier Vilela <[email protected]>
Cc: Daniel Gustafsson <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: [PATCH] Add tests for Bitmapset
Date: Thu, 9 Oct 2025 09:11:43 -0400
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>


On Oct 8 2025, at 11:47 pm, Michael Paquier <[email protected]> wrote:

> On Thu, Oct 09, 2025 at 04:35:55PM +1300, David Rowley wrote:
>> On Thu, 9 Oct 2025 at 15:13, Michael Paquier <[email protected]> wrote:
>> > What do you think about the attached?
>>  
>> Thanks. Looks pretty good.
>>  
>> > + members = palloc(sizeof(int) * num_ops);
>>  
>> 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] = 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

Michael, David,

This patch LGTM.  I appreciate the additional work you've both put into it.

You're right, of course, I didn’t need to preserve the order of elements
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[pos] = members[--num_members];

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 (!bms_is_member(member, bms1) && !bms_is_member(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

view thread (81+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: [PATCH] Add tests for Bitmapset
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox