Re: [HACKERS] TRUE/FALSE vs true/false
On Sat, Aug 25, 2012 at 12:26:30PM -0400, Robert Haas wrote: > > So a patch of 1K lines would by itself represent about 2% of the typical > > inter-branch delta. Maybe that's below our threshold of pain, or maybe > > it isn't. I'd be happier about it if there were a more compelling > > argument for it, but to me it looks like extremely trivial neatnik-ism. > > I wouldn't mind a bit if we devoted 2% of our inter-branch deltas to > this sort of thing, but I've got to admit that 2% for one patch seems > a little on the high side to me. It might not be a bad idea to > establish one formulation or the other as the one to be used in all > new code, though, to avoid making the problem worse. Patch withdrawn. If we ever do a major code churn, it might be good to revisit this cleanup. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] TRUE/FALSE vs true/false
On Thu, Aug 23, 2012 at 11:01 AM, Tom Lane wrote: > Bruce Momjian writes: >> On Mon, Aug 20, 2012 at 03:09:08PM -0400, Robert Haas wrote: >>> I think the thing we need to look at is what percentage of our code >>> churn is coming from stuff like this, versus what percentage of it is >>> coming from other factors. If we change 250,000 lines of code per >>> release cycle and of that this kind of thing accounts for 5,000 lines >>> of deltas, then IMHO it's not really material. If it accounts for >>> 50,000 lines of deltas out of the same base, that's probably more than >>> can really be justified by the benefit we're going to get out of it. > >> The true/false capitalization patch changes 1.2k lines. > > I did a quick look at git diff --stat between recent branches: > > $ git diff --shortstat REL9_0_9 REL9_1_5 > 3186 files changed, 314847 insertions(+), 210452 deletions(-) > $ git diff --shortstat REL9_1_5 REL9_2_BETA4 > 2037 files changed, 290919 insertions(+), 189487 deletions(-) > > However, when you look at things a bit closer, these numbers are > misleading because they include the .po files, which seem to have huge > inter-branch churn --- well in excess of 10 lines changed per > release, at least in git's simpleminded view. Excluding those, as well > as src/test/isolation/expected/prepared-transactions.out which added > 34843 lines all by itself, I get > 173080 insertions, 70300 deletions for 9.0.9 -> 9.1.5 > 130706 insertions, 55714 deletions for 9.1.5 -> 9.2beta4. > So it looks like we touch order-of-magnitude of 100K lines per release; > which still seems astonishingly high, but then this includes docs and > regression tests not just code. If I restrict the stat to *.[chyl] > files it's about half that: > > $ git diff --numstat REL9_0_9 REL9_1_5 | grep '\.[chyl]$' | awk '{a += $1; b > += $2} > END{print a,b}' > 90234 33902 > $ git diff --numstat REL9_1_5 REL9_2_BETA4 | grep '\.[chyl]$' | awk '{a += > $1; b += $2} > END{print a,b}' > 90200 42218 > > So a patch of 1K lines would by itself represent about 2% of the typical > inter-branch delta. Maybe that's below our threshold of pain, or maybe > it isn't. I'd be happier about it if there were a more compelling > argument for it, but to me it looks like extremely trivial neatnik-ism. I wouldn't mind a bit if we devoted 2% of our inter-branch deltas to this sort of thing, but I've got to admit that 2% for one patch seems a little on the high side to me. It might not be a bad idea to establish one formulation or the other as the one to be used in all new code, though, to avoid making the problem worse. -- Robert Haas 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] TRUE/FALSE vs true/false
Bruce Momjian writes: > On Mon, Aug 20, 2012 at 03:09:08PM -0400, Robert Haas wrote: >> I think the thing we need to look at is what percentage of our code >> churn is coming from stuff like this, versus what percentage of it is >> coming from other factors. If we change 250,000 lines of code per >> release cycle and of that this kind of thing accounts for 5,000 lines >> of deltas, then IMHO it's not really material. If it accounts for >> 50,000 lines of deltas out of the same base, that's probably more than >> can really be justified by the benefit we're going to get out of it. > The true/false capitalization patch changes 1.2k lines. I did a quick look at git diff --stat between recent branches: $ git diff --shortstat REL9_0_9 REL9_1_5 3186 files changed, 314847 insertions(+), 210452 deletions(-) $ git diff --shortstat REL9_1_5 REL9_2_BETA4 2037 files changed, 290919 insertions(+), 189487 deletions(-) However, when you look at things a bit closer, these numbers are misleading because they include the .po files, which seem to have huge inter-branch churn --- well in excess of 10 lines changed per release, at least in git's simpleminded view. Excluding those, as well as src/test/isolation/expected/prepared-transactions.out which added 34843 lines all by itself, I get 173080 insertions, 70300 deletions for 9.0.9 -> 9.1.5 130706 insertions, 55714 deletions for 9.1.5 -> 9.2beta4. So it looks like we touch order-of-magnitude of 100K lines per release; which still seems astonishingly high, but then this includes docs and regression tests not just code. If I restrict the stat to *.[chyl] files it's about half that: $ git diff --numstat REL9_0_9 REL9_1_5 | grep '\.[chyl]$' | awk '{a += $1; b += $2} END{print a,b}' 90234 33902 $ git diff --numstat REL9_1_5 REL9_2_BETA4 | grep '\.[chyl]$' | awk '{a += $1; b += $2} END{print a,b}' 90200 42218 So a patch of 1K lines would by itself represent about 2% of the typical inter-branch delta. Maybe that's below our threshold of pain, or maybe it isn't. I'd be happier about it if there were a more compelling argument for it, but to me it looks like extremely trivial neatnik-ism. 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] TRUE/FALSE vs true/false
On Mon, Aug 20, 2012 at 03:09:08PM -0400, Robert Haas wrote: > I have difficult believing that a change of this type, if implemented > judiciously, is really going to create that much difficulty in > back-patching. I don't do as much back-patching as Tom either (no one > does), but most of the patches I do back-patch can be cherry-picked > all the way back without a problem. Some require adjustment, but even > then this kind of thing is pretty trivial to handle, as it's pretty > obvious what happened when you look through it. The really nasty > problems tend to come from places where the code has been rearranged, > rather than simple A-for-B substitutions. > > I think the thing we need to look at is what percentage of our code > churn is coming from stuff like this, versus what percentage of it is > coming from other factors. If we change 250,000 lines of code per > release cycle and of that this kind of thing accounts for 5,000 lines > of deltas, then IMHO it's not really material. If it accounts for > 50,000 lines of deltas out of the same base, that's probably more than > can really be justified by the benefit we're going to get out of it. The true/false capitalization patch changes 1.2k lines. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] TRUE/FALSE vs true/false
On Thu, Aug 16, 2012 at 3:32 PM, Bruce Momjian wrote: > On Thu, Aug 16, 2012 at 02:21:12PM -0500, Kevin Grittner wrote: >> Bruce Momjian wrote: >> >> > So what do we want to do with this? I am a little concerned that >> > we are sacrificing code clarity for backpatching ease, but I don't >> > do as much backpatching as Tom. >> >> Well, if you back-patched this change, it would eliminate the issue >> for Tom, wouldn't it? Not sure if that's sane; just a thought. > > I would be worried about some instability in backpatching. I was > looking for an 'ignore-case' mode to patch, but I don't see it. I have difficult believing that a change of this type, if implemented judiciously, is really going to create that much difficulty in back-patching. I don't do as much back-patching as Tom either (no one does), but most of the patches I do back-patch can be cherry-picked all the way back without a problem. Some require adjustment, but even then this kind of thing is pretty trivial to handle, as it's pretty obvious what happened when you look through it. The really nasty problems tend to come from places where the code has been rearranged, rather than simple A-for-B substitutions. I think the thing we need to look at is what percentage of our code churn is coming from stuff like this, versus what percentage of it is coming from other factors. If we change 250,000 lines of code per release cycle and of that this kind of thing accounts for 5,000 lines of deltas, then IMHO it's not really material. If it accounts for 50,000 lines of deltas out of the same base, that's probably more than can really be justified by the benefit we're going to get out of it. -- Robert Haas 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] TRUE/FALSE vs true/false
On Thu, Aug 16, 2012 at 02:21:12PM -0500, Kevin Grittner wrote: > Bruce Momjian wrote: > > > So what do we want to do with this? I am a little concerned that > > we are sacrificing code clarity for backpatching ease, but I don't > > do as much backpatching as Tom. > > Well, if you back-patched this change, it would eliminate the issue > for Tom, wouldn't it? Not sure if that's sane; just a thought. I would be worried about some instability in backpatching. I was looking for an 'ignore-case' mode to patch, but I don't see it. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] TRUE/FALSE vs true/false
Bruce Momjian wrote: > So what do we want to do with this? I am a little concerned that > we are sacrificing code clarity for backpatching ease, but I don't > do as much backpatching as Tom. Well, if you back-patched this change, it would eliminate the issue for Tom, wouldn't it? Not sure if that's sane; just a thought. -Kevin -- 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] TRUE/FALSE vs true/false
On Tue, Aug 14, 2012 at 10:57:08PM -0400, Peter Eisentraut wrote: > On Tue, 2012-08-14 at 17:36 -0400, Bruce Momjian wrote: > > On Tue, Aug 14, 2012 at 05:34:02PM -0400, Tom Lane wrote: > > > Bruce Momjian writes: > > > > On Thu, Aug 4, 2011 at 09:00:11PM +0300, Peter Eisentraut wrote: > > > >> On tor, 2011-08-04 at 14:44 +0200, Boszormenyi Zoltan wrote: > > > >>> I meant a mass "sed -e 's/TRUE/true/g' -e 's/FALSE/false/g'" run > > > >>> so all the ~200 occurrences of both "TRUE" and "FALSE" get > > > >>> converted so the whole source tree is consistent. > > > > > > >> I would be in favor of that. > > > > > > > I have implemented this with the patch at: > > > > http://momjian.us/expire/true_diff.txt > > > > > > Does this really do anything for us that will justify the extra > > > back-patching pain it will cause? I don't see that it's improving > > > code readability any. > > > > I think it is more of a consistency issue. There were multiple people > > who wanted this change. Of course, some of those people don't backport > > stuff. > > I guess you could argue this particular case without end, but I think we > should be open to these kinds of changes. They will make future code > easier to deal with and confuse new developers less (when to use which, > do they mean different things, etc.). > > If back-patching really becomes a problem, we might want to look a > little deeper into git options. I think the Linux kernel people do > these kinds of cleanups more often, so there is probably some better > support for it. So what do we want to do with this? I am a little concerned that we are sacrificing code clarity for backpatching ease, but I don't do as much backpatching as Tom. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] TRUE/FALSE vs true/false
On Tue, 2012-08-14 at 17:36 -0400, Bruce Momjian wrote: > On Tue, Aug 14, 2012 at 05:34:02PM -0400, Tom Lane wrote: > > Bruce Momjian writes: > > > On Thu, Aug 4, 2011 at 09:00:11PM +0300, Peter Eisentraut wrote: > > >> On tor, 2011-08-04 at 14:44 +0200, Boszormenyi Zoltan wrote: > > >>> I meant a mass "sed -e 's/TRUE/true/g' -e 's/FALSE/false/g'" run > > >>> so all the ~200 occurrences of both "TRUE" and "FALSE" get > > >>> converted so the whole source tree is consistent. > > > > >> I would be in favor of that. > > > > > I have implemented this with the patch at: > > > http://momjian.us/expire/true_diff.txt > > > > Does this really do anything for us that will justify the extra > > back-patching pain it will cause? I don't see that it's improving > > code readability any. > > I think it is more of a consistency issue. There were multiple people > who wanted this change. Of course, some of those people don't backport > stuff. I guess you could argue this particular case without end, but I think we should be open to these kinds of changes. They will make future code easier to deal with and confuse new developers less (when to use which, do they mean different things, etc.). If back-patching really becomes a problem, we might want to look a little deeper into git options. I think the Linux kernel people do these kinds of cleanups more often, so there is probably some better support for it. -- 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] TRUE/FALSE vs true/false
On Tue, Aug 14, 2012 at 05:34:02PM -0400, Tom Lane wrote: > Bruce Momjian writes: > > On Thu, Aug 4, 2011 at 09:00:11PM +0300, Peter Eisentraut wrote: > >> On tor, 2011-08-04 at 14:44 +0200, Boszormenyi Zoltan wrote: > >>> I meant a mass "sed -e 's/TRUE/true/g' -e 's/FALSE/false/g'" run > >>> so all the ~200 occurrences of both "TRUE" and "FALSE" get > >>> converted so the whole source tree is consistent. > > >> I would be in favor of that. > > > I have implemented this with the patch at: > > http://momjian.us/expire/true_diff.txt > > Does this really do anything for us that will justify the extra > back-patching pain it will cause? I don't see that it's improving > code readability any. I think it is more of a consistency issue. There were multiple people who wanted this change. Of course, some of those people don't backport stuff. Other comments? -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] TRUE/FALSE vs true/false
Bruce Momjian writes: > On Thu, Aug 4, 2011 at 09:00:11PM +0300, Peter Eisentraut wrote: >> On tor, 2011-08-04 at 14:44 +0200, Boszormenyi Zoltan wrote: >>> I meant a mass "sed -e 's/TRUE/true/g' -e 's/FALSE/false/g'" run >>> so all the ~200 occurrences of both "TRUE" and "FALSE" get >>> converted so the whole source tree is consistent. >> I would be in favor of that. > I have implemented this with the patch at: > http://momjian.us/expire/true_diff.txt Does this really do anything for us that will justify the extra back-patching pain it will cause? I don't see that it's improving code readability any. 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] TRUE/FALSE vs true/false
On Thu, Aug 4, 2011 at 09:00:11PM +0300, Peter Eisentraut wrote: > On tor, 2011-08-04 at 14:44 +0200, Boszormenyi Zoltan wrote: > > 2011-08-04 14:32 keltezéssel, Robert Haas írta: > > > 2011/8/4 Boszormenyi Zoltan : > > >> Shouldn't these get fixed to be consistent? > > > I believe I already did. See commit > > > 85b436f7b1f06a6ffa8d2f29b03d6e440de18784. > > > > I meant a mass "sed -e 's/TRUE/true/g' -e 's/FALSE/false/g'" run > > so all the ~200 occurrences of both "TRUE" and "FALSE" get > > converted so the whole source tree is consistent. > > I would be in favor of that. I have implemented this with the patch at: http://momjian.us/expire/true_diff.txt I did not modify the Win32 code which traditionally uses uppercase for TRUE/FALSE. It would be applied only to HEAD. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] TRUE/FALSE vs true/false
On tor, 2011-08-04 at 14:44 +0200, Boszormenyi Zoltan wrote: > 2011-08-04 14:32 keltezéssel, Robert Haas írta: > > 2011/8/4 Boszormenyi Zoltan : > >> Shouldn't these get fixed to be consistent? > > I believe I already did. See commit > > 85b436f7b1f06a6ffa8d2f29b03d6e440de18784. > > I meant a mass "sed -e 's/TRUE/true/g' -e 's/FALSE/false/g'" run > so all the ~200 occurrences of both "TRUE" and "FALSE" get > converted so the whole source tree is consistent. I would be in favor of that. -- 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] TRUE/FALSE vs true/false
On 4 August 2011 13:57, Robert Haas wrote: > Oh, I see. Well, I don't care either way, so I'll let others weigh > in. The way it is doesn't bother me, but fixing it doesn't bother me > either. Idiomatic win32 code uses BOOL and TRUE/FALSE. They are macros defined somewhere or other. -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and 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] TRUE/FALSE vs true/false
On Thu, Aug 4, 2011 at 8:44 AM, Boszormenyi Zoltan wrote: > 2011-08-04 14:32 keltezéssel, Robert Haas írta: >> 2011/8/4 Boszormenyi Zoltan : >>> Shouldn't these get fixed to be consistent? >> I believe I already did. See commit >> 85b436f7b1f06a6ffa8d2f29b03d6e440de18784. > > I meant a mass "sed -e 's/TRUE/true/g' -e 's/FALSE/false/g'" run > so all the ~200 occurrences of both "TRUE" and "FALSE" get > converted so the whole source tree is consistent. Oh, I see. Well, I don't care either way, so I'll let others weigh in. The way it is doesn't bother me, but fixing it doesn't bother me either. -- Robert Haas 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] TRUE/FALSE vs true/false
2011-08-04 14:32 keltezéssel, Robert Haas írta: > 2011/8/4 Boszormenyi Zoltan : >> Shouldn't these get fixed to be consistent? > I believe I already did. See commit 85b436f7b1f06a6ffa8d2f29b03d6e440de18784. I meant a mass "sed -e 's/TRUE/true/g' -e 's/FALSE/false/g'" run so all the ~200 occurrences of both "TRUE" and "FALSE" get converted so the whole source tree is consistent. -- -- Zoltán Böszörményi Cybertec Schönig & Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/ -- 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] TRUE/FALSE vs true/false
2011/8/4 Boszormenyi Zoltan : > Shouldn't these get fixed to be consistent? I believe I already did. See commit 85b436f7b1f06a6ffa8d2f29b03d6e440de18784. -- Robert Haas 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
[HACKERS] TRUE/FALSE vs true/false
Hi, I looked at b4fbe392f8ff6ff1a66b488eb7197eef9e1770a4 and I noticed that it's using TRUE, FALSE, true and false inconsistently: @@ -248,6 +249,7 @@ CreateSharedInvalidationState(void) shmInvalBuffer->procState[i].nextMsgNum = 0;/* meaningless */ shmInvalBuffer->procState[i].resetState = false; shmInvalBuffer->procState[i].signaled = false; + shmInvalBuffer->procState[i].hasMessages = false; shmInvalBuffer->procState[i].nextLXID = InvalidLocalTransactionId; } } @@ -316,6 +316,7 @@ SharedInvalBackendInit(bool sendOnly) stateP->nextMsgNum = segP->maxMsgNum; stateP->resetState = false; stateP->signaled = false; + stateP->hasMessages = false; stateP->sendOnly = sendOnly; LWLockRelease(SInvalWriteLock); @@ -459,6 +461,19 @@ SIInsertDataEntries(const SharedInvalidationMessage *data, int n) SpinLockRelease(&vsegP->msgnumLock); } + /* +* Now that the maxMsgNum change is globally visible, we give +* everyone a swift kick to make sure they read the newly added +* messages. Releasing SInvalWriteLock will enforce a full memory +* barrier, so these (unlocked) changes will be committed to memory +* before we exit the function. +*/ + for (i = 0; i < segP->lastBackend; i++) + { + ProcState *stateP = &segP->procState[i]; + stateP->hasMessages = TRUE; + } + LWLockRelease(SInvalWriteLock); } } @@ -499,11 +514,36 @@ SIGetDataEntries(SharedInvalidationMessage *data, int datasize) ... +* Note that, if we don't end up reading all of the messages, we had +* better be certain to reset this flag before exiting! +*/ + stateP->hasMessages = FALSE; + @@ -544,10 +584,16 @@ SIGetDataEntries(SharedInvalidationMessage *data, int datasize) ... if (stateP->nextMsgNum >= max) stateP->signaled = false; + else + stateP->hasMessages = TRUE; Also, grepping for checking for or assigning bool values reveal that "true" and "false" are used far more than "TRUE" and "FALSE": [zozo@localhost backend]$ find . -name "*.c" | xargs grep -w true | grep -v 'true"' | grep = | wc -l 2446 [zozo@localhost backend]$ find . -name "*.c" | xargs grep -w false | grep -v 'false"' | grep = | wc -l 2745 [zozo@localhost backend]$ find . -name "*.c" | xargs grep -w TRUE | grep -v 'TRUE"' | grep = | wc -l 119 [zozo@localhost backend]$ find . -name "*.c" | xargs grep -w FALSE | grep -v 'FALSE"' | grep = | wc -l 140 Shouldn't these get fixed to be consistent? Best regards, Zoltán Böszörményi -- -- Zoltán Böszörményi Cybertec Schönig & Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers