public inbox for [email protected]
help / color / mirror / Atom feedFrom: Michael Paquier <[email protected]>
To: David Rowley <[email protected]>
Cc: Greg Burd <[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: Mon, 20 Apr 2026 06:38:43 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAApHDvrDW9W72vAr7h7XeCu7+Qz-_Vff02Q+RPPuVeM0Qf0MCw@mail.gmail.com>
References: <CAEudQAq_zOSA2NUQSWePTGV_=90Uw0WcXxGOWnN-vwF046OOqA@mail.gmail.com>
<[email protected]>
<CAApHDvqXdBWMwGuzJBSbq4eiNLDaR738-upd8NJpmhUq3a+XLQ@mail.gmail.com>
<[email protected]>
<CAApHDvpn3bshX+_2t0NbJ263r1etcBdmkROjuJw8DvyZkMcXqg@mail.gmail.com>
<[email protected]>
<CAApHDvp=pEkO6LKfXKDjRNeQ1LkV7A5fsTn29ahUBXxmjyWPHg@mail.gmail.com>
<[email protected]>
<CAApHDvrDW9W72vAr7h7XeCu7+Qz-_Vff02Q+RPPuVeM0Qf0MCw@mail.gmail.com>
On Sat, Apr 18, 2026 at 09:06:02PM +1200, David Rowley wrote:
> On Fri, 10 Oct 2025 at 11:30, Michael Paquier <[email protected]> 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.
>
> 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.
Fine by me.
> 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.
Using the existing HEAD approach where STRICT avoids these extra NULL
checks, or adding explicit NULL checks without STRICT does not strike
me as a big difference in this context.
> 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.
> 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.
These make sense.
> 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.
It is a test module, it would be a big issue if new pieces are
backpatched in this area. In short I'm fine with these. Thanks for
asking.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, 2-signature.asc)
download
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