Re: Making empty Bitmapsets always be NULL

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Nathan Bossart <nathandbossart(at)gmail(dot)com>
Cc: pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: Making empty Bitmapsets always be NULL
Date: 2023-03-01 22:59:45
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Nathan Bossart <nathandbossart(at)gmail(dot)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

Attachment Content-Type Size
v2-0001-Remove-bms_first_member.patch text/x-diff 9.2 KB
v2-0002-Mop-up-some-undue-familiarity-with-the-innards-of.patch text/x-diff 6.6 KB
v2-0003-Require-empty-Bitmapsets-to-be-represented-as-NUL.patch text/x-diff 8.0 KB
v2-0004-Remove-local-optimizations-of-empty-Bitmapsets-in.patch text/x-diff 8.0 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2023-03-01 23:04:50 Re: typedef struct LogicalDecodingContext
Previous Message Nathan Bossart 2023-03-01 22:47:51 Re: stopgap fix for signal handling during restore_command