Обсуждение: att_isnull
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." 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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
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
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> 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.
+1
>> 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.
It is documented in the "Database Physical Storage" part of the docs,
but no particular emphasis is laid on the 1-vs-0 convention. Maybe
a few more words there are worthwhile?
regards, tom lane
On Fri, May 10, 2019 at 10:46 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Alvaro Herrera <alvherre@2ndquadrant.com> writes: > > 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. > > +1 OK, committed. I assume nobody is going to complain that such changes are off-limits during feature freeze, but maybe I'll be unpleasantly surprised. > > 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. > > It is documented in the "Database Physical Storage" part of the docs, > but no particular emphasis is laid on the 1-vs-0 convention. Maybe > a few more words there are worthwhile? To me it seems like we more need to emphasize it in the code comments, but I have no concrete proposal. I don't think this is an urgent problem that needs to consume a lot of cycles right now, but I thought it was worth mentioning for the archives and just to get the idea out there that maybe we could do better someday. (My first idea was to deadpan a proposal that we reverse the convention, but then I realized that trolling the list might not be my best strategy.) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company