> On Aug 15, 2025, at 12:37 AM, David Rowley wrote:
>
> On Fri, 15 Aug 2025 at 15:24, Tom Lane wrote:
>>
>> David Rowley writes:
>>> I'm happy to push Greg's v5 patch if you have no counterarguments.
>>
>> In the end this isn't something I find worth arguing about. If
>> you prefer v5, sur
On Fri, 15 Aug 2025 at 15:24, Tom Lane wrote:
>
> David Rowley writes:
> > I'm happy to push Greg's v5 patch if you have no counterarguments.
>
> In the end this isn't something I find worth arguing about. If
> you prefer v5, sure. I do suggest though that if we're installing
> Asserts at all,
David Rowley writes:
> FWIW, after sleeping, I'm now very much against using < rather than <=
> for the Assert. The reason being that it makes it impossible to build
> bms_prev_member() loops with a dynamic start point. Right now we
> document that we expect the loop to be started with -1, but if
On Fri, 15 Aug 2025 at 07:45, Tom Lane wrote:
> Anyway, that's off-topic for the present thread. I believe it's
> middle-of-the-night in Rowley's time zone, so I was waiting for
> further comment from him before taking any action.
The v5 patch looks good to me.
FWIW, after sleeping, I'm now ver
> On Aug 14, 2025, at 3:45 PM, Tom Lane wrote:
>
> Greg Burd writes:
>> Spoiler alert, I have larger plans[2] for Bitmapset which is why I
>> started down this road to begin with as I wanted to capture the existing
>> API/contract for it before I proposed changes to it.
>
> FWIW, I'd be serio
Greg Burd writes:
> Spoiler alert, I have larger plans[2] for Bitmapset which is why I
> started down this road to begin with as I wanted to capture the existing
> API/contract for it before I proposed changes to it.
FWIW, I'd be seriously resistant to simply replacing Bitmapset with
something li
Tom, David,
I really appreciate the time spent and the thoughtful replies on this
thread today. Honestly, I thought this would be a nice and easy single
line fix ahead of a proposal for the addition of tests for Bitmapset[1]
(which identified the issue)
I think, with v5, we're fixing this small i
On Fri, 15 Aug 2025 at 03:14, Tom Lane wrote:
>
> David Rowley writes:
> > It is valid to pass prevbit as a->nwords * BITS_PER_BITMAPWORD as the
> > code does "prevbit--;". Maybe it would be less confusing if it were
> > written as:
> > * "prevbit" must be less than or equal to "a->nwords *
> >
On Aug 14 2025, at 11:49 am, David Rowley wrote:
> On Fri, 15 Aug 2025 at 03:43, Greg Burd wrote:
>> Well, that was rushed. Apologies.
>
> Would you be ok with adding the Assert after the "a == NULL" check?, i.e:
>
> if (a == NULL || prevbit == 0)
>return -2;
>
> /* Validate callers did
Greg Burd writes:
> Well, that was rushed. Apologies.
I was thinking something more like
/* transform -1 to the highest possible bit we could have set */
if (prevbit == -1)
prevbit = a->nwords * BITS_PER_BITMAPWORD - 1;
else
+{
+Assert(prevbit > 0 && prevbit
On Fri, 15 Aug 2025 at 03:43, Greg Burd wrote:
> Well, that was rushed. Apologies.
Would you be ok with adding the Assert after the "a == NULL" check?, i.e:
if (a == NULL || prevbit == 0)
return -2;
/* Validate callers didn't give us something out of range */
Assert(prevbit <= a->nwords *
On Aug 14 2025, at 11:37 am, Greg Burd wrote:
>
> On Aug 14 2025, at 11:14 am, Tom Lane wrote:
>
>> David Rowley writes:
>>> It is valid to pass prevbit as a->nwords * BITS_PER_BITMAPWORD as the
>>> code does "prevbit--;". Maybe it would be less confusing if it were
>>> written as:
>>> * "p
On Aug 14 2025, at 11:14 am, Tom Lane wrote:
> David Rowley writes:
>> It is valid to pass prevbit as a->nwords * BITS_PER_BITMAPWORD as the
>> code does "prevbit--;". Maybe it would be less confusing if it were
>> written as:
>> * "prevbit" must be less than or equal to "a->nwords * BITS_PER_
David Rowley writes:
> It is valid to pass prevbit as a->nwords * BITS_PER_BITMAPWORD as the
> code does "prevbit--;". Maybe it would be less confusing if it were
> written as:
> * "prevbit" must be less than or equal to "a->nwords * BITS_PER_BITMAPWORD".
> The Assert should be using <= rather th
On Fri, 15 Aug 2025 at 02:21, Greg Burd wrote:
> I found this too, and also the "one above" part seemed wrong to me as well.
It is valid to pass prevbit as a->nwords * BITS_PER_BITMAPWORD as the
code does "prevbit--;". Maybe it would be less confusing if it were
written as:
* "prevbit" must be
On Aug 14 2025, at 10:06 am, Tom Lane wrote:
> David Rowley writes:
>> There's a comment saying:
>> * "prevbit" must NOT be more than one above the highest possible bit
>> that can
>> * be set at the Bitmapset at its current size.
>> So looks like it's the fault of the calling code and not a
On Aug 14 2025, at 9:46 am, David Rowley wrote:
> On Fri, 15 Aug 2025 at 01:21, Greg Burd wrote:
>> I've been working on Bitmapset and while creating a test suite for it I
>> found that there is a missing bounds check in bms_prev_member(). The
>> function takes the prevbit argument and converts
On Fri, 15 Aug 2025 at 02:06, Tom Lane wrote:
> David Rowley writes:
> > So looks like it's the fault of the calling code and not an issue with
> > bms_prev_member().
>
> Yeah. But perhaps it'd be reasonable to put in an Assert checking
> that?
Seems like a good idea.
> Grammar nitpick: commen
David Rowley writes:
> There's a comment saying:
> * "prevbit" must NOT be more than one above the highest possible bit that can
> * be set at the Bitmapset at its current size.
> So looks like it's the fault of the calling code and not an issue with
> bms_prev_member().
Yeah. But perhaps it'd
On Fri, 15 Aug 2025 at 01:21, Greg Burd wrote:
> I've been working on Bitmapset and while creating a test suite for it I
> found that there is a missing bounds check in bms_prev_member(). The
> function takes the prevbit argument and converts it to an index into the
> words array using WORDNUM() w
Hello,
I've been working on Bitmapset and while creating a test suite for it I
found that there is a missing bounds check in bms_prev_member(). The
function takes the prevbit argument and converts it to an index into the
words array using WORDNUM() without checking to ensure that prevbit is
within
21 matches
Mail list logo
