MIME-Version: 1.0
References: 
 <CAApHDvq0T=iJ0Sf5TNE9yyWwfOeVjmrBt0wSywDnGD9Y4YJQBA@mail.gmail.com>
 <0BF8BC4F-ED7D-445F-8BB3-2336F1E17CB4@gmail.com>
In-Reply-To: <0BF8BC4F-ED7D-445F-8BB3-2336F1E17CB4@gmail.com>
From: David Rowley <dgrowleyml@gmail.com>
Date: Fri, 3 Apr 2026 11:20:50 +1300
Message-ID: 
 <CAApHDvpFAjbiNQpCWanuJb4o4=ZbLYtsxLnzYWhCbjYZuK2kfQ@mail.gmail.com>
Subject: Re: Small and unlikely overflow hazard in bms_next_member()
To: Chao Li <li.evan.chao@gmail.com>
Cc: PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAApHDvpFAjbiNQpCWanuJb4o4%3DZbLYtsxLnzYWhCbjYZuK2kfQ%40mail.gmail.com>
Precedence: bulk

On Thu, 2 Apr 2026 at 19:39, Chao Li <li.evan.chao@gmail.com> wrote:
> Both the return value of bms_next_member() and the parameter =E2=80=9Cpre=
vbit" are defined as int, which seems to imply that a bitmap can hold at mo=
st INT32_MAX elements. So I wonder if a cleaner fix would be to just add a =
range guard, like this:
>
> ```
> diff --git a/src/backend/nodes/bitmapset.c b/src/backend/nodes/bitmapset.=
c
> index 786f343b3c9..7f79f81f278 100644
> --- a/src/backend/nodes/bitmapset.c
> +++ b/src/backend/nodes/bitmapset.c
> @@ -1294,7 +1294,7 @@ bms_next_member(const Bitmapset *a, int prevbit)
>
>         Assert(bms_is_valid_set(a));
>
> -       if (a =3D=3D NULL)
> +       if (a =3D=3D NULL || prevbit =3D=3D INT32_MAX)

I don't think that's a nice way at all. Adding a special case plus
extra instructions, including a new jump instruction for the boolean
short-circuiting. More instruction decoding, more L1i space needed,
more branches to predict and more space in the branch predictor tables
to overwrite other useful branches. Adds run-time overhead.

I was aiming for low overhead and no special cases. 2a600a93c means we
don't care about the performance on 32-bit hardware anymore, so that
can't be used as a counterargument.

David