Thread: A minor correction in comment in heaptuple.c

A minor correction in comment in heaptuple.c

From
Amit Langote
Date:
Hi,

Should it be: "return true if attnum is out of range according to the
tupdesc" instead of "return NULL if attnum is out of range according
to the tupdesc" at src/backend/access/common/heaptuple.c: 1345

/*
 * return true if attnum is out of range according to the tupdesc
*/
if (attnum > tupleDesc->natts)
return true;

Attached patch fixes this.

--
Amit Langote

Attachment

Re: A minor correction in comment in heaptuple.c

From
Andres Freund
Date:
Hi,

On 2013-06-18 17:56:34 +0900, Amit Langote wrote:
> Should it be: "return true if attnum is out of range according to the
> tupdesc" instead of "return NULL if attnum is out of range according
> to the tupdesc" at src/backend/access/common/heaptuple.c: 1345
> 
> /*
>  * return true if attnum is out of range according to the tupdesc
> */
> if (attnum > tupleDesc->natts)
> return true;

Well, true actually tells us that the attribute is null, since that's
the purpose of the function:

/** slot_attisnull*        Detect whether an attribute of the slot is null, without*        actually fetching it.*/

I think the comment is more meaningfull before the change since it tells
us how nonexisting columns are interpreted.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services



Re: A minor correction in comment in heaptuple.c

From
Amit Langote
Date:
On Tue, Jun 18, 2013 at 6:01 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> Hi,
>
> On 2013-06-18 17:56:34 +0900, Amit Langote wrote:
>> Should it be: "return true if attnum is out of range according to the
>> tupdesc" instead of "return NULL if attnum is out of range according
>> to the tupdesc" at src/backend/access/common/heaptuple.c: 1345
>>
>> /*
>>  * return true if attnum is out of range according to the tupdesc
>> */
>> if (attnum > tupleDesc->natts)
>> return true;
>
> Well, true actually tells us that the attribute is null, since that's
> the purpose of the function:
>
> /*
>  * slot_attisnull
>  *              Detect whether an attribute of the slot is null, without
>  *              actually fetching it.
>  */
>
> I think the comment is more meaningfull before the change since it tells
> us how nonexisting columns are interpreted.
>

Okay, that makes sense.


--
Amit Langote



Re: A minor correction in comment in heaptuple.c

From
"D'Arcy J.M. Cain"
Date:
On Tue, 18 Jun 2013 11:01:28 +0200
Andres Freund <andres@2ndquadrant.com> wrote:
> > /*
> >  * return true if attnum is out of range according to the tupdesc
> > */
> > if (attnum > tupleDesc->natts)
> > return true;
> 
> I think the comment is more meaningfull before the change since it
> tells us how nonexisting columns are interpreted.

I think that the comment is bad either way.  Comments should explain
the code, not repeat it.  The above is not far removed from...
 return 5; /* return the number 5 */

How about "check if attnum is out of range according to the tupdesc"
instead?

-- 
D'Arcy J.M. Cain <darcy@druid.net>         |  Democracy is three wolves
http://www.druid.net/darcy/                |  and a sheep voting on
+1 416 788 2246     (DoD#0082)    (eNTP)   |  what's for dinner.
IM: darcy@Vex.Net, VOIP: sip:darcy@Vex.Net



Re: A minor correction in comment in heaptuple.c

From
Andres Freund
Date:
On 2013-06-18 05:21:15 -0400, D'Arcy J.M. Cain wrote:
> On Tue, 18 Jun 2013 11:01:28 +0200
> Andres Freund <andres@2ndquadrant.com> wrote:
> > > /*
> > >  * return true if attnum is out of range according to the tupdesc
> > > */
> > > if (attnum > tupleDesc->natts)
> > > return true;
> > 
> > I think the comment is more meaningfull before the change since it
> > tells us how nonexisting columns are interpreted.
> 
> I think that the comment is bad either way.  Comments should explain
> the code, not repeat it.  The above is not far removed from...
> 
>   return 5; /* return the number 5 */
> 
> How about "check if attnum is out of range according to the tupdesc"
> instead?

I can't follow. Minus the word 'NULL' - which carries meaning - your
suggested comment pretty much is the same as the existing comment except
that you use 'check' instead of 'return'.

Original:/* * return NULL if attnum is out of range according to the tupdesc */if (attnum > tupleDesc->natts)    return
true;


Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services



Re: A minor correction in comment in heaptuple.c

From
Kevin Grittner
Date:
Andres Freund <andres@2ndquadrant.com> wrote:
> On 2013-06-18 05:21:15 -0400, D'Arcy J.M. Cain wrote:
>> On Tue, 18 Jun 2013 11:01:28 +0200
>> Andres Freund <andres@2ndquadrant.com> wrote:
>> > > /*
>> > >  * return true if attnum is out of range according to the tupdesc
>> > >  */
>> > > if (attnum > tupleDesc->natts)
>> > > return true;
>> >
>> > I think the comment is more meaningfull before the change since it
>> > tells us how nonexisting columns are interpreted.
>>
>> I think that the comment is bad either way.  Comments should explain
>> the code, not repeat it.  The above is not far removed from...
>>
>>   return 5; /* return the number 5 */

I agree with this -- the comment as it stands adds no information
to what is obvious from a glance at the code, and may waste the
time it takes to mentally resolve the discrepancy between "return
NULL" in the comment and "return true;" in the statement.  Unless
it adds information, we'd be better off deleting the comment.

>> How about "check if attnum is out of range according to the
>> tupdesc" instead?
>
> I can't follow. Minus the word 'NULL' - which carries meaning - your
> suggested comment pretty much is the same as the existing comment except
> that you use 'check' instead of 'return'.

The word "indicate" might waste a few milliseconds less in the
double-take; but better would be some explanation of why you might
have an attnum value greater than the maximum, and why the right
thing to do is indicate NULL rather than throwing an error.

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



Re: A minor correction in comment in heaptuple.c

From
"D'Arcy J.M. Cain"
Date:
On Tue, 18 Jun 2013 11:38:45 +0200
Andres Freund <andres@2ndquadrant.com> wrote:
> > How about "check if attnum is out of range according to the tupdesc"
> > instead?
> 
> I can't follow. Minus the word 'NULL' - which carries meaning - your
> suggested comment pretty much is the same as the existing comment
> except that you use 'check' instead of 'return'.

The difference is that I say what the purpose of the function is but
don't say what it actually returns.  The code itself does that.

> Original:
>     /*
>      * return NULL if attnum is out of range according to the
> tupdesc */

Obviously wrong so it should be changed.  As for the exact wording,
flip a coin and get the bikeshed painted.  It's not all that critical.
You could probably leave out the comment altogether.  The code is
pretty short and self explanatory.

Perhaps the comment should explain why we don't test for negative
numbers.  I assume that that's an impossible situation.

-- 
D'Arcy J.M. Cain <darcy@druid.net>         |  Democracy is three wolves
http://www.druid.net/darcy/                |  and a sheep voting on
+1 416 788 2246     (DoD#0082)    (eNTP)   |  what's for dinner.
IM: darcy@Vex.Net, VOIP: sip:darcy@Vex.Net



Re: A minor correction in comment in heaptuple.c

From
Andres Freund
Date:
On 2013-06-18 13:14:30 -0400, D'Arcy J.M. Cain wrote:
> On Tue, 18 Jun 2013 11:38:45 +0200
> Andres Freund <andres@2ndquadrant.com> wrote:
> > > How about "check if attnum is out of range according to the tupdesc"
> > > instead?
> > 
> > I can't follow. Minus the word 'NULL' - which carries meaning - your
> > suggested comment pretty much is the same as the existing comment
> > except that you use 'check' instead of 'return'.
> 
> The difference is that I say what the purpose of the function is but
> don't say what it actually returns.  The code itself does that.
>
> > Original:
> >     /*
> >      * return NULL if attnum is out of range according to the
> > tupdesc */
> 
> Obviously wrong so it should be changed.

The NULL refers to the *meaning* of the function (remember, it's called
slot_attisnull) . Which is to test whether an attribute is null. Not to
a C NULL.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services



Re: A minor correction in comment in heaptuple.c

From
"D'Arcy J.M. Cain"
Date:
On Tue, 18 Jun 2013 19:19:40 +0200
Andres Freund <andres@2ndquadrant.com> wrote:
> The NULL refers to the *meaning* of the function (remember, it's
> called slot_attisnull) . Which is to test whether an attribute is
> null. Not to a C NULL.

Actually, the comment is not for the function.  It only describes the
two lines that follow.  In C the string "NULL" is commonly a reference
to C's NULL value.  Anyone reading C code can be excused for assuming
that if it isn't otherwise clear.  How about "Indicate that the
attribute is NULL if out of range..."?

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.

-- 
D'Arcy J.M. Cain <darcy@druid.net>         |  Democracy is three wolves
http://www.druid.net/darcy/                |  and a sheep voting on
+1 416 788 2246     (DoD#0082)    (eNTP)   |  what's for dinner.
IM: darcy@Vex.Net, VOIP: sip:darcy@Vex.Net



Re: A minor correction in comment in heaptuple.c

From
Kevin Grittner
Date:
D'Arcy J.M. Cain <darcy@druid.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 (which passes `make
check-world`):

--- a/src/backend/access/common/heaptuple.c
+++ b/src/backend/access/common/heaptuple.c
@@ -1323,6 +1323,8 @@ slot_attisnull(TupleTableSlot *slot, int attnum)
    HeapTuple   tuple = slot->tts_tuple;
    TupleDesc   tupleDesc = slot->tts_tupleDescriptor;
 
+   Assert(attnum <= tupleDesc->natts);
+
    /*
     * system attributes are handled by heap_attisnull
     */
@@ -1342,12 +1344,6 @@ slot_attisnull(TupleTableSlot *slot, int attnum)
        return slot->tts_isnull[attnum - 1];
 
    /*
-    * return NULL if attnum is out of range according to the tupdesc
-    */
-   if (attnum > tupleDesc->natts)
-       return true;
-
-   /*
     * otherwise we had better have a physical tuple (tts_nvalid should equal
     * natts in all virtual-tuple cases)
     */

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



Re: A minor correction in comment in heaptuple.c

From
Bruce Momjian
Date:
On Tue, Jun 18, 2013 at 11:04:25AM -0700, Kevin Grittner wrote:
> D'Arcy J.M. Cain <darcy@druid.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 (which passes `make
> check-world`):

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

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +

Attachment

Re: A minor correction in comment in heaptuple.c

From
Andres Freund
Date:
On 2014-01-25 16:28:09 -0500, Bruce Momjian wrote:
> On Tue, Jun 18, 2013 at 11:04:25AM -0700, Kevin Grittner wrote:
> > D'Arcy J.M. Cain <darcy@druid.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 (which passes `make
> > check-world`):
>
> Is everyone OK with me applying this patch from Kevin, attached?

No. I still think this is stupid. Not at all clearer and possibly breaks
stuff.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services



Re: A minor correction in comment in heaptuple.c

From
Bruce Momjian
Date:
On Sat, Jan 25, 2014 at 10:29:36PM +0100, Andres Freund wrote:
> On 2014-01-25 16:28:09 -0500, Bruce Momjian wrote:
> > On Tue, Jun 18, 2013 at 11:04:25AM -0700, Kevin Grittner wrote:
> > > D'Arcy J.M. Cain <darcy@druid.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 (which passes `make
> > > check-world`):
> > 
> > Is everyone OK with me applying this patch from Kevin, attached?
> 
> No. I still think this is stupid. Not at all clearer and possibly breaks
> stuff.

OK, how about if we change the comment to this:
   /*
-->  * assume NULL if attnum is out of range according to the tupdesc    */   if (attnum > tupleDesc->natts)
returntrue;
 

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +



Re: A minor correction in comment in heaptuple.c

From
Andres Freund
Date:
On 2014-01-25 16:33:16 -0500, Bruce Momjian wrote:
> On Sat, Jan 25, 2014 at 10:29:36PM +0100, Andres Freund wrote:
> > On 2014-01-25 16:28:09 -0500, Bruce Momjian wrote:
> > > On Tue, Jun 18, 2013 at 11:04:25AM -0700, Kevin Grittner wrote:
> > > > D'Arcy J.M. Cain <darcy@druid.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 (which passes `make
> > > > check-world`):
> > >
> > > Is everyone OK with me applying this patch from Kevin, attached?
> >
> > No. I still think this is stupid. Not at all clearer and possibly breaks
> > stuff.
>
> OK, how about if we change the comment to this:
>
>     /*
> -->  * assume NULL if attnum is out of range according to the tupdesc
>      */
>     if (attnum > tupleDesc->natts)
>         return true;

I don't think it improves things relevantly, but it doesn't make
anything worse either. So if that makes anybody happy...

I think this style of pinhole copy editing is pretty pointless. There's
dozen checks just like this around. If somebody wants to change the rules
or improve comment it takes more than picking a random one.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services



Re: A minor correction in comment in heaptuple.c

From
Bruce Momjian
Date:
On Sat, Jan 25, 2014 at 10:40:28PM +0100, Andres Freund wrote:
> I don't think it improves things relevantly, but it doesn't make
> anything worse either. So if that makes anybody happy...
> 
> I think this style of pinhole copy editing is pretty pointless. There's
> dozen checks just like this around. If somebody wants to change the rules
> or improve comment it takes more than picking a random one.

OK, change made.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +



Re: A minor correction in comment in heaptuple.c

From
Tom Lane
Date:
Andres Freund <andres@2ndquadrant.com> writes:
> On 2014-01-25 16:28:09 -0500, Bruce Momjian wrote:
>> Is everyone OK with me applying this patch from Kevin, attached?

> No. I still think this is stupid. Not at all clearer and possibly breaks
> stuff.

I agree; this patch is flat out wrong.  It converts a valid situation
which is correctly handled into an Assert trap, or probably a core dump
in a non-assert build.
        regards, tom lane



Re: A minor correction in comment in heaptuple.c

From
Tom Lane
Date:
Bruce Momjian <bruce@momjian.us> writes:
> On Sat, Jan 25, 2014 at 10:40:28PM +0100, Andres Freund wrote:
>> I think this style of pinhole copy editing is pretty pointless. There's
>> dozen checks just like this around. If somebody wants to change the rules
>> or improve comment it takes more than picking a random one.

> OK, change made.

FWIW, I don't find that an improvement either.  As Andres says, this
is just applying the same rule that's used in many other places, ie
return null if the requested attnum is off the end of the tuple.
        regards, tom lane



Re: A minor correction in comment in heaptuple.c

From
Bruce Momjian
Date:
On Sat, Jan 25, 2014 at 04:56:37PM -0500, Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > On Sat, Jan 25, 2014 at 10:40:28PM +0100, Andres Freund wrote:
> >> I think this style of pinhole copy editing is pretty pointless. There's
> >> dozen checks just like this around. If somebody wants to change the rules
> >> or improve comment it takes more than picking a random one.
> 
> > OK, change made.
> 
> FWIW, I don't find that an improvement either.  As Andres says, this
> is just applying the same rule that's used in many other places, ie
> return null if the requested attnum is off the end of the tuple.

OK, I can revert it, but I don't see any other cases of the string
'return NULL if' in the executor code.  What the code really is doing is
"Assume NULL so return true if".  The code was never returning NULL, it
was assuming the attribute was NULL and returning true.  Am I missing
something?

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +



Re: A minor correction in comment in heaptuple.c

From
Andres Freund
Date:
On 2014-01-25 17:15:01 -0500, Bruce Momjian wrote:
> On Sat, Jan 25, 2014 at 04:56:37PM -0500, Tom Lane wrote:
> > Bruce Momjian <bruce@momjian.us> writes:
> > > On Sat, Jan 25, 2014 at 10:40:28PM +0100, Andres Freund wrote:
> > >> I think this style of pinhole copy editing is pretty pointless. There's
> > >> dozen checks just like this around. If somebody wants to change the rules
> > >> or improve comment it takes more than picking a random one.
> > 
> > > OK, change made.
> > 
> > FWIW, I don't find that an improvement either.  As Andres says, this
> > is just applying the same rule that's used in many other places, ie
> > return null if the requested attnum is off the end of the tuple.
> 
> OK, I can revert it, but I don't see any other cases of the string
> 'return NULL if' in the executor code.  What the code really is doing is
> "Assume NULL so return true if".  The code was never returning NULL, it
> was assuming the attribute was NULL and returning true.  Am I missing
> something?

The friggin function in which you whacked around the comment is called
"slot_attisnull()". Referring to the functions meaning in a comment
above an early return isn't a novel thing.

Just search for attnum > tupleDesc->natts to find lots of similar chunks
of code, several of them even in the same file.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services



Re: A minor correction in comment in heaptuple.c

From
Kevin Grittner
Date:
Bruce Momjian <bruce@momjian.us> wrote:
> On Tue, Jun 18, 2013 at 11:04:25AM -0700, Kevin Grittner wrote:
>> D'Arcy J.M. Cain <darcy@druid.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



Re: A minor correction in comment in heaptuple.c

From
Bruce Momjian
Date:
On Mon, Jan 27, 2014 at 02:51:59PM -0800, Kevin Grittner wrote:
> 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....

OK, so does anyone object to removing this comment line?

    slot_attisnull()
    ...
        /*
    -->  * assume NULL if attnum is out of range according to the tupdesc
         */
        if (attnum > tupleDesc->natts)
            return true;


--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +

Re: A minor correction in comment in heaptuple.c

From
Andres Freund
Date:
On 2014-01-28 11:14:49 -0500, Bruce Momjian wrote:
> On Mon, Jan 27, 2014 at 02:51:59PM -0800, Kevin Grittner wrote:
> > 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....
>
> OK, so does anyone object to removing this comment line?

Let's just not do anything. This is change for changes sake. Not
improving anything the slightest.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services



Re: A minor correction in comment in heaptuple.c

From
Tom Lane
Date:
Andres Freund <andres@2ndquadrant.com> writes:
> On 2014-01-28 11:14:49 -0500, Bruce Momjian wrote:
>> OK, so does anyone object to removing this comment line?

> Let's just not do anything. This is change for changes sake. Not
> improving anything the slightest.

Indeed.  I'd actually request that you revert your previous change to the
comment, as it didn't improve matters and is only likely to cause pain for
future back-patching.
        regards, tom lane



Re: A minor correction in comment in heaptuple.c

From
Bruce Momjian
Date:
On Tue, Jan 28, 2014 at 11:20:39AM -0500, Tom Lane wrote:
> Andres Freund <andres@2ndquadrant.com> writes:
> > On 2014-01-28 11:14:49 -0500, Bruce Momjian wrote:
> >> OK, so does anyone object to removing this comment line?
> 
> > Let's just not do anything. This is change for changes sake. Not
> > improving anything the slightest.
> 
> Indeed.  I'd actually request that you revert your previous change to the
> comment, as it didn't improve matters and is only likely to cause pain for
> future back-patching.

OK, so we have a don't change anything and a revert. I am thinking the
new wording as a super-minor improvement.  Anyone else want to vote?

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +



Re: A minor correction in comment in heaptuple.c

From
Alvaro Herrera
Date:
Bruce Momjian escribió:
> On Tue, Jan 28, 2014 at 11:20:39AM -0500, Tom Lane wrote:
> > Andres Freund <andres@2ndquadrant.com> writes:
> > > On 2014-01-28 11:14:49 -0500, Bruce Momjian wrote:
> > >> OK, so does anyone object to removing this comment line?
> > 
> > > Let's just not do anything. This is change for changes sake. Not
> > > improving anything the slightest.
> > 
> > Indeed.  I'd actually request that you revert your previous change to the
> > comment, as it didn't improve matters and is only likely to cause pain for
> > future back-patching.
> 
> OK, so we have a don't change anything and a revert. I am thinking the
> new wording as a super-minor improvement.  Anyone else want to vote?

I vote to revert to the original and can we please wait for longer than
a few hours on a weekend before applying this kind of change that is
obviously not without controversy.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services



Re: A minor correction in comment in heaptuple.c

From
Bruce Momjian
Date:
On Tue, Jan 28, 2014 at 02:25:51PM -0300, Alvaro Herrera wrote:
> Bruce Momjian escribió:
> > On Tue, Jan 28, 2014 at 11:20:39AM -0500, Tom Lane wrote:
> > > Andres Freund <andres@2ndquadrant.com> writes:
> > > > On 2014-01-28 11:14:49 -0500, Bruce Momjian wrote:
> > > >> OK, so does anyone object to removing this comment line?
> > > 
> > > > Let's just not do anything. This is change for changes sake. Not
> > > > improving anything the slightest.
> > > 
> > > Indeed.  I'd actually request that you revert your previous change to the
> > > comment, as it didn't improve matters and is only likely to cause pain for
> > > future back-patching.
> > 
> > OK, so we have a don't change anything and a revert. I am thinking the
> > new wording as a super-minor improvement.  Anyone else want to vote?
> 
> I vote to revert to the original and can we please wait for longer than
> a few hours on a weekend before applying this kind of change that is
> obviously not without controversy.

OK, reverted.  I have to question how well-balanced we are when a word
change in a C comment can cause so much contention.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +



Re: A minor correction in comment in heaptuple.c

From
Andres Freund
Date:
On 2014-01-28 12:29:25 -0500, Bruce Momjian wrote:
> On Tue, Jan 28, 2014 at 02:25:51PM -0300, Alvaro Herrera wrote:
> > Bruce Momjian escribió:
> > > On Tue, Jan 28, 2014 at 11:20:39AM -0500, Tom Lane wrote:
> > > > Andres Freund <andres@2ndquadrant.com> writes:
> > > > > On 2014-01-28 11:14:49 -0500, Bruce Momjian wrote:
> > > > >> OK, so does anyone object to removing this comment line?
> > > >
> > > > > Let's just not do anything. This is change for changes sake. Not
> > > > > improving anything the slightest.
> > > >
> > > > Indeed.  I'd actually request that you revert your previous change to the
> > > > comment, as it didn't improve matters and is only likely to cause pain for
> > > > future back-patching.
> > >
> > > OK, so we have a don't change anything and a revert. I am thinking the
> > > new wording as a super-minor improvement.  Anyone else want to vote?
> >
> > I vote to revert to the original and can we please wait for longer than
> > a few hours on a weekend before applying this kind of change that is
> > obviously not without controversy.
>
> OK, reverted.  I have to question how well-balanced we are when a word
> change in a C comment can cause so much contention.

The question is rather why to do such busywork changes in the first
place imo. Without ever looking at more than one a few lines up/down
especially.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services



Re: A minor correction in comment in heaptuple.c

From
Bruce Momjian
Date:
On Tue, Jan 28, 2014 at 06:30:40PM +0100, Andres Freund wrote:
> > OK, reverted.  I have to question how well-balanced we are when a word
> > change in a C comment can cause so much contention.
> 
> The question is rather why to do such busywork changes in the first
> place imo. Without ever looking at more than one a few lines up/down
> especially.

I see what you mean that the identical comment appears in the same C
file.  :-(

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +