Re: Incorrect comment in get_partition_dispatch_recurse

On Thu, May 17, 2018 at 10:13 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Maybe what you need is a redesign.  This convention seems impossibly
> confusing and hence error-prone.  What about using a separate bool to
> indicate which list the index refers to?

I really don't think it's a big deal.  The comments may need some
improvement (which is what we're doing here), but it's not as if there
are no other places in the PostgreSQL code where positive and negative
values indicate different kinds of things, user and system columns
being the most obvious such distinction. What we're doing here can't
possibly be more error-prone than adding and subtracting
FirstLowInvalidHeapAttributeNumber all over the place.

I'm biased, of course.  I invented this particular convention.  If
somebody else feels like redesigning it for a future release, and the
result is better than what we have now, cool.  But I do not think that
it would be a clear improvement to have a boolean indicating whether
or not the index is an index into subpartitions or leaf nodes and have
the index value always be positive.  In the current convention, if you
fail to handle negative values, you will probably crash.  With that
convention, if you forget to check the boolean and just assume you
have a leaf partition, it's quite likely that it will look like it
works, but do the wrong thing.  And as David points out, it also uses
less memory (which also makes it faster).

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company