Tom Lane wrote:
Clean up the #include mess a little.

walsender.h should depend on xlog.h, not vice versa. (Actually, the
inclusion was circular until a couple hours ago, which was even sillier;
but Bruce broke it in the expedient rather than logically correct
direction.) Because of that poor decision, plus blind application of
pgrminclude, we had a situation where half the system was depending on
xlog.h to include such unrelated stuff as array.h and guc.h. Clean up
the header inclusion, and manually revert a lot of what pgrminclude had
done so things build again.

This episode reinforces my feeling that pgrminclude should not be run
without adult supervision. Inclusion changes in header files in particular
need to be reviewed with great care. More generally, it'd be good if we
had a clearer notion of module layering to dictate which headers can sanely
include which others ... but that's a big task for another day.
What pgrminclude does it to lock down the minimal set of includes, and
that easily could cause something like xlog.h becoming the go-to include
file for many C files. I don't remember this problem happening before
but it clearly happened this time.

Not sure how to avoid that except, as you said, analyze the entire
changeset of pgrminclude. For this run, I focused on not breaking any
platform builds so I was not focusing on the actual include file layout.
I assumed fiddling with the actual pgrminclude output would likely break
builds.

The process I used was to get pgcompinclude to allow all include files
to compile (so they their inclusion would not bleed into files that
included them), then run pgrminclude.

Well, I assume we are done for another five years. The includes removed
were minimal, especially considering five years of work.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Search Discussions

  • Greg Stark at Sep 5, 2011 at 1:57 pm

    On Mon, Sep 5, 2011 at 2:52 PM, Bruce Momjian wrote:
    Well, I assume we are done for another five years.  The includes removed
    were minimal, especially considering five years of work.
    What I wouldn't mind seeing is a graph of all includes and what they
    include. This might help figure out what layering violations there are
    like the one that caused this mess. I think I've seen tools to do this
    already somewhere.

    --
    greg
  • Magnus Hagander at Sep 5, 2011 at 2:02 pm

    On Mon, Sep 5, 2011 at 15:55, Greg Stark wrote:
    On Mon, Sep 5, 2011 at 2:52 PM, Bruce Momjian wrote:
    Well, I assume we are done for another five years.  The includes removed
    were minimal, especially considering five years of work.
    What I wouldn't mind seeing is a graph of all includes and what they
    include. This might help figure out what layering violations there are
    like the one that caused this mess. I think I've seen tools to do this
    already somewhere.

    http://doxygen.postgresql.org will do some of that, but I think not
    globally - but if you click into one header, I think it shows you the
    map from that perspective.
  • Alvaro Herrera at Sep 5, 2011 at 3:07 pm

    Excerpts from Magnus Hagander's message of lun sep 05 11:02:23 -0300 2011:
    On Mon, Sep 5, 2011 at 15:55, Greg Stark wrote:
    On Mon, Sep 5, 2011 at 2:52 PM, Bruce Momjian wrote:
    Well, I assume we are done for another five years.  The includes removed
    were minimal, especially considering five years of work.
    What I wouldn't mind seeing is a graph of all includes and what they
    include. This might help figure out what layering violations there are
    like the one that caused this mess. I think I've seen tools to do this
    already somewhere.
    http://doxygen.postgresql.org will do some of that, but I think not
    globally - but if you click into one header, I think it shows you the
    map from that perspective.
    Yeah; and it isn't always complete, because some graphs tend to get too
    unwieldy so it has to prune (you can see this because some nodes show up
    with red borders).

    I am not sure it is really feasible to build a complete graph for all
    headers. We have too many of them and too many dependencies.

    Another useful graph to see is what files include a given header. A
    funny thing is that doxygen doesn't always display this; for example
    http://doxygen.postgresql.org/rel_8h.html

    --
    Álvaro Herrera <alvherre@commandprompt.com>
    The PostgreSQL Company - Command Prompt, Inc.
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support
  • Tom Lane at Sep 5, 2011 at 3:59 pm

    Alvaro Herrera writes:
    I am not sure it is really feasible to build a complete graph for all
    headers. We have too many of them and too many dependencies.
    Yeah, it's the "too many dependencies" aspect that is bothering me.

    The only concrete idea I've come up with so far is that it'd be a good
    idea to isolate certain primitive datatypes into their own group of
    headers. We have a number of headers that are meant to be this sort
    of animal already, eg storage/block.h, storage/relfilenode.h. But
    (1) there's no clear distinction between these headers and ones like,
    say, storage/smgr.h or storage/proc.h.
    (2) other things that have become widely-used primitive datatypes,
    such as TimestampTz, are declared in places that ideally ought to be
    near the top of the #include hierarchy not the bottom.

    So I think we could make some forward progress by moving all these
    simple datatype declarations into a separate set of headers in their
    own subdirectory of src/include/, perhaps "datatype". There would
    be a hard and fast rule that no header in this set could depend on
    anything beyond postgres.h and other members of the same set, so
    that these headers clearly form the bottom level of the #include
    hierarchy. Probably some of the stuff now in postgres.h could migrate
    to this group too.

    Eventually I'd like to see some fairly clear layering rules at the
    header-directory level, like "storage/ is lower-level than commands/
    so nothing in the former directory should include anything in the
    latter". But achieving that is a long way off.

    Of course, the problem with all of this is that making much progress
    would be a large amount of work with relatively small concrete payoff.
    Still, I'm starting to feel that we've got such a spaghetti-like mess
    that we need to do something.

    regards, tom lane
  • Ants Aasma at Sep 6, 2011 at 3:40 pm

    On Mon, Sep 5, 2011 at 4:55 PM, Greg Stark wrote:
    What I wouldn't mind seeing is a graph of all includes and what they
    include. This might help figure out what layering violations there are
    like the one that caused this mess. I think I've seen tools to do this
    already somewhere.
    I whipped together a quick Python script to do this. Attached is the
    Python script (requires pydot) and the result of running it on includes/.
    I didn't attach the png version of the output because it was 7MB.

    If rendering all includes at once doesn't give a good overview it can
    also select a subset through traversing dependencies. For example:
    render_includes.py -i include/ \
    --select="storage/spin.h+*,access/xlog.h+*" output.png

    This will render everything that directly or indirectly depends on those
    two headers. See --help for details.

    --
    Ants Aasma
  • Alvaro Herrera at Sep 6, 2011 at 7:18 pm

    Excerpts from Ants Aasma's message of mar sep 06 12:40:04 -0300 2011:
    On Mon, Sep 5, 2011 at 4:55 PM, Greg Stark wrote:
    What I wouldn't mind seeing is a graph of all includes and what they
    include. This might help figure out what layering violations there are
    like the one that caused this mess. I think I've seen tools to do this
    already somewhere.
    I whipped together a quick Python script to do this. Attached is the
    Python script (requires pydot) and the result of running it on includes/.
    I didn't attach the png version of the output because it was 7MB.

    If rendering all includes at once doesn't give a good overview it can
    also select a subset through traversing dependencies. For example:
    render_includes.py -i include/ \
    --select="storage/spin.h+*,access/xlog.h+*" output.png

    This will render everything that directly or indirectly depends on those
    two headers. See --help for details.
    Wow, interesting, thanks.

    What this says to me is that we should do something about execnodes.h
    and some other nodes file (parsenodes.h I think).

    I wonder what happens if files in the same subdir are grouped in a
    subgraph. Is that possible?

    --
    Álvaro Herrera <alvherre@commandprompt.com>
    The PostgreSQL Company - Command Prompt, Inc.
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support
  • Ants Aasma at Sep 6, 2011 at 10:20 pm

    On Tue, Sep 6, 2011 at 10:18 PM, Alvaro Herrera wrote:
    I wonder what happens if files in the same subdir are grouped in a
    subgraph.  Is that possible?
    Possible, and done. Also added possivility to add .c files to the graph,
    coloring by subdir and possibility exclude nodes from the graph. I didn't yet
    bother to clean up the code - to avoid eye damage, don't look at the source.

    Bad news is that it doesn't significantly help readability for the all nodes
    case. See all_with_subgraphs.svgz. It does help for other cases.
    For example parsenodes.h.svgz has the result for
    render_includes.py --select='nodes/parsenodes.h+*-*' --subgraphs
    and execnodes.h.svgz for
    --subgraphs --select='nodes/execnodes.h+*-*'

    --
    Ants Aasma
  • Bruce Momjian at Aug 15, 2012 at 10:30 pm

    On Wed, Sep 7, 2011 at 01:20:17AM +0300, Ants Aasma wrote:
    On Tue, Sep 6, 2011 at 10:18 PM, Alvaro Herrera
    wrote:
    I wonder what happens if files in the same subdir are grouped in a
    subgraph.  Is that possible?
    Possible, and done. Also added possivility to add .c files to the graph,
    coloring by subdir and possibility exclude nodes from the graph. I didn't yet
    bother to clean up the code - to avoid eye damage, don't look at the source.

    Bad news is that it doesn't significantly help readability for the all nodes
    case. See all_with_subgraphs.svgz. It does help for other cases.
    For example parsenodes.h.svgz has the result for
    render_includes.py --select='nodes/parsenodes.h+*-*' --subgraphs
    and execnodes.h.svgz for
    --subgraphs --select='nodes/execnodes.h+*-*'
    Should we add this script and instructions to src/tools/pginclude?

    --
    Bruce Momjian <bruce@momjian.us> http://momjian.us
    EnterpriseDB http://enterprisedb.com

    + It's impossible for everything to be true. +
  • Alvaro Herrera at Aug 16, 2012 at 3:15 pm

    Excerpts from Bruce Momjian's message of mié ago 15 18:30:40 -0400 2012:
    On Wed, Sep 7, 2011 at 01:20:17AM +0300, Ants Aasma wrote:
    On Tue, Sep 6, 2011 at 10:18 PM, Alvaro Herrera
    wrote:
    I wonder what happens if files in the same subdir are grouped in a
    subgraph.  Is that possible?
    Possible, and done. Also added possivility to add .c files to the graph,
    coloring by subdir and possibility exclude nodes from the graph. I didn't yet
    bother to clean up the code - to avoid eye damage, don't look at the source.

    Bad news is that it doesn't significantly help readability for the all nodes
    case. See all_with_subgraphs.svgz. It does help for other cases.
    For example parsenodes.h.svgz has the result for
    render_includes.py --select='nodes/parsenodes.h+*-*' --subgraphs
    and execnodes.h.svgz for
    --subgraphs --select='nodes/execnodes.h+*-*'
    Should we add this script and instructions to src/tools/pginclude?
    Probably not, but maybe the developer FAQ in the wiki?

    --
    Álvaro Herrera http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training & Services
  • Bruce Momjian at Aug 16, 2012 at 10:52 pm

    On Thu, Aug 16, 2012 at 11:15:24AM -0400, Alvaro Herrera wrote:
    Excerpts from Bruce Momjian's message of mié ago 15 18:30:40 -0400 2012:
    On Wed, Sep 7, 2011 at 01:20:17AM +0300, Ants Aasma wrote:
    On Tue, Sep 6, 2011 at 10:18 PM, Alvaro Herrera
    wrote:
    I wonder what happens if files in the same subdir are grouped in a
    subgraph.  Is that possible?
    Possible, and done. Also added possivility to add .c files to the graph,
    coloring by subdir and possibility exclude nodes from the graph. I didn't yet
    bother to clean up the code - to avoid eye damage, don't look at the source.

    Bad news is that it doesn't significantly help readability for the all nodes
    case. See all_with_subgraphs.svgz. It does help for other cases.
    For example parsenodes.h.svgz has the result for
    render_includes.py --select='nodes/parsenodes.h+*-*' --subgraphs
    and execnodes.h.svgz for
    --subgraphs --select='nodes/execnodes.h+*-*'
    Should we add this script and instructions to src/tools/pginclude?
    Probably not, but maybe the developer FAQ in the wiki?
    I just added the script email URL to the pginclude README.

    --
    Bruce Momjian <bruce@momjian.us> http://momjian.us
    EnterpriseDB http://enterprisedb.com

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

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedSep 5, '11 at 1:52p
activeAug 16, '12 at 10:52p
posts11
users7
websitepostgresql.org...
irc#postgresql

People

Translate

site design / logo © 2022 Grokbase