On Wed, Jul 31, 2019 at 12:26 PM Andres Freund <andres@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