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