Re: A minor correction in comment in heaptuple.c

From: Kevin Grittner <kgrittn(at)ymail(dot)com>
To: Bruce Momjian <bruce(at)momjian(dot)us>
Cc: "D'Arcy J(dot)M(dot) Cain" <darcy(at)druid(dot)net>, Andres Freund <andres(at)2ndquadrant(dot)com>, Amit Langote <amitlangote09(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: A minor correction in comment in heaptuple.c
Date: 2014-01-27 22:51:59
Message-ID: 1390863119.74880.YahooMailNeo@web122306.mail.ne1.yahoo.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> On Tue, Jun 18, 2013 at 11:04:25AM -0700, Kevin Grittner wrote:
>> D'Arcy J.M. Cain <darcy(at)druid(dot)net>
>>
>>> Although, the more I think about it, the more I think that the
>>> comment is both confusing and superfluous.  The code itself is
>>> much clearer.
>>
>> Seriously, if there is any comment there at all, it should be a
>> succinct explanation for why we didn't do this

> Is everyone OK with me applying this patch from Kevin, attached?

I guess my intent was misunderstood -- what I was trying to get
across was that the comment added exactly nothing to what you could
get more quickly by reading the code below it.  I felt the comment
should either be removed entirely, or a concise explanation of why
it was right thing to do should be there rather than just echoing
the code in English.  I wasn't suggesting applying the mini-patch,
but suggesting that *if* we have a comment there at all, it should
make clear why such a patch would be wrong; i.e., why is it is OK
to have attnum > tupdesc->natts here?  How would we get to such a
state, and why is NULL the right thing for it?  Such a comment
would actually help someone trying to understand the code, rather
than wasting their time.  After all, in the same file we have this:

    /* Check for caller error */
    if (attnum <= 0 || attnum > slot->tts_tupleDescriptor->natts)
        elog(ERROR, "invalid attribute number %d", attnum);

An explanation of why it is caller error one place and not another
isn't a waste of space.

>> (which passes `make check-world`)

And I was disappointed that our regression tests don't actually
exercise that code path, which would be another good way to make
the point.

So anyway, *I* would object to applying that; it was meant to
illustrate what the comment, if any, should cover; not to be an
actual code change.  I don't think the change that was pushed helps
that comment carry its own weight, either.  If we can't do better
than that, we should just drop it.

I guess I won't try to illustrate a point *that* particular way
again....

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Geoghegan 2014-01-27 22:54:52 Re: Add min and max execute statement time in pg_stat_statement
Previous Message Dean Rasheed 2014-01-27 22:28:27 Re: [PATCH] Negative Transition Aggregate Functions (WIP)