| From: | "Greg Burd" <greg(at)burd(dot)me> |
|---|---|
| To: | "David Rowley" <dgrowleyml(at)gmail(dot)com>, "Peter Eisentraut" <peter(at)eisentraut(dot)org> |
| Cc: | "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us>, "Chao Li" <li(dot)evan(dot)chao(at)gmail(dot)com>, "PostgreSQL Hackers" <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: Add bms_offset_members() function for bitshifting Bitmapsets |
| Date: | 2026-08-24 06:25:08 |
| Message-ID: | 66d041b4-32f4-4097-bb2c-0e7147337a5e@app.fastmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Sat, Aug 22, 2026, at 9:38 PM, David Rowley wrote:
> On Thu, 9 Jul 2026 at 11:55, David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
>> I pushed a change to make seed int64 instead. It's still possible to
>> specify the full range of possible values that GetCurrentTimestamp()
>> can (or could ever) return with the int64 type. This way, there's no
>> longer a mismatch in the format specifier for elog and the type we're
>> formatting. That's an improvement, so thanks.
Hey David,
> I was just in this area again and noticed that the nmembers
> calculation does not take into account the min_value parameter. The
> bad code is:
>
> nmembers = pg_prng_uint32(&state) % max_range + min_value;
Indeed, good catch.
> That should really be subtracting the min_value, as I had intended
> nmembers to do was populate a random set of members in the range
> specified by the function. While there, I noticed that the parameter
> names are not very consistently named and having the max_value before
> the min_value seems weird too.
I agree that is an implicit convention and is more intuitive.
> I understand test_random_operations()
> has it that way. I didn't adjust that function. Maybe that could be
> done separately since it's also in v19, whereas
> test_random_offset_operations() is new to v20.
I think this is a good idea, simply to be consistent/tidy and re-enforce the pattern for future work.
> I've attached a patch which fixes this plus a few more things that
> Claude code pointed out to me when I asked it to review my changes.
> Those are primarily around overflow risk if max_value was given as
> INT32_MAX.
I applied, reviewed, and tested the v1 patch. I think it is good and necessary for the reasons you've outlined. I'd happily help or implement the additional test_random_operations() for v20/v19 when I get a second (currently traveling) if you're not going to do so.
Thanks for continuing to improve this testing module.
best.
-greg
> David
>
> Attachments:
> * v1-0001-Fix-incorrect-nmembers-calculation-in-test_bitmap.patch
| From | Date | Subject | |
|---|---|---|---|
| Next Message | shveta malik | 2026-08-24 06:27:17 | Re: Support EXCEPT for TABLES IN SCHEMA publications |
| Previous Message | Peter Smith | 2026-08-24 06:13:05 | Re: Support EXCEPT for TABLES IN SCHEMA publications |