While working on [1], I noticed some strange code in
DiscreteKnapsack() which seems to be aiming to copy the Bitmapset.
It's not that obvious at a casual glance, but:
sets[j] = bms_del_members(sets[j], sets[j]);
this is aiming to zero all the words in the set by passing the same
set in both parameters.
Now that 00b41463c changed Bitmapset to have NULL be the only valid
representation of an empty set, this code no longer makes sense. We
may as well just bms_free() the original set and bms_copy() in the new
set as the bms_del_members() call will always pfree the set anyway.
I've done that in the attached.
I did consider if we might want bms_merge_members() or
bms_exchange_members() or some other function suitably named function
to perform a del/add operation, but given the lack of complaints about
any performance regressions here, I think it's not worthwhile.
The code could also be adjusted to:
sets[j] = bms_add_members(sets[j], sets[ow]);
sets[j] = bms_del_members(sets[j], sets[j]);
sets[j] = bms_add_members(sets[j], sets[ow]); // re-add any deletions
so that the set never becomes fully empty... but ... that's pretty horrid.
00b41463c is in PG16, but I'm not proposing to backpatch this. The
misleading comment does not seem critical enough and the resulting
behaviour isn't changing, just the performance characteristics.
Unless there's some objection, I plan to push this in the next day or two.
David
[1] https://postgr.es/m/CAApHDvoXPoaDYEjMj9e1ihZZZynCtGqdAppWgPZMaMQ222NAkw@mail.gmail.com