Horton

Thread

  1. [PATCH] Remove unused #include's in src/backend/commands/*

    Aleksander Alekseev <aleksander@tigerdata.com> — 2025-10-08T13:07:06Z

    Hi,

clangd indicates that certain #include's are redundant. Removing them
will speed up the build process a bit.

-- 
Best regards,
Aleksander Alekseev

  2. Re: [PATCH] Remove unused #include's in src/backend/commands/*

    Aleksander Alekseev <aleksander@tigerdata.com> — 2025-10-08T14:30:58Z

    > clangd indicates that certain #include's are redundant. Removing them
> will speed up the build process a bit.

OK, cfbot tells me that touching collationcmds.c was a mistake. Here
is a corrected patch.

-- 
Best regards,
Aleksander Alekseev

  3. Re: [PATCH] Remove unused #include's in src/backend/commands/*

    Nathan Bossart <nathandbossart@gmail.com> — 2025-10-08T16:50:34Z

    On Wed, Oct 08, 2025 at 05:30:58PM +0300, Aleksander Alekseev wrote:
>> clangd indicates that certain #include's are redundant. Removing them
>> will speed up the build process a bit.
> 
> OK, cfbot tells me that touching collationcmds.c was a mistake. Here
> is a corrected patch.

Seems reasonable to me.  Most of these seem to be recent additions since
last year's round of IWYU commits.  Any reason to limit this to
src/backend/commands?  Why not tackle the whole tree at once?

-- 
nathan

  4. Re: [PATCH] Remove unused #include's in src/backend/commands/*

    Aleksander Alekseev <aleksander@tigerdata.com> — 2025-10-08T18:41:30Z

    Hi Nathan,

> Seems reasonable to me.  Most of these seem to be recent additions since
> last year's round of IWYU commits.  Any reason to limit this to
> src/backend/commands?  Why not tackle the whole tree at once?

The main problem here is that clangd is a language server, which means
I see errors reported by it when I open a particular file in a text
editor. To go manually over ~2500 .c/.h files we have and then see
what will break on CI because of an #ifdef (as it was with the patch
v1) doesn't strike me as a great idea :)

In theory there is a way to get clangd warnings for a given file using
a command like this:

# use meson; make sure build/compile_commands.json exists
path/to/bin/clangd --enable-config \
  --log=error \
  --compile-commands-dir=build \
  --check="src/backend/commands/collationcmds.c"

... which can be executed in a loop for all the existing files.
Unfortunately it doesn't work. The command outputs multiple errors,
something about:

```
FAIL: The new replacement overlaps with an existing replacement.
```

¯\_(ツ)_/¯  So far I haven't figured out how to make it work.

Alternatively we could prioritize ~100 .c files which can be checked
manually. Or we can make changes iteratively, as with patches v1/v2.

Personally I prefer multiple small changes to a single but huge one to
be honest.

-- 
Best regards,
Aleksander Alekseev

  5. Re: [PATCH] Remove unused #include's in src/backend/commands/*

    Nathan Bossart <nathandbossart@gmail.com> — 2025-10-08T21:31:13Z

    On Wed, Oct 08, 2025 at 09:41:30PM +0300, Aleksander Alekseev wrote:
> The main problem here is that clangd is a language server, which means
> I see errors reported by it when I open a particular file in a text
> editor. To go manually over ~2500 .c/.h files we have and then see
> what will break on CI because of an #ifdef (as it was with the patch
> v1) doesn't strike me as a great idea :)

Why not use IWYU as recommended in src/tools/pginclude/README?  I gave that
a try and it didn't take very long for src/.

> Alternatively we could prioritize ~100 .c files which can be checked
> manually. Or we can make changes iteratively, as with patches v1/v2.

Excluding system headers, I'm seeing 390 suggestions for all C files in
src/, some of which I'd probably skip (e.g., snowball).  That doesn't seem
too bad to me.

-- 
nathan

  6. Re: [PATCH] Remove unused #include's in src/backend/commands/*

    Andres Freund <andres@anarazel.de> — 2025-10-08T21:38:26Z

    Hi,

On 2025-10-08 16:31:13 -0500, Nathan Bossart wrote:
> On Wed, Oct 08, 2025 at 09:41:30PM +0300, Aleksander Alekseev wrote:
> > The main problem here is that clangd is a language server, which means
> > I see errors reported by it when I open a particular file in a text
> > editor. To go manually over ~2500 .c/.h files we have and then see
> > what will break on CI because of an #ifdef (as it was with the patch
> > v1) doesn't strike me as a great idea :)
> 
> Why not use IWYU as recommended in src/tools/pginclude/README?  I gave that
> a try and it didn't take very long for src/.

I've not rechecked today, but the last time I did, iwyu needed a lot of adult
supervision with the current amount of annotations. If we did all the
necessary annotations and caught up with all the things it suggests it should
be easier, but as-is it's imo just usable as an idea-giver for manually
written patches.

Greetings,

Andres Freund

  7. Re: [PATCH] Remove unused #include's in src/backend/commands/*

    Aleksander Alekseev <aleksander@tigerdata.com> — 2025-10-09T10:21:57Z

    Hi,

> > Why not use IWYU as recommended in src/tools/pginclude/README?  I gave that
> > a try and it didn't take very long for src/.
>
> I've not rechecked today, but the last time I did, iwyu needed a lot of adult
> supervision with the current amount of annotations. If we did all the
> necessary annotations and caught up with all the things it suggests it should
> be easier, but as-is it's imo just usable as an idea-giver for manually
> written patches.

OK, I tried include-what-you-use with a little post-processing script
and the scope of work seems reasonable. I will submit the updated
patch shortly.

-- 
Best regards,
Aleksander Alekseev

  8. Re: [PATCH] Remove unused #include's in src/backend/commands/*

    Aleksander Alekseev <aleksander@tigerdata.com> — 2025-10-09T12:29:14Z

    Hi,

> OK, I tried include-what-you-use with a little post-processing script
> and the scope of work seems reasonable. I will submit the updated
> patch shortly.

Here is the updated patch v3.

-- 
Best regards,
Aleksander Alekseev

  9. Re: [PATCH] Remove unused #include's in src/backend/commands/*

    Aleksander Alekseev <aleksander@tigerdata.com> — 2025-10-09T13:08:38Z

    Hi,

> > OK, I tried include-what-you-use with a little post-processing script
> > and the scope of work seems reasonable. I will submit the updated
> > patch shortly.
>
> Here is the updated patch v3.

Apparently touching collationcmds.c, restricted_token.c and
fe-auth-oauth.c was a mistake. Here is the corrected patch v4.

-- 
Best regards,
Aleksander Alekseev

  10. Re: [PATCH] Remove unused #include's in src/backend/commands/*

    Andres Freund <andres@anarazel.de> — 2025-10-09T13:18:06Z

    Hi,

On 2025-10-09 16:08:38 +0300, Aleksander Alekseev wrote:
> > > OK, I tried include-what-you-use with a little post-processing script
> > > and the scope of work seems reasonable. I will submit the updated
> > > patch shortly.
> >
> > Here is the updated patch v3.
> 
> Apparently touching collationcmds.c, restricted_token.c and
> fe-auth-oauth.c was a mistake. Here is the corrected patch v4.

Clearly this approach is not bulletproof enough to actually yield something
reliable enough to actually be applied.


> From 81089fdf47561a4a68f80ebe238e8d5a452f7c0b Mon Sep 17 00:00:00 2001
> From: Aleksander Alekseev <aleksander@tigerdata.com>
> Date: Wed, 8 Oct 2025 15:58:38 +0300
> Subject: [PATCH v4] Remove unused #include's
> 
> Author: Aleksander Alekseev <aleksander@tigerdata.com>
> Reviewed-by: Nathan Bossart <nathandbossart@gmail.com>
> Reviewed-by: Andres Freund <andres@anarazel.de>

I've not reviewed this.

Greetings,

Andres Freund

  11. Re: [PATCH] Remove unused #include's in src/backend/commands/*

    Aleksander Alekseev <aleksander@tigerdata.com> — 2025-10-09T13:26:20Z

    Hi Andres,

> > Apparently touching collationcmds.c, restricted_token.c and
> > fe-auth-oauth.c was a mistake. Here is the corrected patch v4.
>
> Clearly this approach is not bulletproof enough to actually yield something
> reliable enough to actually be applied.

I'm open to suggestions. I only have three machines to test my patches
(Linux, MacOS and Windows, and I've just discovered that the master
branch doesn't compile on Windows 11 at the moment).

> > Reviewed-by: Andres Freund <andres@anarazel.de>
>
> I've not reviewed this.

Apologies. Fixed.

-- 
Best regards,
Aleksander Alekseev

  12. Re: [PATCH] Remove unused #include's in src/backend/commands/*

    Andres Freund <andres@anarazel.de> — 2025-10-09T13:36:45Z

    Hi,

On 2025-10-09 16:26:20 +0300, Aleksander Alekseev wrote:
> > > Apparently touching collationcmds.c, restricted_token.c and
> > > fe-auth-oauth.c was a mistake. Here is the corrected patch v4.
> >
> > Clearly this approach is not bulletproof enough to actually yield something
> > reliable enough to actually be applied.
> 
> I'm open to suggestions. I only have three machines to test my patches
> (Linux, MacOS and Windows, and I've just discovered that the master
> branch doesn't compile on Windows 11 at the moment).

Manually analyze each of the removed includes by hand. Fix all the issues that
lead to iwyu giving you wrong suggestions. Test it with CI in your own repo.

Or just don't do it.

Greetings,

Andres Freund

  13. Re: [PATCH] Remove unused #include's in src/backend/commands/*

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-10-09T13:47:20Z

    On 2025-Oct-09, Aleksander Alekseev wrote:

> diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.c
> index bb7d90aa5d9..0a8d621a373 100644
> --- a/src/backend/backup/basebackup.c
> +++ b/src/backend/backup/basebackup.c
> @@ -36,7 +36,6 @@
>  #include "postmaster/syslogger.h"
>  #include "postmaster/walsummarizer.h"
>  #include "replication/slot.h"
> -#include "replication/walsender.h"
>  #include "replication/walsender_private.h"
>  #include "storage/bufpage.h"
>  #include "storage/checksum.h"

I wonder how many of these changes pass the compilation only because the
header you're removing is indirectly being included via another header.
In this particular case, it's because slot.h includes walreceiver.h
which includes walsender.h, so this removal has no effect.

Maybe play with the script here
https://postgr.es/m/202510021240.ptc2zl5cvwen@alvherre.pgsql
to see if your changes would have any effect.  The inclusion graphs in
https://doxygen.postgresql.org may also be useful.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"La virtud es el justo medio entre dos defectos" (Aristóteles)

  14. Re: [PATCH] Remove unused #include's in src/backend/commands/*

    Aleksander Alekseev <aleksander@tigerdata.com> — 2025-10-09T14:04:16Z

    Hi Andres,

> Manually analyze each of the removed includes by hand. Fix all the issues that
> lead to iwyu giving you wrong suggestions.

Well, I did, and played it as safe as I could in fact. Unfortunately
given the amount of (build systems * supported hardware * supported
operating systems * OS versions * build flags * #ifdefs in the code *
compilers) this is a difficult problem to reason about, for both
people and IWYU.

> Test it with CI in your own repo.

I see your point. I thought I tested the patch good enough locally to
make it pass on cfbot the first time, but clearly I was wrong. If
cfbot will complain again I'll continue experimenting with the patch
in my own repository.

Thanks for your feedback and apologies for the noise on the mailing
list. It was not intended.

-- 
Best regards,
Aleksander Alekseev

  15. Re: [PATCH] Remove unused #include's in src/backend/commands/*

    Nathan Bossart <nathandbossart@gmail.com> — 2025-10-09T15:07:03Z

    On Thu, Oct 09, 2025 at 05:04:16PM +0300, Aleksander Alekseev wrote:
> Thanks for your feedback and apologies for the noise on the mailing
> list. It was not intended.

I wasn't aware of these IWYU pitfalls, sorry.  In any case, the reason I
pushed for doing one remove-#includes commit for the whole tree is because
I don't want to start a trend that results in hundreds of commits.  Perhaps
this should be treated more like the other cleanup we do prior to creating
a new stable branch.  I'm dubious it's worth doing more often than once per
year, if at all.

-- 
nathan

  16. Re: [PATCH] Remove unused #include's in src/backend/commands/*

    Aleksander Alekseev <aleksander@tigerdata.com> — 2025-10-09T15:08:39Z

    Hi Álvaro,

> I wonder how many of these changes pass the compilation only because the
> header you're removing is indirectly being included via another header.
> In this particular case, it's because slot.h includes walreceiver.h
> which includes walsender.h, so this removal has no effect.

Hm.... on the flip side if the file is already included what's the
point in the second #include? It doesn't do anything and is redundant,
isn't it? At least my text editor highlights it as such (because
clangd tells it so). This is not a huge problem of course, just a bit
distracting.

If the idea is to always have an explicit list of all the includes (no
indirect ones) I think we may end up with pretty long lists, and I
don't instantly see value in this to be honest.

-- 
Best regards,
Aleksander Alekseev

  17. Re: [PATCH] Remove unused #include's in src/backend/commands/*

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-10-09T15:17:15Z

    On 2025-Oct-09, Aleksander Alekseev wrote:

> If the idea is to always have an explicit list of all the includes (no
> indirect ones) I think we may end up with pretty long lists, and I
> don't instantly see value in this to be honest.

It isn't, but the fact that the source is not actually improved in any
way makes the change irrelevant.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"El Maquinismo fue proscrito so pena de cosquilleo hasta la muerte"
(Ijon Tichy en Viajes, Stanislaw Lem)

  18. Re: [PATCH] Remove unused #include's in src/backend/commands/*

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-09T15:44:46Z

    =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de> writes:
> On 2025-Oct-09, Aleksander Alekseev wrote:
>> If the idea is to always have an explicit list of all the includes (no
>> indirect ones) I think we may end up with pretty long lists, and I
>> don't instantly see value in this to be honest.

> It isn't, but the fact that the source is not actually improved in any
> way makes the change irrelevant.

Maybe I shouldn't put words in Álvaro's mouth, but I think the reason
you're getting so much pushback from senior hackers is that we
remember some very bad experiences with automated #include-removal
years ago --- particularly 1609797c2, which turned me for one off the
idea altogether.  The currently available tools are probably smarter
than what we were using back then, but they're evidently still not
perfect, and we're leery of having to undo work.

I think the issues you're hitting right now may stem from not
compiling with all available options.  Notably, if you don't have
--enable-cassert turned on, there's a pretty fair amount of code
you might be failing to account for.

			regards, tom lane

  19. Re: [PATCH] Remove unused #include's in src/backend/commands/*

    Aleksander Alekseev <aleksander@tigerdata.com> — 2025-10-15T10:40:34Z

    Hi Tom,

> Maybe I shouldn't put words in Álvaro's mouth, but I think the reason
> you're getting so much pushback from senior hackers is that we
> remember some very bad experiences with automated #include-removal
> years ago --- particularly 1609797c2, which turned me for one off the
> idea altogether.  The currently available tools are probably smarter
> than what we were using back then, but they're evidently still not
> perfect, and we're leery of having to undo work.
>
> I think the issues you're hitting right now may stem from not
> compiling with all available options.  Notably, if you don't have
> --enable-cassert turned on, there's a pretty fair amount of code
> you might be failing to account for.

I did test --enable-cassert and some other options I typically enable,
but I definitely didn't test all of them.

cfbot is happy with the patch right now but given the controversy
around it I guess we better invest our time into something else.
Withdrawn.

-- 
Best regards,
Aleksander Alekseev