Feedback-ID: i675e48f3:Fastmail
MIME-Version: 1.0
Date: Sun, 19 Apr 2026 07:41:23 -0700
From: "Greg Burd" <greg@burd.me>
To: "David Rowley" <dgrowleyml@gmail.com>,
 "Michael Paquier" <michael@paquier.xyz>
Cc: "Ranier Vilela" <ranier.vf@gmail.com>,
 "Daniel Gustafsson" <daniel@yesql.se>,
 "PostgreSQL Hackers" <pgsql-hackers@lists.postgresql.org>
Message-Id: <12754d02-855c-4b11-ae7c-48a73124e715@app.fastmail.com>
In-Reply-To: 
 <CAApHDvrDW9W72vAr7h7XeCu7+Qz-_Vff02Q+RPPuVeM0Qf0MCw@mail.gmail.com>
References: 
 <CAEudQAq_zOSA2NUQSWePTGV_=90Uw0WcXxGOWnN-vwF046OOqA@mail.gmail.com>
 <2D90FFB0-C80A-4189-A5BF-C37F05E271D7@greg.burd.me>
 <CAApHDvqXdBWMwGuzJBSbq4eiNLDaR738-upd8NJpmhUq3a+XLQ@mail.gmail.com>
 <aOcaIr8QKMvbIlnA@paquier.xyz>
 <CAApHDvpn3bshX+_2t0NbJ263r1etcBdmkROjuJw8DvyZkMcXqg@mail.gmail.com>
 <aOcwU_0-U3zbNDki@paquier.xyz>
 <CAApHDvp=pEkO6LKfXKDjRNeQ1LkV7A5fsTn29ahUBXxmjyWPHg@mail.gmail.com>
 <aOg3X3BXjiXljHcR@paquier.xyz>
 <CAApHDvrDW9W72vAr7h7XeCu7+Qz-_Vff02Q+RPPuVeM0Qf0MCw@mail.gmail.com>
Subject: Re: [PATCH] Add tests for Bitmapset
Content-Type: text/plain
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/12754d02-855c-4b11-ae7c-48a73124e715%40app.fastmail.com>
Precedence: bulk


On Sat, Apr 18, 2026, at 2:06 AM, David Rowley wrote:
> On Fri, 10 Oct 2025 at 11:30, Michael Paquier <michael@paquier.xyz> wrote:
>> Thanks for double-checking.  Applied after running an indent.
>
> I was working on test_bitmapset.c to add some tests for a new
> bitmapset function. I noticed a few weird things.

Hi David,

I took a look at your patch, it seems straight forward and valuable.  Thanks for pushing this forward a bit.

> 1. test_random_operations() is coded to use GetCurrentTimestamp() as a
> seed when the given seed is <= 0. Of course, it'll be a while before
> the return value of that wraps beyond 2^63 (292250 years), but I still
> can't help but think that NULL is a better value to use to have the
> seed auto-generate.

I don't recall off hand why I had it coded that way, your changes make sense.

> 2. Doing #1 means the function can't be STRICT. I do think it's wrong
> that the function is marked as strict. That's normally reserved for
> functions that we needn't call because NULL input(s) yield a NULL
> output. That's not the case for this function.

Agreed, good call.

> 3. There's no CHECK_FOR_INTERRUPTS() in test_random_operations(). If
> someone uses a large num_ops, there's no way to cancel the query.

Absolutely, thanks for finding the oversight.

> 4. If there happened to be some rare bug in bitmapset.c that
> test_random_operations() we might struggle to find it again, as we
> don't report which seed we used in the ERROR message.

Makes perfect sense.

> 5. Couple of minor cosmetics; header order, whilespace.

+1

> I felt it was worth fixing these now as the function I plan to add
> there does #1, #2, #3 and #4. If I add the new function for v20, the
> discrepancy seems questionable.
>
> David

I think these changes are reasonable and improve the tests.  Now I need to review your new bitmapset feature... :)

best.

-greg

> Attachments:
> * test_bitmapset_fixes.patch