Re: Unused header file inclusion

From: Andres Freund <andres(at)anarazel(dot)de>
To: vignesh C <vignesh21(at)gmail(dot)com>
Cc: Pg Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Unused header file inclusion
Date: 2019-07-31 06:56:07
Message-ID: 20190731065607.he5335ubyxxfmibt@alap3.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

> /* 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.

Greetings,

Andres Freund

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Masahiko Sawada 2019-07-31 07:11:03 Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)
Previous Message Michael Paquier 2019-07-31 06:37:49 Re: Unused header file inclusion