Remove unnecessary "lmgr.h" in stat_utils.c

On Mon, May 05, 2025 at 04:33:20PM +0300, Ilia Evdokimov wrote:

While reviewing the import/export statistics, I noticed that relation
locking are handled via relation_open() and relation_close() in
stats_lock_check_privileges(), and no calls to other lock-manager routines
are actually used there. As a result, the inclusion of the lock-manager
header

#include "storage/lmgr.h"

is not needed. I have attached a small patch which simply removes that
include.

True that we try to be clean when it comes to that.

I suspect that this is an artifact from one of the original versions
of the patch compared to what has been committed when this feature has
been discussed. Corey?
--
Michael

Hi,

On Wed, May 07, 2025 at 09:10:17AM +0900, Michael Paquier wrote:

On Mon, May 05, 2025 at 04:33:20PM +0300, Ilia Evdokimov wrote:

While reviewing the import/export statistics, I noticed that relation
locking are handled via relation_open() and relation_close() in
stats_lock_check_privileges(), and no calls to other lock-manager routines
are actually used there. As a result, the inclusion of the lock-manager
header

#include "storage/lmgr.h"

is not needed. I have attached a small patch which simply removes that
include.

True that we try to be clean when it comes to that.

Re-sharing my thoughts as it looks like that my previous email did not reach the
mailing list (yet?): probably because the attachement mentioned below was too
large.

"
Thanks for the report!

Indeed, and that's what clang-tidy (misc-include-cleaner) also reports:

$ run-clang-tidy -checks="-*,misc-include-cleaner" | grep "is not used directly" | grep stat_utils.c
../src/backend/statistics/stat_utils.c:26:1: warning: included header lmgr.h is not used directly [misc-include-cleaner]

Actually it reports much more (see attached):

$ run-clang-tidy -checks="-*,misc-include-cleaner" | grep -c "is not used directly"
794

I did not look to check if all of them make sense (I'd guess probably not), but
I'm wondering if it would make sense to work an a "larger" cleanup instead?
(in the same vein as [1]/messages/by-id/af837490-6b2f-46df-ba05-37ea6a6653fc@eisentraut.org did)

[1]: /messages/by-id/af837490-6b2f-46df-ba05-37ea6a6653fc@eisentraut.org

"

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Hi,

On Wed, May 07, 2025 at 07:28:01AM +0000, Bertrand Drouvot wrote:

Hi,

On Wed, May 07, 2025 at 09:10:17AM +0900, Michael Paquier wrote:

On Mon, May 05, 2025 at 04:33:20PM +0300, Ilia Evdokimov wrote:

While reviewing the import/export statistics, I noticed that relation
locking are handled via relation_open() and relation_close() in
stats_lock_check_privileges(), and no calls to other lock-manager routines
are actually used there. As a result, the inclusion of the lock-manager
header

#include "storage/lmgr.h"

is not needed. I have attached a small patch which simply removes that
include.

True that we try to be clean when it comes to that.

Re-sharing my thoughts as it looks like that my previous email did not reach the
mailing list (yet?): probably because the attachement mentioned below was too
large.

"
Thanks for the report!

Indeed, and that's what clang-tidy (misc-include-cleaner) also reports:

$ run-clang-tidy -checks="-*,misc-include-cleaner" | grep "is not used directly" | grep stat_utils.c
../src/backend/statistics/stat_utils.c:26:1: warning: included header lmgr.h is not used directly [misc-include-cleaner]

Actually it reports much more (see attached):

$ run-clang-tidy -checks="-*,misc-include-cleaner" | grep -c "is not used directly"
794

I did not look to check if all of them make sense (I'd guess probably not), but
I'm wondering if it would make sense to work an a "larger" cleanup instead?
(in the same vein as [1] did)

[1]: /messages/by-id/af837490-6b2f-46df-ba05-37ea6a6653fc@eisentraut.org

"

FWIW, please find attached a subset of the initial report that focus on
the "is not used directly" warnings.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Wed, May 07, 2025 at 05:56:55PM +0000, Bertrand Drouvot wrote:

FWIW, please find attached a subset of the initial report that focus on
the "is not used directly" warnings.

Seems worth doing to simplify the code in the long-run, even if these
require a case-by-case manual lookup. Did you cross-check with the
information that IWYU generates? Do we have some consistency?

All that is potential work for v19, of course.
--
Michael

On 2025-May-08, Michael Paquier wrote:

Seems worth doing to simplify the code in the long-run, even if these
require a case-by-case manual lookup.

Yeah, I don't think we can take a patch that simply removes everything
that this report mentions, because some of the includes (especially the
system includes) could be platform-dependent. Also I imagine some of
them depend on compile-time environment -- e.g., I'm pretty sure
src/port/pg_popcount_aarch64.c is not going to work well without
pg_bitutils.h.

All that is potential work for v19, of course.

Yep.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Update: super-fast reaction on the Postgres bugs mailing list. The report
was acknowledged [...], and a fix is under discussion.
The wonders of open-source !"
https://twitter.com/gunnarmorling/status/1596080409259003906