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
> I found just a few places that have issues with this idea. One thing
> that is problematic is bms_first_member(): assuming you allow it to
> loop to completion, it ends with the passed Bitmapset being empty,
> which is now an invariant violation. I made it pfree the argument
> at that point, and fixed a couple of callers that would be broken
> thereby; but I wonder if it wouldn't be better to get rid of that
> function entirely and convert all its callers to use bms_next_member.
> There are only about half a dozen.
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.
> 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. That was
> probably acceptable when it was written, but whoever added
> classify_matching_subplans made a hash of the state invariants here,
> because that can set as_valid_subplans to empty. I added a separate
> boolean as an easy way out, but maybe that code could do with a more
> thorough revisit.
The separate boolean certainly seems less fragile. That might even be
worthwhile independent of the rest of the patch.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
