Обсуждение: [PATCH] Remove unused #include's in src/backend/commands/*
Hi, clangd indicates that certain #include's are redundant. Removing them will speed up the build process a bit. -- Best regards, Aleksander Alekseev
Вложения
> 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
Вложения
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
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
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
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
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
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
Вложения
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
Вложения
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
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
Вложения
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
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)
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
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
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
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)
=?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
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