Re: [HACKERS] Large C files

2011-10-19 Thread Alvaro Herrera

Excerpts from David Fetter's message of lun oct 17 03:00:19 -0300 2011:
 On Fri, Oct 14, 2011 at 07:36:32PM +0100, Peter Geoghegan wrote:
  This evening, David Fetter described a problem to me that he was
  having building the Twitter FDW. It transpired that it was down to its
  dependence on an #include that was recently judged to be
  redundant.This seems like another reason to avoid using pgrminclude -
  even if we can be certain that the #includes are redundant within
  Postgres, we cannot be sure that they are redundant in third party
  code.
 
 Perhaps something that tested some third-party code
 automatically...say, doesn't the new buildfarm stuff allow running
 some arbitrary code?

I think you could run your own buildfarm member and add whatever steps
you wanted (we have one building JDBC).  I'm not sure that we want that
though -- it'd start polluting the greenness of our buildfarm with
failures from code outside of our control.

I mean, if the third party code fails to compile, surely it's the third
party devs that care.

I fail to see why this is such a big deal.

-- 
Álvaro Herrera alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

-- 
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] Large C files

2011-10-17 Thread David Fetter
On Fri, Oct 14, 2011 at 07:36:32PM +0100, Peter Geoghegan wrote:
 This evening, David Fetter described a problem to me that he was
 having building the Twitter FDW. It transpired that it was down to its
 dependence on an #include that was recently judged to be
 redundant.This seems like another reason to avoid using pgrminclude -
 even if we can be certain that the #includes are redundant within
 Postgres, we cannot be sure that they are redundant in third party
 code.

Perhaps something that tested some third-party code
automatically...say, doesn't the new buildfarm stuff allow running
some arbitrary code?

Cheers,
David.
-- 
David Fetter da...@fetter.org http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david.fet...@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

-- 
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] Large C files

2011-10-14 Thread Peter Geoghegan
This evening, David Fetter described a problem to me that he was
having building the Twitter FDW. It transpired that it was down to its
dependence on an #include that was recently judged to be
redundant.This seems like another reason to avoid using pgrminclude -
even if we can be certain that the #includes are redundant within
Postgres, we cannot be sure that they are redundant in third party
code.

-- 
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] Large C files

2011-10-14 Thread Alvaro Herrera

Excerpts from Peter Geoghegan's message of vie oct 14 15:36:32 -0300 2011:
 This evening, David Fetter described a problem to me that he was
 having building the Twitter FDW. It transpired that it was down to its
 dependence on an #include that was recently judged to be
 redundant.This seems like another reason to avoid using pgrminclude -
 even if we can be certain that the #includes are redundant within
 Postgres, we cannot be sure that they are redundant in third party
 code.

I'm not sure about this.  I mean, surely the problem was easily detected
and fixed because the compile fails outright, yes?

I do take time on ocassion to do some header reorganization and remove
inclusions that are redundant or unnecessary.  I don't want that work
thrown aside because we have to support some hypothetical third party
module that would fail to compile.  (In fact, we purposedly removed some
header from spi.h because it was unnecessary).

-- 
Álvaro Herrera alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

-- 
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] Large C files

2011-09-24 Thread Hannu Krosing
On Wed, 2011-09-07 at 01:22 +0200, Jan Urbański wrote:
 On 07/09/11 01:13, Peter Geoghegan wrote:
  On 6 September 2011 08:29, Peter Eisentraut pete...@gmx.net wrote:
  I was thinking about splitting up plpython.c, but it's not even on that
  list. ;-)
  
  IIRC the obesity of that file is something that Jan Urbański intends
  to fix, or is at least concerned about. Perhaps you should compare
  notes.
 
 Yeah, plpython.c could easily be splitted into submodules for builtin
 functions, spi support, subtransactions support, traceback support etc.
 
 If there is consensus that this should be done, I'll see if I can find
 time to submit a giant-diff-no-behaviour-changes patch for the next CF.

+1 from me.

Would make working with it much nicer.

 Cheers,
 Jan
 

-- 
---
Hannu Krosing
PostgreSQL Unlimited Scalability and Performance Consultant
2ndQuadrant Nordic
PG Admin Book: http://www.2ndQuadrant.com/books/


-- 
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] Large C files

2011-09-24 Thread Bruce Momjian
Robert Haas wrote:
 On Fri, Sep 9, 2011 at 11:28 PM, Peter Geoghegan pe...@2ndquadrant.com 
 wrote:
  It's very difficult or impossible to anticipate how effective the tool
  will be in practice, but when you consider that it works and does not
  produce false positives for the first 3 real-world cases tested, it
  seems reasonable to assume that it's at least worth having around. Tom
  recently said of a previous pgrminclude campaign in July 2006 that It
  took us two weeks to mostly recover, but we were still dealing with
  some fallout in December. I think that makes the case for adding this
  tool or some refinement as a complement to pgrminclude in src/tools
  fairly compelling.
 
 I'm not opposed to adding something like this, but I think it needs to
 either be tied into the actual running of the script, or have a lot
 more documentation than it does now, or both.  I am possibly stupid,
 but I can't understand from reading the script (or, honestly, the
 thread) exactly what kind of pgrminclude-induced errors this is
 protecting against; but even if we clarify that, it seems like it
 would be a lot of work to run it manually on all the files that might
 be affected by a pgrminclude run, unless we can somehow automate that.
 
 I'm also curious to see how much more fallout we're going to see from
 that run.  We had a few glitches when it was first done, but it didn't
 seem like they were really all that bad.  It might be that we'd be
 better off running pgrminclude a lot *more* often (like once a cycle,
 or even after every CommitFest), because the scope of the changes
 would then be far smaller and we wouldn't be dealing with 5 years of
 accumulated cruft all at once; we'd also get a lot more experience
 with what works or does not work with the script, which might lead to
 improvements in that script on a less-than-geologic time scale.

Interesting idea.  pgrminclude has three main problems:

o  defines --- to prevent removing an include that is referenced in an
#ifdef block that is not reached, it removes ifdef blocks, though that
might cause the file not to compile and be skipped.

o  ## expansion --- we found that CppAsString2() uses the CCP expansion
##, which throws no error if the symbol is does not exist (perhaps
through the removal of a define).  This is the problem we had with
tablespace directory names, and the script now checks for CppAsString2
and friends and skips the file.

o  imbalance of includes --- pgrminclude can remove includes that are
not required, but this can cause other files to need many more includes.
This is the imbalance include problem Tom found.

The submitted patch to compare object files only catches the second
problem we had.  

I think we determined that the best risk/reward is to run it every five
years.  The pgrminclude run removed 627 include references, which is
0.006% of our 1,077,878 lines of C code.

-- 
  Bruce Momjian  br...@momjian.ushttp://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] Large C files

2011-09-24 Thread Tom Lane
Bruce Momjian br...@momjian.us writes:
 Robert Haas wrote:
 I'm also curious to see how much more fallout we're going to see from
 that run.  We had a few glitches when it was first done, but it didn't
 seem like they were really all that bad.  It might be that we'd be
 better off running pgrminclude a lot *more* often (like once a cycle,
 or even after every CommitFest), because the scope of the changes
 would then be far smaller and we wouldn't be dealing with 5 years of
 accumulated cruft all at once; we'd also get a lot more experience
 with what works or does not work with the script, which might lead to
 improvements in that script on a less-than-geologic time scale.

 Interesting idea.  pgrminclude has three main problems: [ snipped ]

Actually, I believe that the *main* problem with pgrminclude is that
it fails to account for combinations of build options other than those
that Bruce uses.  In the previous go-round, the reason we were still
squashing bugs months later is that it took that long for people to
notice and complain hey, compiling with LOCK_DEBUG no longer works,
or various other odd build options that the buildfarm doesn't exercise.
I have 100% faith that we'll be squashing some bugs like that ... very
possibly, the exact same ones as five years ago ... over the next few
months.  Peter's proposed tool would catch issues like the CppAsString2
failure, but it's still only going to exercise the build options you're
testing with.

If we could get pgrminclude up to a similar level of reliability as
pgindent, I'd be for running it on every cycle.  But I'm not sure that
the current approach to it is capable even in theory of getting to it
just works reliability.  I'm also not impressed at all by the hack of
avoiding problems by excluding entire files from the processing ---
what's the point of having the tool then?

 I think we determined that the best risk/reward is to run it every five
 years.

Frankly, with the tool in its current state I'd rather not run it at
all, ever.  The value per man-hour expended is too low.  The mess it
made out of the xlog-related includes this time around makes me question
whether it's even a net benefit, regardless of whether it can be
guaranteed not to break things.  Fundamentally, there's a large
component of design judgment/taste in the question of which header files
should include which others, but this tool does not have any taste.

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] Large C files

2011-09-24 Thread Bruce Momjian
Tom Lane wrote:
 Actually, I believe that the *main* problem with pgrminclude is that
 it fails to account for combinations of build options other than those
 that Bruce uses.  In the previous go-round, the reason we were still
 squashing bugs months later is that it took that long for people to
 notice and complain hey, compiling with LOCK_DEBUG no longer works,
 or various other odd build options that the buildfarm doesn't exercise.
 I have 100% faith that we'll be squashing some bugs like that ... very
 possibly, the exact same ones as five years ago ... over the next few
 months.  Peter's proposed tool would catch issues like the CppAsString2

The new code removes #ifdef markers so all code is compiled, or the file
is skipped if it can't be compiled.  That should avoid this problem.

 failure, but it's still only going to exercise the build options you're
 testing with.
 
 If we could get pgrminclude up to a similar level of reliability as
 pgindent, I'd be for running it on every cycle.  But I'm not sure that
 the current approach to it is capable even in theory of getting to it
 just works reliability.  I'm also not impressed at all by the hack of
 avoiding problems by excluding entire files from the processing ---
 what's the point of having the tool then?

We remove the includes we safely can, and skip the others --- the
benefit is only for the files we can process.

-- 
  Bruce Momjian  br...@momjian.ushttp://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] Large C files

2011-09-24 Thread Tom Lane
Bruce Momjian br...@momjian.us writes:
 Tom Lane wrote:
 Actually, I believe that the *main* problem with pgrminclude is that
 it fails to account for combinations of build options other than those
 that Bruce uses.  In the previous go-round, the reason we were still
 squashing bugs months later is that it took that long for people to
 notice and complain hey, compiling with LOCK_DEBUG no longer works,
 or various other odd build options that the buildfarm doesn't exercise.
 I have 100% faith that we'll be squashing some bugs like that ... very
 possibly, the exact same ones as five years ago ... over the next few
 months.  Peter's proposed tool would catch issues like the CppAsString2

 The new code removes #ifdef markers so all code is compiled, or the file
 is skipped if it can't be compiled.  That should avoid this problem.

It avoids it at a very large cost, namely skipping all the files where
it's not possible to compile each arm of every #if on the machine being
used.  I do not think that's a solution, just a band-aid; for instance,
won't it prevent include optimization in every file that contains even
one #ifdef WIN32?  Or what about files in which there are #if blocks
that each define the same function, constant table, etc?

The right solution would involve testing each #if block under the
conditions in which it was *meant* to be compiled.

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] Large C files

2011-09-24 Thread Peter Geoghegan
On 24 September 2011 16:41, Tom Lane t...@sss.pgh.pa.us wrote:
 Frankly, with the tool in its current state I'd rather not run it at
 all, ever.  The value per man-hour expended is too low.  The mess it
 made out of the xlog-related includes this time around makes me question
 whether it's even a net benefit, regardless of whether it can be
 guaranteed not to break things.  Fundamentally, there's a large
 component of design judgment/taste in the question of which header files
 should include which others, but this tool does not have any taste.

I agree. If this worked well in a semi-automated fashion, there'd be
some other open source tool already available for us to use. As far as
I know, there isn't. As we work around pgrminclude's bugs, its
benefits become increasingly small and hard to quantify.

If we're not going to use it, it should be removed from the tree.

-- 
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] Large C files

2011-09-24 Thread Andrew Dunstan



On 09/24/2011 01:10 PM, Peter Geoghegan wrote:

On 24 September 2011 16:41, Tom Lanet...@sss.pgh.pa.us  wrote:

Frankly, with the tool in its current state I'd rather not run it at
all, ever.  The value per man-hour expended is too low.  The mess it
made out of the xlog-related includes this time around makes me question
whether it's even a net benefit, regardless of whether it can be
guaranteed not to break things.  Fundamentally, there's a large
component of design judgment/taste in the question of which header files
should include which others, but this tool does not have any taste.

I agree. If this worked well in a semi-automated fashion, there'd be
some other open source tool already available for us to use. As far as
I know, there isn't. As we work around pgrminclude's bugs, its
benefits become increasingly small and hard to quantify.

If we're not going to use it, it should be removed from the tree.



Yeah, I've always been dubious about the actual benefit.  At best this 
can be seen as identifying some candidates for pruning, but as an 
automated tool I'm inclined to write it off as a failed experiment.


cheers

andrew

--
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] Large C files

2011-09-24 Thread Bruce Momjian
Tom Lane wrote:
 Bruce Momjian br...@momjian.us writes:
  Tom Lane wrote:
  Actually, I believe that the *main* problem with pgrminclude is that
  it fails to account for combinations of build options other than those
  that Bruce uses.  In the previous go-round, the reason we were still
  squashing bugs months later is that it took that long for people to
  notice and complain hey, compiling with LOCK_DEBUG no longer works,
  or various other odd build options that the buildfarm doesn't exercise.
  I have 100% faith that we'll be squashing some bugs like that ... very
  possibly, the exact same ones as five years ago ... over the next few
  months.  Peter's proposed tool would catch issues like the CppAsString2
 
  The new code removes #ifdef markers so all code is compiled, or the file
  is skipped if it can't be compiled.  That should avoid this problem.
 
 It avoids it at a very large cost, namely skipping all the files where
 it's not possible to compile each arm of every #if on the machine being
 used.  I do not think that's a solution, just a band-aid; for instance,
 won't it prevent include optimization in every file that contains even
 one #ifdef WIN32?  Or what about files in which there are #if blocks
 that each define the same function, constant table, etc?
 
 The right solution would involve testing each #if block under the
 conditions in which it was *meant* to be compiled.

Right.  It is under the better than nothing category, which is better
than nothing (not running it).  ;-)

-- 
  Bruce Momjian  br...@momjian.ushttp://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] Large C files

2011-09-23 Thread Robert Haas
On Fri, Sep 9, 2011 at 11:28 PM, Peter Geoghegan pe...@2ndquadrant.com wrote:
 It's very difficult or impossible to anticipate how effective the tool
 will be in practice, but when you consider that it works and does not
 produce false positives for the first 3 real-world cases tested, it
 seems reasonable to assume that it's at least worth having around. Tom
 recently said of a previous pgrminclude campaign in July 2006 that It
 took us two weeks to mostly recover, but we were still dealing with
 some fallout in December. I think that makes the case for adding this
 tool or some refinement as a complement to pgrminclude in src/tools
 fairly compelling.

I'm not opposed to adding something like this, but I think it needs to
either be tied into the actual running of the script, or have a lot
more documentation than it does now, or both.  I am possibly stupid,
but I can't understand from reading the script (or, honestly, the
thread) exactly what kind of pgrminclude-induced errors this is
protecting against; but even if we clarify that, it seems like it
would be a lot of work to run it manually on all the files that might
be affected by a pgrminclude run, unless we can somehow automate that.

I'm also curious to see how much more fallout we're going to see from
that run.  We had a few glitches when it was first done, but it didn't
seem like they were really all that bad.  It might be that we'd be
better off running pgrminclude a lot *more* often (like once a cycle,
or even after every CommitFest), because the scope of the changes
would then be far smaller and we wouldn't be dealing with 5 years of
accumulated cruft all at once; we'd also get a lot more experience
with what works or does not work with the script, which might lead to
improvements in that script on a less-than-geologic time scale.

-- 
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] Large C files

2011-09-23 Thread Peter Geoghegan
On 23 September 2011 15:46, Robert Haas robertmh...@gmail.com wrote:
 I'm not opposed to adding something like this, but I think it needs to
 either be tied into the actual running of the script, or have a lot
 more documentation than it does now, or both.  I am possibly stupid,
 but I can't understand from reading the script (or, honestly, the
 thread) exactly what kind of pgrminclude-induced errors this is
 protecting against;

The basic idea is simple. There is a high likelihood that if removing
a header alters the behaviour of an object file silently, that it will
also alter the symbol table for the same object file - in particular,
the value of function symbols (their local offset in the object
file), which relates to the number of instructions in each function.
Yes, that is imperfect, but it's better than nothing, and intuitively
I think that the only things that it won't catch in practice are
completely inorganic bugs (i.e. things done for the express purpose of
proving that it can be broken). Thinking about it now though, I have
to wonder if I could have done a better job with objdump -td.

 but even if we clarify that, it seems like it
 would be a lot of work to run it manually on all the files that might
 be affected by a pgrminclude run, unless we can somehow automate that.

I would have automated it if anyone had expressed any interest in the
basic idea - it might be an over-reaction to the problem. I'm not
sure. I'd have had it detect which object files might have been
affected (through directly including the header, or indirectly
including it by proxy). It could rename them such that, for example,
xlog.o was renamed to xlog_old.o . Then, you make your changes,
rebuild, and run the program again in a different mode. It notices the
*_old.o files, and runs nm-diff on them.

 I'm also curious to see how much more fallout we're going to see from
 that run.  We had a few glitches when it was first done, but it didn't
 seem like they were really all that bad.  It might be that we'd be
 better off running pgrminclude a lot *more* often (like once a cycle,
 or even after every CommitFest), because the scope of the changes
 would then be far smaller and we wouldn't be dealing with 5 years of
 accumulated cruft all at once; we'd also get a lot more experience
 with what works or does not work with the script, which might lead to
 improvements in that script on a less-than-geologic time scale.

Fair point. I'm a little busy with other things right now, but I'll
revisit it soon.

-- 
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] Large C files

2011-09-09 Thread Greg Stark
On Fri, Sep 9, 2011 at 6:57 AM, Heikki Linnakangas
heikki.linnakan...@enterprisedb.com wrote:
 In particular, I'd like to know what
 boundaries it is envisaged that the code should be refactored along to
 increase its conceptual integrity, or to better separate concerns. I
 assume that that's the idea, since each new .c file would have to have
 a discrete purpose.

 I'd like to see it split into routines involved in writing WAL, and those
 involved in recovery. And maybe a third file for archiving-related stuff.

Having a clean API for working with WAL independently of recovery
would let us have a maintainable xlogdump tool that doesn't constantly
get out of sync with the wal archive format. It would also make it
easier to write things like prefretching logic that requires reading
the upcoming xlog before its time to actually replay it.


-- 
greg

-- 
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] Large C files

2011-09-09 Thread Peter Geoghegan
I've produced the refinement of my little shell script anticipated by
my last e-mail (using sed to remove irrelevant variations in
__func__.12345 type symbol names). I decided to test it for:

1. Detecting behavioural changes when removing existing pgrminclude
ignore files (Basically headers that won't break the build if
removed, but will break Postgres). I stuck with .c files here for the
sake of convenience.

2. False positives by including a bunch of random headers.

Fist file tested was pl_comp.c. The script detected the change in
behaviour due to the omission of that header (incidentally, the way
the header is used there strikes me as a bit ugly). The script did not
produce false positives with the addition of these headers:

#include commands/portalcmds.h
#include commands/conversioncmds.h
#include commands/defrem.h
#include commands/proclang.h
#include commands/tablecmds.h
#include commands/tablespace.h
#include commands/typecmds.h
#include commands/collationcmds.h
#include commands/prepare.h
#include commands/explain.h
#include commands/comment.h
#include commands/cluster.h
#include commands/discard.h
#include commands/trigger.h
#include commands/user.h
#include commands/view.h
#include commands/sequence.h
#include commands/dbcommands.h
#include commands/async.h
#include commands/lockcmds.h
#include commands/vacuum.h

The second file tested was regerror.c . Similarly, the omission was
detected, and the addition of the same headers did not produce a false
positive.

It's very difficult or impossible to anticipate how effective the tool
will be in practice, but when you consider that it works and does not
produce false positives for the first 3 real-world cases tested, it
seems reasonable to assume that it's at least worth having around. Tom
recently said of a previous pgrminclude campaign in July 2006 that It
took us two weeks to mostly recover, but we were still dealing with
some fallout in December. I think that makes the case for adding this
tool or some refinement as a complement to pgrminclude in src/tools
fairly compelling.

-- 
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services


nm-diff.sh
Description: Bourne shell script

-- 
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] Large C files

2011-09-08 Thread Simon Riggs
On Wed, Sep 7, 2011 at 9:02 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Simon Riggs si...@2ndquadrant.com writes:
 Please lets not waste effort on refactoring efforts in mid dev cycle.

 Say what?  When else would you have us do it?

When else would you have us develop?

Major changes happen at start of a dev cycle, after some discussion.
Not in the middle and especially not for low priority items. It's not
even a bug, just code beautification.

We've all accepted the need for some change, but I would like to see
it happen slowly and carefully because of the very high risk of
introducing bugs into stable code that doesn't have a formal
verification mechanism currently. Anybody that could be trusted to
make those changes ought to have something better to do. If they don't
then that is in itself a much more serious issue than this.

-- 
 Simon Riggs   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] Large C files

2011-09-08 Thread Tom Lane
Simon Riggs si...@2ndquadrant.com writes:
 On Wed, Sep 7, 2011 at 9:02 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Simon Riggs si...@2ndquadrant.com writes:
 Please lets not waste effort on refactoring efforts in mid dev cycle.

 Say what?  When else would you have us do it?

 When else would you have us develop?

In my eyes that sort of activity *is* development.  I find the
distinction you are drawing entirely artificial, and more calculated to
make sure refactoring never happens than to add any safety.  Any
significant development change carries a risk of breakage.

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] Large C files

2011-09-08 Thread Bruce Momjian
Tom Lane wrote:
 Simon Riggs si...@2ndquadrant.com writes:
  On Wed, Sep 7, 2011 at 9:02 PM, Tom Lane t...@sss.pgh.pa.us wrote:
  Simon Riggs si...@2ndquadrant.com writes:
  Please lets not waste effort on refactoring efforts in mid dev cycle.
 
  Say what? ?When else would you have us do it?
 
  When else would you have us develop?
 
 In my eyes that sort of activity *is* development.  I find the
 distinction you are drawing entirely artificial, and more calculated to
 make sure refactoring never happens than to add any safety.  Any
 significant development change carries a risk of breakage.

I ran pgrminclude a week ago and that is certainly a larger change than
this.  Early in the development cycle people are merging in their saved
patches, so now is a fine time to do refactoring.

-- 
  Bruce Momjian  br...@momjian.ushttp://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] Large C files

2011-09-08 Thread Robert Haas
On Thu, Sep 8, 2011 at 10:29 AM, Bruce Momjian br...@momjian.us wrote:
 Tom Lane wrote:
 Simon Riggs si...@2ndquadrant.com writes:
  On Wed, Sep 7, 2011 at 9:02 PM, Tom Lane t...@sss.pgh.pa.us wrote:
  Simon Riggs si...@2ndquadrant.com writes:
  Please lets not waste effort on refactoring efforts in mid dev cycle.

  Say what?  When else would you have us do it?

  When else would you have us develop?

 In my eyes that sort of activity *is* development.  I find the
 distinction you are drawing entirely artificial, and more calculated to
 make sure refactoring never happens than to add any safety.  Any
 significant development change carries a risk of breakage.

 I ran pgrminclude a week ago and that is certainly a larger change than
 this.  Early in the development cycle people are merging in their saved
 patches, so now is a fine time to do refactoring.

+1.

I'd feel more comfortable refactoring it if we had some automated
testing of those code paths, but I don't see anything wrong with doing
it now from a timing perspective.  We still have 4 months until the
start of the final CommitFest.  I wouldn't be too enthusiastic about
starting a project like this in January, but now seems fine.  A bigger
problem is that I don't hear anyone volunteering to do the work.

-- 
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] Large C files

2011-09-08 Thread Simon Riggs
On Thu, Sep 8, 2011 at 3:25 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Simon Riggs si...@2ndquadrant.com writes:
 On Wed, Sep 7, 2011 at 9:02 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Simon Riggs si...@2ndquadrant.com writes:
 Please lets not waste effort on refactoring efforts in mid dev cycle.

 Say what?  When else would you have us do it?

 When else would you have us develop?

 In my eyes that sort of activity *is* development.  I find the
 distinction you are drawing entirely artificial, and more calculated to
 make sure refactoring never happens than to add any safety.  Any
 significant development change carries a risk of breakage.

You clearly have the bit between your teeth on this.

That doesn't make it worthwhile or sensible though.

I've offered to do it slowly and carefully over time, but that seems
not enough for some reason. What is the real reason for this?

I assume whoever does it will be spending significant time on testing
and bug fixing afterwards. I foresee lots of while I'm there, I
thought I'd just mend X, so we'll spend lots of time fighting to keep
functionality that's already there. Look at the discussion around
archive_command for an example of that.

-- 
 Simon Riggs   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] Large C files

2011-09-08 Thread Bruce Momjian
Simon Riggs wrote:
 On Thu, Sep 8, 2011 at 3:25 PM, Tom Lane t...@sss.pgh.pa.us wrote:
  Simon Riggs si...@2ndquadrant.com writes:
  On Wed, Sep 7, 2011 at 9:02 PM, Tom Lane t...@sss.pgh.pa.us wrote:
  Simon Riggs si...@2ndquadrant.com writes:
  Please lets not waste effort on refactoring efforts in mid dev cycle.
 
  Say what? ?When else would you have us do it?
 
  When else would you have us develop?
 
  In my eyes that sort of activity *is* development. ?I find the
  distinction you are drawing entirely artificial, and more calculated to
  make sure refactoring never happens than to add any safety. ?Any
  significant development change carries a risk of breakage.
 
 You clearly have the bit between your teeth on this.

So I guess Tom, Robert Haas, and I all have bits then.

 That doesn't make it worthwhile or sensible though.

We are not saying do it now.  We are disputing your suggestion that now
is not a good time --- there is a difference.

 I've offered to do it slowly and carefully over time, but that seems
 not enough for some reason. What is the real reason for this?

Oh, if we told you, then, well, you would know, and the big secret would
be out.  (Hint:  when three people are telling you the same thing, odds
are there is not some secret motive.)

-- 
  Bruce Momjian  br...@momjian.ushttp://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] Large C files

2011-09-08 Thread Tom Lane
Simon Riggs si...@2ndquadrant.com writes:
 You clearly have the bit between your teeth on this.

Personally, I'm neither intending to break up xlog.c right now, nor
asking you to do it.  I'm just objecting to your claim that there
should be some project-policy restriction on when refactoring gets done.
I do have other refactoring-ish plans in mind, eg
http://archives.postgresql.org/message-id/9232.1315441...@sss.pgh.pa.us
and am not going to be happy if you claim I can't do that now.

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] Large C files

2011-09-08 Thread Peter Geoghegan
On 8 September 2011 15:43, Robert Haas robertmh...@gmail.com wrote:
 I wouldn't be too enthusiastic about
 starting a project like this in January, but now seems fine.  A bigger
 problem is that I don't hear anyone volunteering to do the work.

You seem to have a fairly strong opinion on the xlog.c code. It would
be useful to hear any preliminary thoughts that you might have on what
we'd end up with when this refactoring work is finished. If I'm not
mistaken, you think that it is a good candidate for being refactored
not so much because of its size, but for other reasons -  could you
please elaborate on those? In particular, I'd like to know what
boundaries it is envisaged that the code should be refactored along to
increase its conceptual integrity, or to better separate concerns. I
assume that that's the idea, since each new .c file would have to have
a discrete purpose.

On 7 September 2011 19:12, Tom Lane t...@sss.pgh.pa.us wrote:
 The pgrminclude-induced bug you just fixed shows a concrete way in which
 moving code from one file to another might silently break it, ie, it
 still compiles despite lack of definition of some symbol it's intended
 to see.

Of course, the big unknown here is the C preprocessor. There is
nothing in principle that stops a header file from slightly or utterly
altering the semantics of any file it is included in. That said, it
wouldn't surprise me if the nm-diff shell script I proposed (or a
slight variant intended to catch pgrminclude type bugs) caught many of
those problems in practice.

Attached is simple POC nm-diff.sh, intended to detect
pgrminclude-induced bugs (split c file variant may follow if there is
interest). I tested this on object files compiled before and after the
bug fix to catalog.h in this commit:

http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=f81fb4f690355bc88fee69624103956fb4576fe5

or rather, I tested things after that commit with and without the
supposedly unneeded header; I removed catalog version differences,
applying Ockham's razor, so there was exactly one difference. That
didn't change anything; I just saw the same thing as before, which
was:

[peter@localhost commands]$ nm-diff.sh tablespace.o tablespace.old.o
Symbol table differences:
50c50
 05e7 r __func__.15690
---
 05ef r __func__.15690
WARNING: Symbol tables don't match!


I decided to add a bunch of obviously superfluous headers (a bunch of
xlog stuff) to the same file to verify that they *didn't* change
anything, figuring that it was very unlikely that adding these headers
could *really* change something. However, they did, but I don't think
that that proves that the script is fundamentally flawed (they didn't
*really* change anything):

[peter@localhost commands]$ nm-diff.sh tablespace.o tablespace.old.o
Symbol table differences:
41,50c41,50
 06f0 r __func__.15719 -- symbol name differs, offset does not
 06d0 r __func__.15730
 06be r __func__.15750
 06a0 r __func__.15759
 0680 r __func__.15771
 0660 r __func__.15793
 0640 r __func__.15803
 0620 r __func__.15825
 0600 r __func__.15886
 05e7 r __func__.15903  -- value/local offset the same as before
---
 06f0 r __func__.15506
 06d0 r __func__.15517
 06be r __func__.15537
 06a0 r __func__.15546
 0680 r __func__.15558
 0660 r __func__.15580
 0640 r __func__.15590
 0620 r __func__.15612
 0600 r __func__.15673
 05ef r __func__.15690  -- Also, value/local offset the same as 
 before (same inconsistency too)
WARNING: Symbol tables don't match!

My analysis is that the fact that the arbitrary symbol names assigned
within this read-only data section differ likely has no significance
(it looks like the compiler assigns them at an early optimisation
stage like Postgres assigns sequence values, before some are
eliminated at a later stage - I read ELF docs, but couldn't confirm
this, but maybe should have read GCC docs), so the next revision of
this script should simply ignore them using a regex if they look like
this; only the internal offsets/values matter, and they have been
observed to differ based on whether or not the header is included per
Bruce's revision (importantly, the difference in offset/values of the
last __func__ remains just the same regardless of whether the
superfluous xlog headers are included).

Does that seem reasonable? Is there interest in developing this idea further?

-- 
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services


nm-diff.sh
Description: Bourne shell script

-- 
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] Large C files

2011-09-08 Thread Josh Berkus
Simon, Robert, Bruce, Tom,

  Say what?  When else would you have us do it?
 
  When else would you have us develop?
 
  In my eyes that sort of activity *is* development.  I find the
  distinction you are drawing entirely artificial, and more calculated to
  make sure refactoring never happens than to add any safety.  Any
  significant development change carries a risk of breakage.
 You clearly have the bit between your teeth on this.
 
 That doesn't make it worthwhile or sensible though.

The above really seems like a non-argument over trivial wording of
stuff.  Can we please give each other the benefit of the doubt and not
read objectionable content into offhand comments?

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

-- 
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] Large C files

2011-09-08 Thread Robert Haas
On Thu, Sep 8, 2011 at 4:45 PM, Peter Geoghegan pe...@2ndquadrant.com wrote:
 On 8 September 2011 15:43, Robert Haas robertmh...@gmail.com wrote:
 I wouldn't be too enthusiastic about
 starting a project like this in January, but now seems fine.  A bigger
 problem is that I don't hear anyone volunteering to do the work.

 You seem to have a fairly strong opinion on the xlog.c code. It would
 be useful to hear any preliminary thoughts that you might have on what
 we'd end up with when this refactoring work is finished. If I'm not
 mistaken, you think that it is a good candidate for being refactored
 not so much because of its size, but for other reasons -  could you
 please elaborate on those? In particular, I'd like to know what
 boundaries it is envisaged that the code should be refactored along to
 increase its conceptual integrity, or to better separate concerns. I
 assume that that's the idea, since each new .c file would have to have
 a discrete purpose.

I'm not sure how strong my opinions are, but one thing that I think
could be improved is that StartupXLOG() is really, really long, and it
does a lot of different stuff:

- Read the control file and sanity check it.
- Read the backup label if there is one or otherwise inspect the
control file's checkpoint information.
- Set up for REDO (including Hot Standby) if required.
- Main REDO loop.
- Check whether we reached consistency and/or whether we need a new TLI.
- Prepare to write WAL.
- Post-recovery cleanup.
- Initialize for normal running.

It seems to me that we could improve the readability of that function
by separating out some of the larger chunks of functionality into
their own static functions.  That wouldn't make xlog.c any shorter,
but it would make that function easier to understand.

Another gripe I have is that recoveryStopsHere() has non-obvious side
effects.  Not sure what to do about that, exactly, but I found it
extremely surprising when first picking my way through this code.

One pretty easy thing to pull out of this file wold be all the
user-callable functions.  pg_xlog_recovery_replay_{pause,resume},
pg_is_xlog_recovery_paused, pg_last_xact_replay_timestamp,
pg_is_in_recovery, pg_{start,stop}_backup(), pg_switch_xlog(),
pg_create_restore_point(), pg_current_xlog_{insert_,}location,
pg_last_xlog_{receive,replay}_location(), pg_xlogfile_name_offset(),
pg_xlogfile_name().

Another chunk that seems like it would be pretty simple to separate
out is the checkpoint-related stuff: LogCheckpointStart(),
LogCheckpointEnd(), CreateCheckPoint(), CheckPointGuts(),
RecoveryRestartPoint(), CreateRestartPoint(), KeepLogSeg().  Not a lot
of code, maybe, but it seems clearly distinguishable from what the
rest of the file is about.

I'm sure there are other good ways to do it, too...

-- 
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] Large C files

2011-09-08 Thread Heikki Linnakangas

On 08.09.2011 23:45, Peter Geoghegan wrote:

On 8 September 2011 15:43, Robert Haasrobertmh...@gmail.com  wrote:

I wouldn't be too enthusiastic about
starting a project like this in January, but now seems fine.  A bigger
problem is that I don't hear anyone volunteering to do the work.


You seem to have a fairly strong opinion on the xlog.c code. It would
be useful to hear any preliminary thoughts that you might have on what
we'd end up with when this refactoring work is finished. If I'm not
mistaken, you think that it is a good candidate for being refactored
not so much because of its size, but for other reasons -  could you
please elaborate on those? In particular, I'd like to know what
boundaries it is envisaged that the code should be refactored along to
increase its conceptual integrity, or to better separate concerns. I
assume that that's the idea, since each new .c file would have to have
a discrete purpose.


I'd like to see it split into routines involved in writing WAL, and 
those involved in recovery. And maybe a third file for archiving-related 
stuff.


--
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

--
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] Large C files

2011-09-07 Thread Robert Haas
On Tue, Sep 6, 2011 at 9:14 PM, Peter Geoghegan pe...@2ndquadrant.com wrote:
 On 7 September 2011 01:18, Bruce Momjian br...@momjian.us wrote:
 I am confused how moving a function from one C file to another could
 cause breakage?

 I'm really concerned about silent breakage, however unlikely - that is
 also Simon and Robert's concern, and rightly so. If it's possible in
 principle to guard against a certain type of problem, we should do so.
 While this is a mechanical process, it isn't quite that mechanical a
 process - I would not expect this work to be strictly limited to
 simply spreading some functions around different files. Certainly, if
 there are any other factors at all that could disrupt things, a
 problem that does not cause a compiler warning or error is vastly more
 troublesome than one that does, and the most plausible such error is
 if a symbol is somehow resolved differently when the function is
 moved. I suppose that the simplest way that this could happen is
 probably by somehow having a variable of the same name and type appear
 twice, once as a static, the other time as a global.

 IMHO, when manipulating code at this sort of macro scale, it's good to
 be paranoid and exhaustive, particularly when it doesn't cost you
 anything anyway. Unlike when writing or fixing a bit of code at a
 time, which we're all used to, if the compiler doesn't tell you about
 it, your chances of catching the problem before a bug manifests itself
 are close to zero.

I was less concerned about the breakage that might be caused by
variables acquiring unintended referents - which should be unlikely
anyway given reasonable variable naming conventions - and more
concerned that the associated refactoring would break recovery.  We
have no recovery regression tests; that's not a good thing.

-- 
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] Large C files

2011-09-07 Thread Bruce Momjian
Robert Haas wrote:
  IMHO, when manipulating code at this sort of macro scale, it's good to
  be paranoid and exhaustive, particularly when it doesn't cost you
  anything anyway. Unlike when writing or fixing a bit of code at a
  time, which we're all used to, if the compiler doesn't tell you about
  it, your chances of catching the problem before a bug manifests itself
  are close to zero.
 
 I was less concerned about the breakage that might be caused by
 variables acquiring unintended referents - which should be unlikely
 anyway given reasonable variable naming conventions - and more
 concerned that the associated refactoring would break recovery.  We
 have no recovery regression tests; that's not a good thing.

So we are talking about more than moving files between functions?  Yes,
it would be risky to restruction functions, but for someone who
understand that code, it might be safe.

-- 
  Bruce Momjian  br...@momjian.ushttp://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] Large C files

2011-09-07 Thread Tom Lane
Bruce Momjian br...@momjian.us writes:
 Robert Haas wrote:
 I was less concerned about the breakage that might be caused by
 variables acquiring unintended referents - which should be unlikely
 anyway given reasonable variable naming conventions - and more
 concerned that the associated refactoring would break recovery.  We
 have no recovery regression tests; that's not a good thing.

 So we are talking about more than moving files between functions?  Yes,
 it would be risky to restruction functions, but for someone who
 understand that code, it might be safe.

The pgrminclude-induced bug you just fixed shows a concrete way in which
moving code from one file to another might silently break it, ie, it
still compiles despite lack of definition of some symbol it's intended
to see.

Having said that, I tend to agree that xlog.c is getting so large and
messy that it needs to be broken up.  But I'm not in favor of breaking
up files just because they're large, eg, ruleutils.c is not in need of
such treatment.  The problem with xlog.c is that it seems to be dealing
with many more considerations than it originally did.

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] Large C files

2011-09-07 Thread Simon Riggs
On Wed, Sep 7, 2011 at 7:12 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Bruce Momjian br...@momjian.us writes:
 Robert Haas wrote:
 I was less concerned about the breakage that might be caused by
 variables acquiring unintended referents - which should be unlikely
 anyway given reasonable variable naming conventions - and more
 concerned that the associated refactoring would break recovery.  We
 have no recovery regression tests; that's not a good thing.

 So we are talking about more than moving files between functions?  Yes,
 it would be risky to restruction functions, but for someone who
 understand that code, it might be safe.

 The pgrminclude-induced bug you just fixed shows a concrete way in which
 moving code from one file to another might silently break it, ie, it
 still compiles despite lack of definition of some symbol it's intended
 to see.

 Having said that, I tend to agree that xlog.c is getting so large and
 messy that it needs to be broken up.  But I'm not in favor of breaking
 up files just because they're large, eg, ruleutils.c is not in need of
 such treatment.  The problem with xlog.c is that it seems to be dealing
 with many more considerations than it originally did.

I agree as well, though we've spawned many new files and directories
in the last 7 years. Almost nothing has gone in there that didn't need
to.

As long as we accept its not a priority, I'll do some work to slide
things away and we can do it over time.

Please lets not waste effort on refactoring efforts in mid dev cycle.
Having this done by someone without good experience is just going to
waste all of our time and sneak bugs into something that does actually
work rather well.

-- 
 Simon Riggs   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] Large C files

2011-09-07 Thread Tom Lane
Simon Riggs si...@2ndquadrant.com writes:
 Please lets not waste effort on refactoring efforts in mid dev cycle.

Say what?  When else would you have us do it?

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] Large C files

2011-09-06 Thread Peter Eisentraut
On lör, 2011-09-03 at 19:18 -0400, Bruce Momjian wrote:
 FYI, here are all the C files with over 6k lines:
 
 -  45133 ./interfaces/ecpg/preproc/preproc.c
 -  33651 ./backend/parser/gram.c
 -  17551 ./backend/parser/scan.c
14209 ./bin/pg_dump/pg_dump.c
10590 ./backend/access/transam/xlog.c
 9764 ./backend/commands/tablecmds.c
 8681 ./backend/utils/misc/guc.c
 -   7667 ./bin/psql/psqlscan.c
 7213 ./backend/utils/adt/ruleutils.c
 6814 ./backend/utils/adt/selfuncs.c
 6176 ./backend/utils/adt/numeric.c
 6030 ./pl/plpgsql/src/pl_exec.c
 
 I have dash-marked the files that are computer-generated.  It seems
 pg_dump.c and xlog.c should be split into smaller C files.

I was thinking about splitting up plpython.c, but it's not even on that
list. ;-)



-- 
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] Large C files

2011-09-06 Thread Bruce Momjian
Peter Eisentraut wrote:
 On l?r, 2011-09-03 at 19:18 -0400, Bruce Momjian wrote:
  FYI, here are all the C files with over 6k lines:
  
  -  45133 ./interfaces/ecpg/preproc/preproc.c
  -  33651 ./backend/parser/gram.c
  -  17551 ./backend/parser/scan.c
 14209 ./bin/pg_dump/pg_dump.c
 10590 ./backend/access/transam/xlog.c
  9764 ./backend/commands/tablecmds.c
  8681 ./backend/utils/misc/guc.c
  -   7667 ./bin/psql/psqlscan.c
  7213 ./backend/utils/adt/ruleutils.c
  6814 ./backend/utils/adt/selfuncs.c
  6176 ./backend/utils/adt/numeric.c
  6030 ./pl/plpgsql/src/pl_exec.c
  
  I have dash-marked the files that are computer-generated.  It seems
  pg_dump.c and xlog.c should be split into smaller C files.
 
 I was thinking about splitting up plpython.c, but it's not even on that
 list. ;-)

For me, the test is when I feel, Yuck, I am in that massive file
again.

-- 
  Bruce Momjian  br...@momjian.ushttp://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] Large C files

2011-09-06 Thread Simon Riggs
On Tue, Sep 6, 2011 at 3:05 PM, Bruce Momjian br...@momjian.us wrote:
 Peter Eisentraut wrote:
 On l?r, 2011-09-03 at 19:18 -0400, Bruce Momjian wrote:
  FYI, here are all the C files with over 6k lines:
 
  -  45133 ./interfaces/ecpg/preproc/preproc.c
  -  33651 ./backend/parser/gram.c
  -  17551 ./backend/parser/scan.c
     14209 ./bin/pg_dump/pg_dump.c
     10590 ./backend/access/transam/xlog.c
      9764 ./backend/commands/tablecmds.c
      8681 ./backend/utils/misc/guc.c
  -   7667 ./bin/psql/psqlscan.c
      7213 ./backend/utils/adt/ruleutils.c
      6814 ./backend/utils/adt/selfuncs.c
      6176 ./backend/utils/adt/numeric.c
      6030 ./pl/plpgsql/src/pl_exec.c
 
  I have dash-marked the files that are computer-generated.  It seems
  pg_dump.c and xlog.c should be split into smaller C files.

 I was thinking about splitting up plpython.c, but it's not even on that
 list. ;-)

 For me, the test is when I feel, Yuck, I am in that massive file
 again.

There are many things to do yet in xlog.c, but splitting it into
pieces is pretty low on the list.

I did look at doing it years ago before we started the heavy lifting
for SR/HS but it was too much work and dangerous too.

The only way I would entertain thoughts of major refactoring would be
if comprehensive tests were contributed, so we could verify everything
still works afterwards.

-- 
 Simon Riggs   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] Large C files

2011-09-06 Thread Robert Haas
On Tue, Sep 6, 2011 at 3:56 PM, Simon Riggs si...@2ndquadrant.com wrote:
 The only way I would entertain thoughts of major refactoring would be
 if comprehensive tests were contributed, so we could verify everything
 still works afterwards.

That sounds like a really good idea.  Mind you, I don't have a very
clear idea of how to design such tests and probably no immediate
availability to do the work either, but I like the concept very much.

-- 
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] Large C files

2011-09-06 Thread Peter Geoghegan
On 6 September 2011 21:07, Robert Haas robertmh...@gmail.com wrote:
 On Tue, Sep 6, 2011 at 3:56 PM, Simon Riggs si...@2ndquadrant.com wrote:
 The only way I would entertain thoughts of major refactoring would be
 if comprehensive tests were contributed, so we could verify everything
 still works afterwards.

 That sounds like a really good idea.  Mind you, I don't have a very
 clear idea of how to design such tests and probably no immediate
 availability to do the work either, but I like the concept very much.

More or less off the top of my head, I don't think that it's much
trouble to write an automated test for this. Of course, it would be as
flawed as any test in that it can only prove the presence of errors by
the criteria of the test, not the absence of all errors.

Here's what could go wrong that we can test for when splitting a
monster translation unit into more manageable modules that I can
immediately think of, that is not already handled by compiler
diagnostics or the build farm (I'm thinking of problems arising when
some headers are excluded in new .c files because they appear to be
superfluous, but turn out to not be on some platform):

* Across TUs, we somehow fail to provide consistent linkage between
the old object file and the sum of the new object files.

* Within TUs, we unshadow a previously shadowed variable, so we link
to a global variable rather than one local to the original/other new
file. Unlikely to be a problem. Here's what I get when I compile
xlog.c in the usual way with the addition of the -Wshadow flag:

[peter@localhost transam]$ gcc -O0 -g -Wall -Wmissing-prototypes
-Wpointer-arith -Wdeclaration-after-statement -Wendif-labels
-Wformat-security -fno-strict-aliasing -fwrapv -Wshadow -g
-I../../../../src/include -D_GNU_SOURCE   -c -o xlog.o xlog.c -MMD -MP
-MF .deps/xlog.Po
xlog.c: In function ‘XLogCheckBuffer’:
xlog.c:1279:12: warning: declaration of ‘lower’ shadows a global declaration
../../../../src/include/utils/builtins.h:793:14: warning: shadowed
declaration is here
xlog.c:1280:12: warning: declaration of ‘upper’ shadows a global declaration
../../../../src/include/utils/builtins.h:794:14: warning: shadowed
declaration is here
xlog.c: In function ‘XLogArchiveNotifySeg’:
xlog.c:1354:29: warning: declaration of ‘log’ shadows a global declaration
xlog.c: In function ‘XLogFileInit’:
xlog.c:2329:21: warning: declaration of ‘log’ shadows a global declaration
xlog.c: In function ‘XLogFileCopy’:
xlog.c:2480:21: warning: declaration of ‘log’ shadows a global declaration
xlog.c: In function ‘InstallXLogFileSegment’:
xlog.c:2598:32: warning: declaration of ‘log’ shadows a global declaration
xlog.c: In function ‘XLogFileOpen’:
xlog.c:2676:21: warning: declaration of ‘log’ shadows a global declaration
xlog.c: In function ‘XLogFileRead’:
*** SNIP, CUT OUT A BIT MORE OF SAME***


Having looked at the man page for nm, it should be possible to hack
together a shellscript for src/tools/ that:

1. Takes one filename as its first argument, and a set of 2 or more
equivalent split file names (no file extension - there is a
requirement that both $FILENAME.c and $FILENAME.o exist).

2. Looks at the symbol table for the original C file's corresponding
object file, say xlog.o, as output from nm, and sorts it.

3. Intelligently diffs that against the concatenated output of the
symbol table for each new object file. This output would be sorted
just as the the single object file nm output was, but only after
concatenation. Here, by intelligently I mean that we exclude undefined
symbols. That way, it shouldn't matter if symbols go missing when, for
example, Text section symbols from one file but show up in multiple
other new files as undefined symbols, nor should it matter that we
call functions like memcpy() from each new file that only appeared
once in the old object file's symbol table.

4. Do a similar kind of intelligent diff with the -Wshadow ouput
above, stripping out line numbers and file names as the first step of
a pipline, using sed, sorting the orignal file's compiler output, and
stripping, concatenating then sorting the new set of outputs. I think
that after that, the output from the original file should be the same
as the combined output of the new files, unless we messed up.

If you have to give a previously static variable global linkage, then
prima facie you're doing it wrong, so we don't have to worry about
that case - maybe you can argue that it's okay in this one case, but
that's considered controversial. We detect this case because the
symbol type goes from 'd' to 'D'.

Of course, we expect that some functions will lose their internal
linkage as part of this process, and that is output by the shellscript
- no attempt is made to hide that. This test doesn't reduce the
failure to a simple pass or fail - it has to be interpreted by a
human. It does take the drudgery out of verifying that this mechanical
process has been performed correctly though.

I agree that C files shouldn't 

Re: [HACKERS] Large C files

2011-09-06 Thread Jan Urbański
On 07/09/11 01:13, Peter Geoghegan wrote:
 On 6 September 2011 08:29, Peter Eisentraut pete...@gmx.net wrote:
 I was thinking about splitting up plpython.c, but it's not even on that
 list. ;-)
 
 IIRC the obesity of that file is something that Jan Urbański intends
 to fix, or is at least concerned about. Perhaps you should compare
 notes.

Yeah, plpython.c could easily be splitted into submodules for builtin
functions, spi support, subtransactions support, traceback support etc.

If there is consensus that this should be done, I'll see if I can find
time to submit a giant-diff-no-behaviour-changes patch for the next CF.

Cheers,
Jan

-- 
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] Large C files

2011-09-06 Thread Peter Geoghegan
On 7 September 2011 00:13, Peter Geoghegan pe...@2ndquadrant.com wrote:
 * Within TUs, we unshadow a previously shadowed variable, so we link
 to a global variable rather than one local to the original/other new
 file. Unlikely to be a problem. Here's what I get when I compile
 xlog.c in the usual way with the addition of the -Wshadow flag:

I hit send too soon. Of course, this isn't going to matter in the case
I described because an extern declaration of int foo cannot appear in
the same TU as a static declaration of int foo - it won't compile. I
hastily gave that as an example of a general phenomenon that can occur
when performing this splitting process. An actually valid example of
same would be if someone refactored functions a bit as part of this
process to make things more modular, and now referenced a global
variable rather than a local one as part of that process. This is
quite possible, because namespace pollution is a big problem with
heavyweight C files - Just look at how much output that -Wshadow flag
gives when used on xlog.c.

-- 
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] Large C files

2011-09-06 Thread Bruce Momjian
Peter Geoghegan wrote:
 On 7 September 2011 00:13, Peter Geoghegan pe...@2ndquadrant.com wrote:
  * Within TUs, we unshadow a previously shadowed variable, so we link
  to a global variable rather than one local to the original/other new
  file. Unlikely to be a problem. Here's what I get when I compile
  xlog.c in the usual way with the addition of the -Wshadow flag:
 
 I hit send too soon. Of course, this isn't going to matter in the case
 I described because an extern declaration of int foo cannot appear in
 the same TU as a static declaration of int foo - it won't compile. I
 hastily gave that as an example of a general phenomenon that can occur
 when performing this splitting process. An actually valid example of
 same would be if someone refactored functions a bit as part of this
 process to make things more modular, and now referenced a global
 variable rather than a local one as part of that process. This is
 quite possible, because namespace pollution is a big problem with
 heavyweight C files - Just look at how much output that -Wshadow flag
 gives when used on xlog.c.

I am confused how moving a function from one C file to another could
cause breakage?

-- 
  Bruce Momjian  br...@momjian.ushttp://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] Large C files

2011-09-06 Thread Peter Geoghegan
On 7 September 2011 01:18, Bruce Momjian br...@momjian.us wrote:
 I am confused how moving a function from one C file to another could
 cause breakage?

I'm really concerned about silent breakage, however unlikely - that is
also Simon and Robert's concern, and rightly so. If it's possible in
principle to guard against a certain type of problem, we should do so.
While this is a mechanical process, it isn't quite that mechanical a
process - I would not expect this work to be strictly limited to
simply spreading some functions around different files. Certainly, if
there are any other factors at all that could disrupt things, a
problem that does not cause a compiler warning or error is vastly more
troublesome than one that does, and the most plausible such error is
if a symbol is somehow resolved differently when the function is
moved. I suppose that the simplest way that this could happen is
probably by somehow having a variable of the same name and type appear
twice, once as a static, the other time as a global.

IMHO, when manipulating code at this sort of macro scale, it's good to
be paranoid and exhaustive, particularly when it doesn't cost you
anything anyway. Unlike when writing or fixing a bit of code at a
time, which we're all used to, if the compiler doesn't tell you about
it, your chances of catching the problem before a bug manifests itself
are close to zero.

-- 
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] Large C files

2011-09-05 Thread Alvaro Herrera
Excerpts from Bruce Momjian's message of sáb sep 03 20:18:47 -0300 2011:
 FYI, here are all the C files with over 6k lines:
 
 -  45133 ./interfaces/ecpg/preproc/preproc.c
 -  33651 ./backend/parser/gram.c
 -  17551 ./backend/parser/scan.c
14209 ./bin/pg_dump/pg_dump.c
10590 ./backend/access/transam/xlog.c
 9764 ./backend/commands/tablecmds.c
 8681 ./backend/utils/misc/guc.c
 -   7667 ./bin/psql/psqlscan.c
 7213 ./backend/utils/adt/ruleutils.c
 6814 ./backend/utils/adt/selfuncs.c
 6176 ./backend/utils/adt/numeric.c
 6030 ./pl/plpgsql/src/pl_exec.c
 
 I have dash-marked the files that are computer-generated.  It seems
 pg_dump.c and xlog.c should be split into smaller C files.

I don't think there's any particular point to this general exercise (too
large for what?), but Simon had patches (or at least ideas) to split
xlog.c IIRC.

-- 
Álvaro Herrera alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

-- 
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] Large C files

2011-09-05 Thread Robert Haas
On Mon, Sep 5, 2011 at 6:56 PM, Alvaro Herrera
alvhe...@commandprompt.com wrote:
 Excerpts from Bruce Momjian's message of sáb sep 03 20:18:47 -0300 2011:
 FYI, here are all the C files with over 6k lines:

 -  45133 ./interfaces/ecpg/preproc/preproc.c
 -  33651 ./backend/parser/gram.c
 -  17551 ./backend/parser/scan.c
    14209 ./bin/pg_dump/pg_dump.c
    10590 ./backend/access/transam/xlog.c
     9764 ./backend/commands/tablecmds.c
     8681 ./backend/utils/misc/guc.c
 -   7667 ./bin/psql/psqlscan.c
     7213 ./backend/utils/adt/ruleutils.c
     6814 ./backend/utils/adt/selfuncs.c
     6176 ./backend/utils/adt/numeric.c
     6030 ./pl/plpgsql/src/pl_exec.c

 I have dash-marked the files that are computer-generated.  It seems
 pg_dump.c and xlog.c should be split into smaller C files.

 I don't think there's any particular point to this general exercise (too
 large for what?), but Simon had patches (or at least ideas) to split
 xlog.c IIRC.

Yeah.  xlog.c and pg_dump.c are really pretty horrible code, and could
probably benefit from being split up.  Actually, just splitting them
up probably isn't enough: I think they need extensive refactoring.
For example, ISTM that StartupXLOG() should delegate substantial
chunks of what it does to subroutines, so that the toplevel function
is short enough to read and understand without getting lost.

On the other hand, I can't help but think splitting up numeric.c would
be a bad idea all around.  There's not really going to be any coherent
way of dividing up the functionality in that file, and it would hinder
the ability to make functions static and keep interfaces private.

-- 
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