On 2019-May-10, Robert Haas wrote:
> Obviously, this macro does not do what it claims to do:
>
> /*
> * check to see if the ATT'th bit of an array of 8-bit bytes is set.
> */
> #define att_isnull(ATT, BITS) (!((BITS)[(ATT) >> 3] & (1 << ((ATT) & 0x07))))
>
> OK, I lied. It's not at all obvious, or at least it wasn't to me.
> The macro actually tests whether the bit is *unset*, because there's
> an exclamation point in there. I think the comment should be updated
> to something like "Check a tuple's null bitmap to determine whether
> the attribute is null. Note that a 0 in the null bitmap indicates a
> null, while 1 indicates non-null."
Yeah, I've noticed this inconsistency too. I doubt we want to change
the macro definition or its name, but +1 for expanding the comment.
Your proposed wording seems sufficient.
> There is some kind of broader confusion here, I think, because we
> refer in many places to the "null bitmap" but it's actually not a
> bitmap of which attributes are null but rather of which attributes are
> not null. That is confusing in and of itself, and it's also not very
> intuitive that it uses exactly the opposite convention from what we do
> with datum/isnull arrays.
I remember being bit by this inconsistency while fixing data corruption
problems, but I'm not sure what, if anything, should we do about it.
Maybe there's a perfect spot where to add some further documentation
about it (a code comment somewhere?) but I don't know where would that
be.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
