On Wed, Jul 31, 2019 at 12:26 PM Andres Freund <and...@anarazel.de> wrote: > > Hi, > > On 2019-07-31 11:19:08 +0530, vignesh C wrote: > > I noticed that there are many header files being > > included which need not be included. > > I have tried this in a few files and found the > > compilation and regression to be working. > > I have attached the patch for the files that > > I tried. > > I tried this in CentOS, I did not find the header > > files to be platform specific. > > Should we pursue this further and cleanup in > > all the files? > > These type of changes imo have a good chance of making things more > fragile. A lot of the includes in header files are purely due to needing > one or two definitions (which often could even be avoided by forward > declarations). If you remove all the includes directly from the c files > that are also included from some .h file, you increase the reliance on > the indirect includes - making it harder to clean up. > > If anything, we should *increase* the number of includes, so we don't > rely on indirect includes. But that's also not necessarily the right > choice, because it adds unnecessary dependencies. > > > > --- a/src/backend/utils/mmgr/freepage.c > > +++ b/src/backend/utils/mmgr/freepage.c > > @@ -56,7 +56,6 @@ > > #include "miscadmin.h" > > > > #include "utils/freepage.h" > > -#include "utils/relptr.h" > > I don't think it's a good idea to remove this header, for example. A > *lot* of code in freepage.c relies on it. The fact that freepage.h also > includes it here is just due to needing other parts of it > Fixed this. > > > /* Magic numbers to identify various page types */ > > diff --git a/src/backend/utils/mmgr/portalmem.c > > b/src/backend/utils/mmgr/portalmem.c > > index 334e35b..67268fd 100644 > > --- a/src/backend/utils/mmgr/portalmem.c > > +++ b/src/backend/utils/mmgr/portalmem.c > > @@ -26,7 +26,6 @@ > > #include "utils/builtins.h" > > #include "utils/memutils.h" > > #include "utils/snapmgr.h" > > -#include "utils/timestamp.h" > > Similarly, this file uses timestamp.h functions directly. The fact that > timestamp.h already is included is just due to implementation details of > xact.h that this file shouldn't depend on. > Fixed this.
Made the fixes based on your comments, updated patch has the changes for the same. Regards, Vignesh EnterpriseDB: http://www.enterprisedb.com
v2-0001-Unused-header-file-removal.patch
Description: Binary data
