Re: Making empty Bitmapsets always be NULL

Nathan Bossart <nathandbossart@gmail.com> writes:
> On Tue, Feb 28, 2023 at 04:59:48PM -0500, Tom Lane wrote:
>> When I designed the Bitmapset module, I set things up so that an empty
>> Bitmapset could be represented either by a NULL pointer, or by an
>> allocated object all of whose bits are zero.  I've recently come to
>> the conclusion that that was a bad idea and we should instead have
>> a convention like the longstanding invariant for Lists: an empty
>> list is represented by NIL and nothing else.

> +1

Thanks for looking at this.

> Unless there is a way to avoid the invariant violation that doesn't involve
> scanning the rest of the words before bms_first_member returns, +1 to
> removing it.  Perhaps we could add a num_members variable to the struct so
> that we know right away when the set becomes empty.  But maintaining that
> might be more trouble than it's worth.

bms_first_member is definitely legacy code, so let's just get
rid of it.  Done like that in 0001 below.  (This was slightly more
complex than I foresaw, because some of the callers were modifying
the result variables.  But they're probably cleaner this way anyway.)

>> I also discovered that nodeAppend.c is relying on bms_del_members
>> not reducing a non-empty set to NULL, because it uses the nullness
>> of appendstate->as_valid_subplans as a state boolean.

> The separate boolean certainly seems less fragile.  That might even be
> worthwhile independent of the rest of the patch.

Yeah.  I split out those executor fixes as 0002; 0003 is the changes
to bitmapsets proper, and then 0004 removes now-dead code.

			regards, tom lane