Thread: Headers dependencies cleanup
During my work I found some header files, which include useless headers
and on other way there are some headers which silently assume that
requested header will be included by some other headers file.
This patch fixes these dependencies around bufpage.h, bufmgr.h
Zdenek
Index: src/backend/access/transam/xlogutils.c
===================================================================
RCS file: /zfs_data/cvs_pgsql/cvsroot/pgsql/src/backend/access/transam/xlogutils.c,v
retrieving revision 1.50
diff -c -r1.50 xlogutils.c
*** src/backend/access/transam/xlogutils.c 2 May 2007 23:18:03 -0000 1.50
--- src/backend/access/transam/xlogutils.c 25 Oct 2007 09:16:59 -0000
***************
*** 18,23 ****
--- 18,24 ----
#include "postgres.h"
#include "access/xlogutils.h"
+ #include "storage/bufmgr.h"
#include "storage/bufpage.h"
#include "storage/smgr.h"
#include "utils/hsearch.h"
Index: src/backend/nodes/print.c
===================================================================
RCS file: /zfs_data/cvs_pgsql/cvsroot/pgsql/src/backend/nodes/print.c,v
retrieving revision 1.85
diff -c -r1.85 print.c
*** src/backend/nodes/print.c 22 Feb 2007 22:00:23 -0000 1.85
--- src/backend/nodes/print.c 25 Oct 2007 09:16:59 -0000
***************
*** 24,30 ****
#include "optimizer/clauses.h"
#include "parser/parsetree.h"
#include "utils/lsyscache.h"
!
/*
* print
--- 24,30 ----
#include "optimizer/clauses.h"
#include "parser/parsetree.h"
#include "utils/lsyscache.h"
! #include "lib/stringinfo.h"
/*
* print
Index: src/backend/nodes/tidbitmap.c
===================================================================
RCS file: /zfs_data/cvs_pgsql/cvsroot/pgsql/src/backend/nodes/tidbitmap.c,v
retrieving revision 1.13
diff -c -r1.13 tidbitmap.c
*** src/backend/nodes/tidbitmap.c 20 Sep 2007 17:56:31 -0000 1.13
--- src/backend/nodes/tidbitmap.c 25 Oct 2007 09:16:59 -0000
***************
*** 36,41 ****
--- 36,42 ----
#include "nodes/tidbitmap.h"
#include "storage/bufpage.h"
#include "utils/hsearch.h"
+ #include "nodes/nodes.h"
/*
* The maximum number of tuples per page is not large (typically 256 with
Index: src/backend/postmaster/postmaster.c
===================================================================
RCS file: /zfs_data/cvs_pgsql/cvsroot/pgsql/src/backend/postmaster/postmaster.c,v
retrieving revision 1.542
diff -c -r1.542 postmaster.c
*** src/backend/postmaster/postmaster.c 26 Sep 2007 22:36:30 -0000 1.542
--- src/backend/postmaster/postmaster.c 25 Oct 2007 09:16:59 -0000
***************
*** 93,98 ****
--- 93,99 ----
#endif
#include "access/transam.h"
+ #include "access/xlog.h"
#include "bootstrap/bootstrap.h"
#include "catalog/pg_control.h"
#include "lib/dllist.h"
Index: src/backend/storage/buffer/bufmgr.c
===================================================================
RCS file: /zfs_data/cvs_pgsql/cvsroot/pgsql/src/backend/storage/buffer/bufmgr.c,v
retrieving revision 1.226
diff -c -r1.226 bufmgr.c
*** src/backend/storage/buffer/bufmgr.c 25 Sep 2007 22:11:48 -0000 1.226
--- src/backend/storage/buffer/bufmgr.c 25 Oct 2007 09:16:59 -0000
***************
*** 37,42 ****
--- 37,43 ----
#include "postmaster/bgwriter.h"
#include "storage/buf_internals.h"
#include "storage/bufpage.h"
+ #include "storage/bufmgr.h"
#include "storage/ipc.h"
#include "storage/proc.h"
#include "storage/smgr.h"
Index: src/backend/utils/adt/domains.c
===================================================================
RCS file: /zfs_data/cvs_pgsql/cvsroot/pgsql/src/backend/utils/adt/domains.c,v
retrieving revision 1.5
diff -c -r1.5 domains.c
*** src/backend/utils/adt/domains.c 5 Jan 2007 22:19:40 -0000 1.5
--- src/backend/utils/adt/domains.c 25 Oct 2007 09:16:59 -0000
***************
*** 35,40 ****
--- 35,41 ----
#include "executor/executor.h"
#include "utils/builtins.h"
#include "utils/lsyscache.h"
+ #include "lib/stringinfo.h"
/*
Index: src/include/access/heapam.h
===================================================================
RCS file: /zfs_data/cvs_pgsql/cvsroot/pgsql/src/include/access/heapam.h,v
retrieving revision 1.127
diff -c -r1.127 heapam.h
*** src/include/access/heapam.h 20 Sep 2007 17:56:32 -0000 1.127
--- src/include/access/heapam.h 25 Oct 2007 11:09:29 -0000
***************
*** 18,27 ****
--- 18,29 ----
#include "access/relscan.h"
#include "access/sdir.h"
#include "access/tupmacs.h"
+ #include "access/xlog.h"
#include "access/xlogutils.h"
#include "nodes/primnodes.h"
#include "storage/block.h"
#include "storage/lmgr.h"
+ #include "storage/bufpage.h"
#include "utils/rel.h"
#include "utils/tqual.h"
Index: src/include/access/htup.h
===================================================================
RCS file: /zfs_data/cvs_pgsql/cvsroot/pgsql/src/include/access/htup.h,v
retrieving revision 1.94
diff -c -r1.94 htup.h
*** src/include/access/htup.h 20 Sep 2007 17:56:32 -0000 1.94
--- src/include/access/htup.h 25 Oct 2007 10:04:04 -0000
***************
*** 16,21 ****
--- 16,22 ----
#include "storage/itemptr.h"
#include "storage/relfilenode.h"
+ #include "storage/bufpage.h"
/*
* MaxTupleAttributeNumber limits the number of (user) columns in a tuple.
Index: src/include/access/relscan.h
===================================================================
RCS file: /zfs_data/cvs_pgsql/cvsroot/pgsql/src/include/access/relscan.h,v
retrieving revision 1.57
diff -c -r1.57 relscan.h
*** src/include/access/relscan.h 20 Sep 2007 17:56:32 -0000 1.57
--- src/include/access/relscan.h 25 Oct 2007 10:02:57 -0000
***************
*** 15,21 ****
#define RELSCAN_H
#include "access/skey.h"
! #include "storage/bufpage.h"
#include "utils/tqual.h"
--- 15,22 ----
#define RELSCAN_H
#include "access/skey.h"
! #include "access/htup.h"
! #include "storage/bufmgr.h"
#include "utils/tqual.h"
Index: src/include/access/tuptoaster.h
===================================================================
RCS file: /zfs_data/cvs_pgsql/cvsroot/pgsql/src/include/access/tuptoaster.h,v
retrieving revision 1.35
diff -c -r1.35 tuptoaster.h
*** src/include/access/tuptoaster.h 6 Apr 2007 04:21:43 -0000 1.35
--- src/include/access/tuptoaster.h 25 Oct 2007 09:16:59 -0000
***************
*** 15,20 ****
--- 15,21 ----
#include "access/htup.h"
#include "storage/bufpage.h"
+ #include "utils/rel.h"
/*
* This enables de-toasting of index entries. Needed until VACUUM is
Index: src/include/storage/bufpage.h
===================================================================
RCS file: /zfs_data/cvs_pgsql/cvsroot/pgsql/src/include/storage/bufpage.h,v
retrieving revision 1.75
diff -c -r1.75 bufpage.h
*** src/include/storage/bufpage.h 21 Sep 2007 21:25:42 -0000 1.75
--- src/include/storage/bufpage.h 25 Oct 2007 09:17:19 -0000
***************
*** 14,20 ****
#ifndef BUFPAGE_H
#define BUFPAGE_H
- #include "storage/bufmgr.h"
#include "storage/item.h"
#include "storage/off.h"
#include "access/xlog.h"
--- 14,19 ----
Zdenek Kotala wrote: > During my work I found some header files, which include useless headers > and on other way there are some headers which silently assume that > requested header will be included by some other headers file. > > This patch fixes these dependencies around bufpage.h, bufmgr.h What's the point here, again? -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Zdenek Kotala wrote:
>> During my work I found some header files, which include useless headers
>> and on other way there are some headers which silently assume that
>> requested header will be included by some other headers file.
>>
>> This patch fixes these dependencies around bufpage.h, bufmgr.h
> What's the point here, again?
Bruce periodically runs a script that's supposed to catch this type
of problem. Maybe we are overdue for another run?
regards, tom lane
Alvaro Herrera napsal(a):
> Zdenek Kotala wrote:
>> During my work I found some header files, which include useless headers
>> and on other way there are some headers which silently assume that
>> requested header will be included by some other headers file.
>>
>> This patch fixes these dependencies around bufpage.h, bufmgr.h
>
> What's the point here, again?
>
Main point there is bufpage.h I have needed include this header in page layout
convector but it include bufmgr.h which is useless there and generates a lot of
another dependences (e.g. on fmgr.h). Other changes are related to this change.
Zdenek
Tom Lane napsal(a):
> Alvaro Herrera <alvherre@commandprompt.com> writes:
>> Zdenek Kotala wrote:
>>> During my work I found some header files, which include useless headers
>>> and on other way there are some headers which silently assume that
>>> requested header will be included by some other headers file.
>>>
>>> This patch fixes these dependencies around bufpage.h, bufmgr.h
>
>> What's the point here, again?
>
> Bruce periodically runs a script that's supposed to catch this type
> of problem. Maybe we are overdue for another run?
>
Yes, but This problem is there for long time and I think Bruce's script does not
catch it.
Zdenek
Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes:
>>> During my work I found some header files, which include useless headers
>>> and on other way there are some headers which silently assume that
>>> requested header will be included by some other headers file.
>>
>> Bruce periodically runs a script that's supposed to catch this type
>> of problem. Maybe we are overdue for another run?
> Yes, but This problem is there for long time and I think Bruce's script does not
> catch it.
Exactly what problem do you see that he didn't catch?
regards, tom lane
Tom Lane napsal(a):
> Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes:
>>>> During my work I found some header files, which include useless headers
>>>> and on other way there are some headers which silently assume that
>>>> requested header will be included by some other headers file.
>>> Bruce periodically runs a script that's supposed to catch this type
>>> of problem. Maybe we are overdue for another run?
>
>> Yes, but This problem is there for long time and I think Bruce's script does not
>> catch it.
>
> Exactly what problem do you see that he didn't catch?
bufpage.h includes bufmgr.h, but bufpage.h does not require any definition from
bufmgr.h. I think this dependency is there for long time.
Zdenek
Tom Lane wrote:
> Alvaro Herrera <alvherre@commandprompt.com> writes:
> > Zdenek Kotala wrote:
> >> During my work I found some header files, which include useless headers
> >> and on other way there are some headers which silently assume that
> >> requested header will be included by some other headers file.
> >>
> >> This patch fixes these dependencies around bufpage.h, bufmgr.h
>
> > What's the point here, again?
>
> Bruce periodically runs a script that's supposed to catch this type
> of problem. Maybe we are overdue for another run?
Last run was July, 2006:
revision 1.48
date: 2006/07/13 18:01:01; author: momjian; state: Exp; lines: +1 -2
More include file adjustments.
----------------------------
revision 1.47
date: 2006/07/13 17:47:01; author: momjian; state: Exp; lines: +2 -1
More include file adjustments.
----------------------------
revision 1.46
date: 2006/07/13 16:49:19; author: momjian; state: Exp; lines: +2 -1
Allow include files to compile own their own.
Strip unused include files out unused include files, and add needed
includes to C files.
I run it every few years rather than every year because it can break
stuff that we have to manually fix by adding includes.
If you want it run now or every year, let me know.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Zdenek Kotala wrote: > Tom Lane napsal(a): > > Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes: > >>>> During my work I found some header files, which include useless headers > >>>> and on other way there are some headers which silently assume that > >>>> requested header will be included by some other headers file. > >>> Bruce periodically runs a script that's supposed to catch this type > >>> of problem. Maybe we are overdue for another run? > > > >> Yes, but This problem is there for long time and I think Bruce's script does not > >> catch it. > > > > Exactly what problem do you see that he didn't catch? > > bufpage.h includes bufmgr.h, but bufpage.h does not require any definition from > bufmgr.h. I think this dependency is there for long time. My scripts should have found this issue, see src/tools/pginclude/pgrminclude. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian <bruce@momjian.us> writes:
> Zdenek Kotala wrote:
>> bufpage.h includes bufmgr.h, but bufpage.h does not require any definition from
>> bufmgr.h. I think this dependency is there for long time.
> My scripts should have found this issue, see
> src/tools/pginclude/pgrminclude.
Looking over the file more closely, I disagree with Zdenek's claim
anyway. Even though the file could physically be scanned without
having included bufmgr.h first, many of the macros it defines can't
be used without bufmgr.h, and so that file really is a prerequisite.
If we removed the include here it would just have to pop up in
calling .c files. Anyplace that that was solely because of calling
one of the macros defined by bufpage.h, rather than explicitly using
anything from bufmgr.h, I claim it'd be wrong.
This example makes me question using scripts for this type of
maintenance, though, as it's not clear that a does-it-compile test
would understand macro dependencies.
It's probably true that we've let the header inclusions in storage/
and access/ get a bit spaghetti-ish. But I think that if we're going
to do something about it, focusing on one or two files is not the
way to start. What we need is for someone to go through all those
files and propose a clear layering of them. This will very likely
involve having to move some declarations around, when we realize
something got put in a poorly chosen place.
regards, tom lane
Bruce Momjian <bruce@momjian.us> writes:
> I run it every few years rather than every year because it can break
> stuff that we have to manually fix by adding includes.
> If you want it run now or every year, let me know.
I'm not in a hurry --- as we found out last time, that script isn't
really bright enough. When the state of the headers gets bad enough
that we're willing to accept collateral damage, then it should be
run, but I don't think it's reliable enough to use routinely.
regards, tom lane
Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > I run it every few years rather than every year because it can break > > stuff that we have to manually fix by adding includes. > > > If you want it run now or every year, let me know. > > I'm not in a hurry --- as we found out last time, that script isn't > really bright enough. When the state of the headers gets bad enough > that we're willing to accept collateral damage, then it should be > run, but I don't think it's reliable enough to use routinely. Agreed, that was my analysis too. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian napsal(a):
> Zdenek Kotala wrote:
>> Tom Lane napsal(a):
>>> Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes:
>>>>>> During my work I found some header files, which include useless headers
>>>>>> and on other way there are some headers which silently assume that
>>>>>> requested header will be included by some other headers file.
>>>>> Bruce periodically runs a script that's supposed to catch this type
>>>>> of problem. Maybe we are overdue for another run?
>>>> Yes, but This problem is there for long time and I think Bruce's script does not
>>>> catch it.
>>> Exactly what problem do you see that he didn't catch?
>> bufpage.h includes bufmgr.h, but bufpage.h does not require any definition from
>> bufmgr.h. I think this dependency is there for long time.
>
> My scripts should have found this issue, see
> src/tools/pginclude/pgrminclude.
>
I'm going to try it and examine output.
Zdenek
Bruce Momjian napsal(a):
> Zdenek Kotala wrote:
>> Tom Lane napsal(a):
>>> Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes:
>>>>>> During my work I found some header files, which include useless headers
>>>>>> and on other way there are some headers which silently assume that
>>>>>> requested header will be included by some other headers file.
>>>>> Bruce periodically runs a script that's supposed to catch this type
>>>>> of problem. Maybe we are overdue for another run?
>>>> Yes, but This problem is there for long time and I think Bruce's script does not
>>>> catch it.
>>> Exactly what problem do you see that he didn't catch?
>> bufpage.h includes bufmgr.h, but bufpage.h does not require any definition from
>> bufmgr.h. I think this dependency is there for long time.
>
> My scripts should have found this issue, see
> src/tools/pginclude/pgrminclude.
>
Hmmm. It is difficult to run it on Solaris. I adjusted scripts but I did not get
any change on current head.
Zdenek
Tom Lane napsal(a):
> Bruce Momjian <bruce@momjian.us> writes:
>> Zdenek Kotala wrote:
>>> bufpage.h includes bufmgr.h, but bufpage.h does not require any definition from
>>> bufmgr.h. I think this dependency is there for long time.
>
>> My scripts should have found this issue, see
>> src/tools/pginclude/pgrminclude.
>
> Looking over the file more closely, I disagree with Zdenek's claim
> anyway. Even though the file could physically be scanned without
> having included bufmgr.h first, many of the macros it defines can't
> be used without bufmgr.h, and so that file really is a prerequisite.
> If we removed the include here it would just have to pop up in
> calling .c files. Anyplace that that was solely because of calling
> one of the macros defined by bufpage.h, rather than explicitly using
> anything from bufmgr.h, I claim it'd be wrong.
Yeah, I see it now. BufferGetPageSize and BufferGetPage macros uses macros from
bufmgr. But I think, They should be moved to the bufmgr.h where is better and
more logic place for them.
<snip>
> It's probably true that we've let the header inclusions in storage/
> and access/ get a bit spaghetti-ish. But I think that if we're going
> to do something about it, focusing on one or two files is not the
> way to start. What we need is for someone to go through all those
> files and propose a clear layering of them. This will very likely
> involve having to move some declarations around, when we realize
> something got put in a poorly chosen place.
Yes, agree. I need also fix problem with pgxlogreset and design how to track
history (multi version) of structures related to upgrade (e.g. page layout ...).
I will look on it.
Zdenek
PS: Is there any reason to do not start to use inline functions instead of
macros in some cases?
Zdenek Kotala wrote: > PS: Is there any reason to do not start to use inline functions instead > of macros in some cases? Not all compilers like (== support) inline macros apparently. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera napsal(a):
> Zdenek Kotala wrote:
>
>> PS: Is there any reason to do not start to use inline functions instead
>> of macros in some cases?
>
> Not all compilers like (== support) inline macros apparently.
Is it your assumption or do you mean some specific compiler? IIRC, inline is
defined in C99 and my assumption :-) is that it should be supported by all
compilers today. I try to look on buildmachine, There should be some useful
configure output.
Zdenek
Zdenek Kotala wrote: > Alvaro Herrera napsal(a): >> Zdenek Kotala wrote: >> >>> PS: Is there any reason to do not start to use inline functions >>> instead of macros in some cases? >> >> Not all compilers like (== support) inline macros apparently. > > Is it your assumption or do you mean some specific compiler? IIRC, inline > is defined in C99 and my assumption :-) is that it should be supported by > all compilers today. I try to look on buildmachine, There should be some > useful configure output. We have some defensive coding for compilers that do not support it, for example on pg_list.h ... I have no idea if there's a compiler in actual use that needs this. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Zdenek Kotala wrote: > > Is it your assumption or do you mean some specific compiler? IIRC, > inline is defined in C99 and my assumption :-) is that it should be > supported by all compilers today. I try to look on buildmachine, There > should be some useful configure output. > My recollection is that we support C89, which might be a bit out of date, but then we try not to obsolete platforms if possible. cheers andrew
Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes:
> Alvaro Herrera napsal(a):
>> Not all compilers like (== support) inline macros apparently.
> Is it your assumption or do you mean some specific compiler? IIRC, inline is
> defined in C99 and my assumption :-) is that it should be supported by all
> compilers today.
The problem is (1) not all compilers support inline, (2) the ones that
do have divergent ideas on its semantics, and (3) the semantics
specified by C99 utterly suck (cf tuplesort.c). gcc's traditional
semantics for inline are far more usable. We have done "#ifdef gcc"
inlines in one or two places where the performance argument for adding
such clutter was compelling. We can do that some more, but you'll need
equally compelling arguments. Do not bother submitting patches to
create a very large number of inlines.
regards, tom lane