Re: [HACKERS] A minor correction in comment in heaptuple.c
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 br...@momjian.ushttp://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
Re: [HACKERS] A minor correction in comment in heaptuple.c
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
Re: [HACKERS] A minor correction in comment in heaptuple.c
Andres Freund and...@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
Re: [HACKERS] A minor correction in comment in heaptuple.c
On Tue, Jan 28, 2014 at 11:20:39AM -0500, Tom Lane wrote: Andres Freund and...@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 br...@momjian.ushttp://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
Re: [HACKERS] A minor correction in comment in heaptuple.c
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 and...@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 br...@momjian.ushttp://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
Re: [HACKERS] A minor correction in comment in heaptuple.c
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 and...@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
Re: [HACKERS] A minor correction in comment in heaptuple.c
Bruce Momjian escribió: On Tue, Jan 28, 2014 at 11:20:39AM -0500, Tom Lane wrote: Andres Freund and...@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 Herrerahttp://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
Re: [HACKERS] A minor correction in comment in heaptuple.c
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 br...@momjian.ushttp://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
Re: [HACKERS] A minor correction in comment in heaptuple.c
Bruce Momjian br...@momjian.us wrote: On Tue, Jun 18, 2013 at 11:04:25AM -0700, Kevin Grittner wrote: D'Arcy J.M. Cain da...@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
Re: [HACKERS] A minor correction in comment in heaptuple.c
On Tue, Jun 18, 2013 at 11:04:25AM -0700, Kevin Grittner wrote:
D'Arcy J.M. Cain da...@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 br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] A minor correction in comment in heaptuple.c
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 da...@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
Re: [HACKERS] A minor correction in comment in heaptuple.c
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 da...@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 br...@momjian.ushttp://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
Re: [HACKERS] A minor correction in comment in heaptuple.c
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 da...@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
Re: [HACKERS] A minor correction in comment in heaptuple.c
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 br...@momjian.ushttp://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
Re: [HACKERS] A minor correction in comment in heaptuple.c
Andres Freund and...@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
Re: [HACKERS] A minor correction in comment in heaptuple.c
Bruce Momjian br...@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
Re: [HACKERS] A minor correction in comment in heaptuple.c
On Sat, Jan 25, 2014 at 04:56:37PM -0500, Tom Lane wrote: Bruce Momjian br...@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 br...@momjian.ushttp://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
Re: [HACKERS] A minor correction in comment in heaptuple.c
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 br...@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
Re: [HACKERS] A minor correction in comment in heaptuple.c
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
Re: [HACKERS] A minor correction in comment in heaptuple.c
On Tue, Jun 18, 2013 at 6:01 PM, Andres Freund and...@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
Re: [HACKERS] A minor correction in comment in heaptuple.c
On Tue, 18 Jun 2013 11:01:28 +0200 Andres Freund and...@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 da...@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: da...@vex.net, VOIP: sip:da...@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
Re: [HACKERS] A minor correction in comment in heaptuple.c
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 and...@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
Re: [HACKERS] A minor correction in comment in heaptuple.c
Andres Freund and...@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 and...@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
Re: [HACKERS] A minor correction in comment in heaptuple.c
On Tue, 18 Jun 2013 11:38:45 +0200 Andres Freund and...@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 da...@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: da...@vex.net, VOIP: sip:da...@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
Re: [HACKERS] A minor correction in comment in heaptuple.c
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 and...@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
Re: [HACKERS] A minor correction in comment in heaptuple.c
On Tue, 18 Jun 2013 19:19:40 +0200 Andres Freund and...@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 da...@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: da...@vex.net, VOIP: sip:da...@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
Re: [HACKERS] A minor correction in comment in heaptuple.c
D'Arcy J.M. Cain da...@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
