Обсуждение: 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
Вложения
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
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
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
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
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
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
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
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
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
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. +
Вложения
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
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. +
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
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. +
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
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
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. +
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
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
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. +
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
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
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. +
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
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. +
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
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. +