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
