On Wed, 15 Apr 2026 at 12:29, Tom Lane <[email protected]> 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
