A minor correction in comment in heaptuple.c
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
Attachments:
correct-a-comment-in-heaptupledotc.patchapplication/octet-stream; name=correct-a-comment-in-heaptupledotc.patchDownload
diff --git a/src/backend/access/common/heaptuple.c b/src/backend/access/common/heaptuple.c
index e39b977..db93520 100644
--- a/src/backend/access/common/heaptuple.c
+++ b/src/backend/access/common/heaptuple.c
@@ -1342,7 +1342,7 @@ slot_attisnull(TupleTableSlot *slot, int attnum)
return slot->tts_isnull[attnum - 1];
/*
- * return NULL if attnum is out of range according to the tupdesc
+ * return true if attnum is out of range according to the tupdesc
*/
if (attnum > tupleDesc->natts)
return true;
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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. +
Attachments:
assert.difftext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/access/common/heaptuple.c b/src/backend/access/common/heaptuple.c
new file mode 100644
index aea9d40..7616cfc
*** a/src/backend/access/common/heaptuple.c
--- b/src/backend/access/common/heaptuple.c
*************** bool
*** 1321,1327 ****
slot_attisnull(TupleTableSlot *slot, int attnum)
{
HeapTuple tuple = slot->tts_tuple;
! TupleDesc tupleDesc = slot->tts_tupleDescriptor;
/*
* system attributes are handled by heap_attisnull
--- 1321,1328 ----
slot_attisnull(TupleTableSlot *slot, int attnum)
{
HeapTuple tuple = slot->tts_tuple;
!
! Assert(attnum <= slot->tts_tupleDescriptor->natts);
/*
* system attributes are handled by heap_attisnull
*************** slot_attisnull(TupleTableSlot *slot, int
*** 1342,1353 ****
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)
*/
--- 1343,1348 ----
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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;
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers