pgsql: Refactor NUM_cache_remove calls in error report path to a PG_TRY
Log Message:
-----------
Refactor NUM_cache_remove calls in error report path to a PG_TRY block.
The code in the new block was not reindented; it will be fixed by pgindent
eventually.
Modified Files:
--------------
pgsql/src/backend/utils/adt:
formatting.c (r1.160 -> r1.161)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/adt/formatting.c?r1=1.160&r2=1.161)
On Mon, Aug 10, 2009 at 4:16 PM, Alvaro Herrera<alvherre@postgresql.org> wrote:
Log Message:
-----------
Refactor NUM_cache_remove calls in error report path to a PG_TRY block.The code in the new block was not reindented; it will be fixed by pgindent
eventually.
...breaking every patch that someone has in progress against that code?
...Robert
Robert Haas escribi�:
On Mon, Aug 10, 2009 at 4:16 PM, Alvaro Herrera<alvherre@postgresql.org> wrote:
Log Message:
-----------
Refactor NUM_cache_remove calls in error report path to a PG_TRY block.The code in the new block was not reindented; it will be fixed by pgindent
eventually....breaking every patch that someone has in progress against that code?
I guess I neglected to add "a year from now or so". I'm not in a hurry
to run pgindent.
--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.
On Mon, Aug 10, 2009 at 4:22 PM, Alvaro
Herrera<alvherre@commandprompt.com> wrote:
Robert Haas escribió:
On Mon, Aug 10, 2009 at 4:16 PM, Alvaro Herrera<alvherre@postgresql.org> wrote:
Log Message:
-----------
Refactor NUM_cache_remove calls in error report path to a PG_TRY block.The code in the new block was not reindented; it will be fixed by pgindent
eventually....breaking every patch that someone has in progress against that code?
I guess I neglected to add "a year from now or so". I'm not in a hurry
to run pgindent.
Me neither, but every place that we know pgindent will touch is like a
little land-mine waiting to go off under somebody's patch. It seems
like we ought to try to keep the tree as pgindent-clean as possible
when we make changes, so that there are as few of those land-mines out
there as possible.
...Robert
Alvaro Herrera <alvherre@commandprompt.com> writes:
Robert Haas escribi�:
The code in the new block was not reindented; it will be fixed by pgindent
eventually....breaking every patch that someone has in progress against that code?
I guess I neglected to add "a year from now or so". I'm not in a hurry
to run pgindent.
Yeah --- if there are any active patches against that code (a fact not
in evidence) then reindenting now would break them now. Leaving it till
the next pgindent run seems fine to me.
regards, tom lane
On Mon, Aug 10, 2009 at 4:31 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote:
Alvaro Herrera <alvherre@commandprompt.com> writes:
Robert Haas escribió:
The code in the new block was not reindented; it will be fixed by pgindent
eventually....breaking every patch that someone has in progress against that code?
I guess I neglected to add "a year from now or so". I'm not in a hurry
to run pgindent.Yeah --- if there are any active patches against that code (a fact not
in evidence) then reindenting now would break them now. Leaving it till
the next pgindent run seems fine to me.
But if there are patches against that code, then they've been broken
now and they will break again when the pgindent run is done. If the
indentation is fixed at commit-time (or before someone goes to the
trouble of fixing them), then they get broken only once. I guess it's
not the end of the world, but it sure seems like the less work
pgindent does when it is run, the better.
...Robert
Robert Haas escribi�:
But if there are patches against that code, then they've been broken
now and they will break again when the pgindent run is done. If the
indentation is fixed at commit-time (or before someone goes to the
trouble of fixing them), then they get broken only once. I guess it's
not the end of the world, but it sure seems like the less work
pgindent does when it is run, the better.
I think that we should be looking at making pgindent runnable by lone
hackers at home in their patched trees. That way they fix their patches
by simply running it when it's run on the CVS tree, before doing an
"update". That should remove/reduce the merge problems.
... at least in CVS; not sure what would happen if this was done in a
GIT repository. (It would probably require a rebase, but then what do I
know about GIT?).
BTW I think it's better to redirect this kind of discussion to -hackers.
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Robert Haas <robertmhaas@gmail.com> writes:
On Mon, Aug 10, 2009 at 4:31 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote:
Yeah --- if there are any active patches against that code (a fact not
in evidence) then reindenting now would break them now. �Leaving it till
the next pgindent run seems fine to me.
But if there are patches against that code, then they've been broken
now
Uh, no --- the point here is that something like two hundred lines of
code were *not* changed by reindenting. Diffs in that area would likely
still apply.
and they will break again when the pgindent run is done.
Only if they aren't applied by then. One reason that we normally only
run pgindent at the end of the devel cycle is that that's when
(presumably) the smallest amount of patches remain outstanding.
regards, tom lane
Robert Haas <robertmhaas@gmail.com> writes:
Me neither, but every place that we know pgindent will touch is like a
little land-mine waiting to go off under somebody's patch. It seems
like we ought to try to keep the tree as pgindent-clean as possible
when we make changes, so that there are as few of those land-mines out
there as possible.
You're ignoring the difference between two significantly different
scenarios. When code that was never in the tree gets added, yeah
it's probably good if it's reindented immediately. But in this
case we are talking about code that is already in the tree, and
has been for awhile, needing to be reindented because of some
changes that aren't textually all that close by. We will break
any outstanding patches against that code when we reindent it, but
it is not at all clear that it's better to do that now than
later. There are likely to be fewer patches outstanding at the
end of the devel cycle than there are now.
regards, tom lane
On Mon, Aug 10, 2009 at 6:52 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Mon, Aug 10, 2009 at 4:31 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote:
Yeah --- if there are any active patches against that code (a fact not
in evidence) then reindenting now would break them now. Leaving it till
the next pgindent run seems fine to me.But if there are patches against that code, then they've been broken
nowUh, no --- the point here is that something like two hundred lines of
code were *not* changed by reindenting. Diffs in that area would likely
still apply.and they will break again when the pgindent run is done.
Only if they aren't applied by then. One reason that we normally only
run pgindent at the end of the devel cycle is that that's when
(presumably) the smallest amount of patches remain outstanding.
OK, I get it. Thanks for bearing with me. The theory that the
smallest amount of patches remain outstanding at that point is
probably only true if the pgindent run is done relatively soon after
the last CommitFest. In the 8.4 cycle, the pgindent run was done
something like 7 months after the start of the last CommitFest, by
which time a fair number of patches had accumulated.
...Robert
Robert Haas <robertmhaas@gmail.com> writes:
On Mon, Aug 10, 2009 at 6:52 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote:
Only if they aren't applied by then. One reason that we normally only
run pgindent at the end of the devel cycle is that that's when
(presumably) the smallest amount of patches remain outstanding.
OK, I get it. Thanks for bearing with me. The theory that the
smallest amount of patches remain outstanding at that point is
probably only true if the pgindent run is done relatively soon after
the last CommitFest. In the 8.4 cycle, the pgindent run was done
something like 7 months after the start of the last CommitFest, by
which time a fair number of patches had accumulated.
Yeah, that's a fair point. Maybe we should institute a new policy that
pgindent should happen immediately after close of the last commitfest
in a cycle, instead of delaying until almost release time.
A more aggressive approach would be to run pgindent immediately after
the close of *each* commitfest, but that would tend to break patches
that had gotten punted to the next fest.
regards, tom lane
On Tue, Aug 11, 2009 at 11:56 AM, Tom Lane<tgl@sss.pgh.pa.us> wrote:
OK, I get it. Thanks for bearing with me. The theory that the
smallest amount of patches remain outstanding at that point is
probably only true if the pgindent run is done relatively soon after
the last CommitFest. In the 8.4 cycle, the pgindent run was done
something like 7 months after the start of the last CommitFest, by
which time a fair number of patches had accumulated.Yeah, that's a fair point. Maybe we should institute a new policy that
pgindent should happen immediately after close of the last commitfest
in a cycle, instead of delaying until almost release time.A more aggressive approach would be to run pgindent immediately after
the close of *each* commitfest, but that would tend to break patches
that had gotten punted to the next fest.
I'm not sure whether that would work out to a net positive or not.
Presumably, it would mostly only break patches that had touched that
part of the code, and therefore were likely to be broken anyway. But
by the same token, since they're under active development, they're
also more likely to have already been fixed. I'm not sure there's a
good solution to this problem short of making pgindent easy enough
that we can make it a requirement for patch submission, and I'm not
sure that's practical.
But in any case, I think running pgindent immediately after the last
CommitFest rather than after a longish delay would be a good idea.
...Robert
Robert Haas wrote:
I'm not sure there's a
good solution to this problem short of making pgindent easy enough
that we can make it a requirement for patch submission, and I'm not
sure that's practical.But in any case, I think running pgindent immediately after the last
CommitFest rather than after a longish delay would be a good idea.
Frankly, fixing up patch bitrot caused by pgindent is not terribly
difficult in my experience - bitrot caused by code drift is a much
harder problem (and yes, git fans, I know git can help with that).
cheers
andrew
On Tue, Aug 11, 2009 at 12:52 PM, Andrew Dunstan<andrew@dunslane.net> wrote:
Robert Haas wrote:
I'm not sure there's a
good solution to this problem short of making pgindent easy enough
that we can make it a requirement for patch submission, and I'm not
sure that's practical.But in any case, I think running pgindent immediately after the last
CommitFest rather than after a longish delay would be a good idea.Frankly, fixing up patch bitrot caused by pgindent is not terribly difficult
in my experience - bitrot caused by code drift is a much harder problem (and
yes, git fans, I know git can help with that).
Where it really bit me as when it reindented the DATA() statements
that were touched by ALTER TABLE ... SET STATISTICS DISTINCT. It's
not so hard to compare code, but comparing DATA() lines is the pits.
...Robert
Robert Haas wrote:
On Tue, Aug 11, 2009 at 12:52 PM, Andrew Dunstan<andrew@dunslane.net> wrote:
Robert Haas wrote:
I'm not sure there's a
good solution to this problem short of making pgindent easy enough
that we can make it a requirement for patch submission, and I'm not
sure that's practical.But in any case, I think running pgindent immediately after the last
CommitFest rather than after a longish delay would be a good idea.Frankly, fixing up patch bitrot caused by pgindent is not terribly difficult
in my experience - bitrot caused by code drift is a much harder problem (and
yes, git fans, I know git can help with that).Where it really bit me as when it reindented the DATA() statements
that were touched by ALTER TABLE ... SET STATISTICS DISTINCT. It's
not so hard to compare code, but comparing DATA() lines is the pits.
Oh? Maybe that's a problem we need to address more directly. I just
looked at what it did to the DATA lines - it seems to have changed 501
of them, and all the changes seem to be to do with tabbing.
cheers
andrew
Andrew Dunstan <andrew@dunslane.net> writes:
Robert Haas wrote:
Where it really bit me as when it reindented the DATA() statements
that were touched by ALTER TABLE ... SET STATISTICS DISTINCT. It's
not so hard to compare code, but comparing DATA() lines is the pits.
Oh? Maybe that's a problem we need to address more directly. I just
looked at what it did to the DATA lines - it seems to have changed 501
of them, and all the changes seem to be to do with tabbing.
That's interesting --- the whitespace in those macros has always been
wildly inconsistent, so I assumed pgindent wasn't touching them at all.
I wonder what it thinks it's doing...
regards, tom lane
Tom Lane wrote:
Andrew Dunstan <andrew@dunslane.net> writes:
Robert Haas wrote:
Where it really bit me as when it reindented the DATA() statements
that were touched by ALTER TABLE ... SET STATISTICS DISTINCT. It's
not so hard to compare code, but comparing DATA() lines is the pits.Oh? Maybe that's a problem we need to address more directly. I just
looked at what it did to the DATA lines - it seems to have changed 501
of them, and all the changes seem to be to do with tabbing.That's interesting --- the whitespace in those macros has always been
wildly inconsistent, so I assumed pgindent wasn't touching them at all.
I wonder what it thinks it's doing...
Here's the extract attached. I replace tabs with a literal '\t' so I
could see what it was doing. I can't make much head or tail of it either.
cheers
andrew
Attachments:
data-diffstext/plain; charset=iso-8859-1; name=data-diffsDownload+501-501
Andrew Dunstan escribi�:
Tom Lane wrote:
Andrew Dunstan <andrew@dunslane.net> writes:
Robert Haas wrote:
Where it really bit me as when it reindented the DATA() statements
that were touched by ALTER TABLE ... SET STATISTICS DISTINCT. It's
not so hard to compare code, but comparing DATA() lines is the pits.Oh? Maybe that's a problem we need to address more directly. I
just looked at what it did to the DATA lines - it seems to have
changed 501 of them, and all the changes seem to be to do with
tabbing.That's interesting --- the whitespace in those macros has always been
wildly inconsistent, so I assumed pgindent wasn't touching them at all.
I wonder what it thinks it's doing...Here's the extract attached. I replace tabs with a literal '\t' so
I could see what it was doing. I can't make much head or tail of it
either.
pgindent uses entab/detab, which counts spaces and replaces them with
tabs. It is wildly undocumented. See src/tools/entab
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> writes:
Andrew Dunstan escribi�:
Here's the extract attached. I replace tabs with a literal '\t' so
I could see what it was doing. I can't make much head or tail of it
either.
pgindent uses entab/detab, which counts spaces and replaces them with
tabs. It is wildly undocumented. See src/tools/entab
What surprises me is that it seems to be changing lines that have been
there for quite some time. Bruce, have you been changing pgindent
underneath us?
regards, tom lane
Tom Lane wrote:
Alvaro Herrera <alvherre@commandprompt.com> writes:
Andrew Dunstan escribi���:
Here's the extract attached. I replace tabs with a literal '\t' so
I could see what it was doing. I can't make much head or tail of it
either.pgindent uses entab/detab, which counts spaces and replaces them with
tabs. It is wildly undocumented. See src/tools/entabWhat surprises me is that it seems to be changing lines that have been
there for quite some time. Bruce, have you been changing pgindent
underneath us?
Not unless I am CVS committing without showing anyone. ;-)
These are the only commits I see recently, which are harmless:
revision 1.101
date: 2009/06/11 22:21:44; author: momjian; state: Exp; lines: +7 -2
Document struct/union problem with pgindent.
----------------------------
revision 1.100
date: 2008/11/03 15:56:47; author: momjian; state: Exp; lines: +2 -2
Small shell syntax improvement.
----------------------------
revision 1.99
date: 2008/04/16 21:03:08; author: momjian; state: Exp; lines: +2 -2
Ignore blank lines in typedef file.
----------------------------
revision 1.98
date: 2008/01/16 20:13:44; author: momjian; state: Exp; lines: +2 -2
Improve usage message for pgindent.
----------------------------
revision 1.97
date: 2007/12/21 14:20:36; author: momjian; state: Exp; lines: +11
-2008
Modify pgindent to use an external typedefs file rather than included
list.
and entab is similarly unchanged:
revision 1.18
date: 2007/02/08 11:10:27; author: petere; state: Exp; lines: +2 -2
Normalize fgets() calls to use sizeof() for calculating the buffer size
where possible, and fix some sites that apparently thought that fgets()
will overwrite the buffer by one byte.
Also add some strlcpy() to eliminate some weird memory handling.
I am stumped, but can keep researching.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +