Date: Sat, 26 Apr 2025 03:20:09 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Avoid circular header file dependency
Message-ID: <aAxQ6fcY5QQV1lo3@ip-10-97-1-34.eu-west-3.compute.internal>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="6mFmyaUyFobjuDOz"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/aAxQ6fcY5QQV1lo3%40ip-10-97-1-34.eu-west-3.compute.internal>
Precedence: bulk


--6mFmyaUyFobjuDOz
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

Hi hackers,

While working on wait events I faced some compilation issues due to circular
header file dependency (introduced in fa88928470b5) between wait_event.h and
wait_event_types.h. Those files have include guards but could still lead to
compilation issues in some cases due to the circular dependency. 

Currently, on HEAD, this doesn't cause any issues but I think it's better to
avoid circular header file dependencies (harder to maintain and understand).

Please find attached a patch to $SUBJECT between those 2 header files: it
extracts (in a new header file) from wait_event.h what is strictly needed in
wait_event_types.h.

Out of curiosity, I ran clang-tidy with misc-header-include-cycle ([1]) and it
also reports:

../src/pl/plpython/plpy_util.h:9:10: warning: circular header file dependency detected while including 'plpython.h'

This one worries me less because plpy_util.h only contains simple external 
function declarations.

I was surprised that clang-tidy does report only the plpy_util.h one (in addition
to the wait_event_types.h one) so I wondered if misc-header-include-cycle was
failing to discover circular dependencies in nested cases.

I did a quick test with: a.h → c.h → b.h → d.h → a.h and it found it:

"
../d.h:6:10: warning: circular header file dependency detected while including 'a.h'
    6 | #include "a.h"
      |          ^
../b.h:6:10: note: 'd.h' included from here
    6 | #include "d.h"
      |          ^
../c.h:6:10: note: 'b.h' included from here
    6 | #include "b.h"
      |          ^
../a.h:6:10: note: 'c.h' included from here
    6 | #include "c.h"
      |          ^
../main.c:2:10: note: 'a.h' included from here
    2 | #include "a.h"
"

So it looks like that our code base only contains those 2: plpy_util.h and
wait_event_types.h cases.

Thoughts?

[1]: https://clang.llvm.org/extra/clang-tidy/checks/misc/header-include-cycle.html

Regards,

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

--6mFmyaUyFobjuDOz
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v1-0001-Avoid-including-wait_event.h-in-wait_event_types..patch"

From e001863ab8645b5b6c2021c857ac541e230398dd Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Thu, 24 Apr 2025 17:11:03 +0000
Subject: [PATCH v1] Avoid including wait_event.h in wait_event_types.h

Adding wait_class_constants.h to avoid including wait_event.h in
wait_event_types.h (that produced a circular include).
---
 .../activity/generate-wait_event_types.pl     |  2 +-
 src/include/utils/wait_class_constants.h      | 29 +++++++++++++++++++
 src/include/utils/wait_event.h                | 17 ++---------
 3 files changed, 32 insertions(+), 16 deletions(-)
 create mode 100644 src/include/utils/wait_class_constants.h

diff --git a/src/backend/utils/activity/generate-wait_event_types.pl b/src/backend/utils/activity/generate-wait_event_types.pl
index 171bf2ae632..52b6b7b2ae6 100644
--- a/src/backend/utils/activity/generate-wait_event_types.pl
+++ b/src/backend/utils/activity/generate-wait_event_types.pl
@@ -168,7 +168,7 @@ if ($gen_code)
 	printf $h $header_comment, 'wait_event_types.h';
 	printf $h "#ifndef WAIT_EVENT_TYPES_H\n";
 	printf $h "#define WAIT_EVENT_TYPES_H\n\n";
-	printf $h "#include \"utils/wait_event.h\"\n\n";
+	printf $h "#include \"utils/wait_class_constants.h\"\n\n";
 
 	printf $c $header_comment, 'pgstat_wait_event.c';
 
diff --git a/src/include/utils/wait_class_constants.h b/src/include/utils/wait_class_constants.h
new file mode 100644
index 00000000000..286f9c2daec
--- /dev/null
+++ b/src/include/utils/wait_class_constants.h
@@ -0,0 +1,29 @@
+/*-------------------------------------------------------------------------
+ * wait_class_constants.h
+ *	  Constants related to wait classes
+ *
+ * Copyright (c) 2001-2025, PostgreSQL Global Development Group
+ *
+ * src/include/utils/wait_class_constants.h
+ * ----------
+ */
+#ifndef WAIT_CLASS_CONSTANTS_H
+#define WAIT_CLASS_CONSTANTS_H
+
+
+/* ----------
+ * Wait Classes
+ * ----------
+ */
+#define PG_WAIT_LWLOCK				0x01000000U
+#define PG_WAIT_LOCK				0x03000000U
+#define PG_WAIT_BUFFERPIN			0x04000000U
+#define PG_WAIT_ACTIVITY			0x05000000U
+#define PG_WAIT_CLIENT				0x06000000U
+#define PG_WAIT_EXTENSION			0x07000000U
+#define PG_WAIT_IPC					0x08000000U
+#define PG_WAIT_TIMEOUT				0x09000000U
+#define PG_WAIT_IO					0x0A000000U
+#define PG_WAIT_INJECTIONPOINT		0x0B000000U
+
+#endif							/* WAIT_CLASS_CONSTANTS_H */
diff --git a/src/include/utils/wait_event.h b/src/include/utils/wait_event.h
index b8cb3e5a430..c3c1ce6ef2e 100644
--- a/src/include/utils/wait_event.h
+++ b/src/include/utils/wait_event.h
@@ -10,21 +10,8 @@
 #ifndef WAIT_EVENT_H
 #define WAIT_EVENT_H
 
-
-/* ----------
- * Wait Classes
- * ----------
- */
-#define PG_WAIT_LWLOCK				0x01000000U
-#define PG_WAIT_LOCK				0x03000000U
-#define PG_WAIT_BUFFERPIN			0x04000000U
-#define PG_WAIT_ACTIVITY			0x05000000U
-#define PG_WAIT_CLIENT				0x06000000U
-#define PG_WAIT_EXTENSION			0x07000000U
-#define PG_WAIT_IPC					0x08000000U
-#define PG_WAIT_TIMEOUT				0x09000000U
-#define PG_WAIT_IO					0x0A000000U
-#define PG_WAIT_INJECTIONPOINT		0x0B000000U
+/* wait class constants */
+#include "utils/wait_class_constants.h"
 
 /* enums for wait events */
 #include "utils/wait_event_types.h"
-- 
2.34.1


--6mFmyaUyFobjuDOz--