Horton

Thread

Commits

  1. Improve comment for att_isnull.

  1. att_isnull

    Robert Haas <robertmhaas@gmail.com> — 2019-05-10T14:20:05Z

    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

  2. Re: att_isnull

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-05-10T14:34:35Z

    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

  3. Re: att_isnull

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-05-10T14:46:06Z

    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

  4. Re: att_isnull

    Robert Haas <robertmhaas@gmail.com> — 2019-05-13T17:20:26Z

    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