MIME-Version: 1.0
References: 
 <CAApHDvq=eEdw2Qp+aSzSOtTSe+h0fnVQ55CcTNqBkLDYiRZmxw@mail.gmail.com>
 <278B9FE3-F349-4494-99C5-483105C1C999@gmail.com>
 <CAApHDvoALVPgVSCuGdAfNUbxx_NacnAd0nuj8Hi=_-61AEqzaQ@mail.gmail.com>
 <1900289.1776212948@sss.pgh.pa.us>
In-Reply-To: <1900289.1776212948@sss.pgh.pa.us>
From: David Rowley <dgrowleyml@gmail.com>
Date: Wed, 15 Apr 2026 14:21:36 +1200
Message-ID: 
 <CAApHDvq4_WDfTaWX6nHU8TV+no+aD1zjLphza685Ek7=xgGzew@mail.gmail.com>
Subject: Re: Add bms_offset_members() function for bitshifting Bitmapsets
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Chao Li <li.evan.chao@gmail.com>,
	PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CAApHDvq4_WDfTaWX6nHU8TV%2Bno%2BaD1zjLphza685Ek7%3DxgGzew%40mail.gmail.com>
Precedence: bulk

On Wed, 15 Apr 2026 at 12:29, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I question the decision to make this change the set in-place.
> Wouldn't it be cheaper and less surprise-prone to always make
> a copy?

I'd not considered surprise-prone as an aspect. I understand we have
bms_join and bms_union, which do the same thing if you only care about
the value of the result and not what happens to the inputs. So I
didn't think I was introducing anything too surpising given we've got
various other Bitmapset functions that modify the input in-place. My
expectation there was that people are used to checking what the
behaviour of the bitmapset function is.

For the current use cases of the function in the patch, I agree that
it would likely be better for performance if the new function
allocated a new set. It was more a question of whether we want to
throw away performance for other cases which are fine with an in-place
update and have a positive offset. Those will never repalloc(). I
didn't really know the answer. I suspect with the current patch that
each of the existing cases will be faster than today's bms_next_member
loops, regardless.  When I wrote the function, I was mainly thinking
of the new use-case that I was working on, which won't require any
palloc() or repalloc() with the current design. Since I was adding
that to a fairly common code path, I thought I had more of a chance of
not having to jump through too many hoops to ensure I don't cause any
performance regressions.

In short, I don't really know what's best. I'm certainly open to
changing it if someone comes up with a good reason to do it the other
way. Maybe catering for the majority of callers is a good enough
reason to change it.

David