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 |
Message-ID: | 1768604.1677711585@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
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 |
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 |