Re: Making empty Bitmapsets always be NULL

On Wed, Mar 01, 2023 at 05:59:45PM -0500, Tom Lane wrote:
> 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.)

Yeah, it's nice that you don't have to subtract
FirstLowInvalidHeapAttributeNumber in so many places.  I think future
changes might end up using attidx when they ought to use attrnum (and vice
versa), but you could just as easily forget to subtract
FirstLowInvalidHeapAttributeNumber today, so it's probably fine.

> +     /* attidx is zero-based, attrnum is the normal attribute number */
> +     int         attrnum = attidx + FirstLowInvalidHeapAttributeNumber;

nitpick: Shouldn't this be an AttrNumber?

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

These all looked reasonable to me.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com