Large C files

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 <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Mon, Sep 5, 2011 at 6:56 PM, Alvaro Herrera
<alvherre@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

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. ;-)

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 <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Tue, Sep 6, 2011 at 3:05 PM, Bruce Momjian <bruce@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

On Tue, Sep 6, 2011 at 3:56 PM, Simon Riggs <simon@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

On 6 September 2011 21:07, Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, Sep 6, 2011 at 3:56 PM, Simon Riggs <simon@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 be split because they're big, but
because modularising them is a maintainability win. I also think that
10,000 line C files are a real problem that we should think about
addressing. These two views may seem to be in tension, but they're not
- if you're not able to usefully modularise such a large file, perhaps
it's time to reconsider your design (this is not an allusion to any
problems that I might believe any particular C file has) .

Anyone interested in this idea? I think that an actual implementation
will probably reveal a few more problems that haven't occurred to me
yet, but it's too late to produce one right now.

On 6 September 2011 08:29, Peter Eisentraut <peter_e@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.

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

On 07/09/11 01:13, Peter Geoghegan wrote:

On 6 September 2011 08:29, Peter Eisentraut <peter_e@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

On 7 September 2011 00:13, Peter Geoghegan <peter@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

Peter Geoghegan wrote:

On 7 September 2011 00:13, Peter Geoghegan <peter@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 <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On 7 September 2011 01:18, Bruce Momjian <bruce@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

On Tue, Sep 6, 2011 at 9:14 PM, Peter Geoghegan <peter@2ndquadrant.com> wrote:

On 7 September 2011 01:18, Bruce Momjian <bruce@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

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 <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian <bruce@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

On Wed, Sep 7, 2011 at 7:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Bruce Momjian <bruce@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

Simon Riggs <simon@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

On Wed, Sep 7, 2011 at 9:02 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Simon Riggs <simon@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

Simon Riggs <simon@2ndQuadrant.com> writes:

On Wed, Sep 7, 2011 at 9:02 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Simon Riggs <simon@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

Tom Lane wrote:

Simon Riggs <simon@2ndQuadrant.com> writes:

On Wed, Sep 7, 2011 at 9:02 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Simon Riggs <simon@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 <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +