Subject: Avoid circular header file dependency

From: Bertrand Drouvot <[email-masked]>
Date: Sat, 26 Apr 2025 03:20:09 +0000

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
>From e001863ab8645b5b6c2021c857ac541e230398dd Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <[email-masked]>
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

---

From: Tom Lane <[email-masked]>
Date: Sat, 26 Apr 2025 01:20:56 -0400

> header file dependency (introduced in fa88928470b5) between wait_event.h and

Ugh.  I still carry the scars of cleaning up after a previous
circular-inclusion mess (cf 1609797c2), so I'm always worried about
introducing new cases.  I don't have an opinion about whether this
specific refactoring is the best way to deal with this case, but
I definitely feel that we mustn't allow the situation to persist.

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

Whatever it contains, we need to kill it with fire before the problem
metastasizes like it did the last time.  (yeah, yeah, badly mixed
metaphors)  I can take a look at this one over the weekend if nobody
beats me to it.

I am very glad to hear that there's a readily available tool to
catch such cases.  We ought to run it every so often.

			regards, tom lane

---

From: Michael Paquier <[email-masked]>
Date: Sat, 26 Apr 2025 16:42:56 +0900

> I definitely feel that we mustn't allow the situation to persist.

This one is my fault, so I'll take care of it.  Splitting the values
of the wait classes into their own header makes sense, but the header
name wait_class_constants.h sounds a bit off.  Why not a simpler
"wait_classes.h" that gets included by wait_event.h and
wait_event_types.h?
--
Michael

---

From: Bertrand Drouvot <[email-masked]>
Date: Sat, 26 Apr 2025 08:11:50 +0000

Hi,

On Sat, Apr 26, 2025 at 01:20:56AM -0400, Tom Lane wrote:
> I don't have an opinion about whether this

Thanks for sharing your thoughts! yeah, I'm not 100% sure that the proposed
refactoring is the best way but it looks simple enough and allows
wait_event_types.h to include "only" what it really needs.

> metaphors)  I can take a look at this one over the weekend if nobody

I had a look at it, what do you think about 0002 attached? (Not 100% sure
that's the best approach though).

> catch such cases.

+1, and it's good to see that there are only 2 cases in the code base.

> We ought to run it every so often.

Yeah, also probably with readability-inconsistent-declaration-parameter-name that
Peter used in [1].

How/where do we "automate" this kind of regular tasks? (I tried to find the answer
in [2] but that thread is pretty long). 

[1]: https://www.postgresql.org/message-id/CAH2-WznJt9CMM9KJTMjJh_zbL5hD9oX44qdJ4aqZtjFi-zA3Tg%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/flat/20200812223409.6di3y2qsnvynao7a%40alap3.anarazel.de

Regards,

>From 2156bf212ce7aabc40e2843c77d796249d12d4f9 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <[email-masked]>
Date: Thu, 24 Apr 2025 17:11:03 +0000
Subject: [PATCH v1 1/2] Avoid including wait_event.h in wait_event_types.h

 

-
-/* ----------
- * Wait Classes
- * ----------
- */

>From 51ddf8b036dd167a29c56632b8855d27fd361838 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <[email-masked]>
Date: Sat, 26 Apr 2025 07:14:37 +0000
Subject: [PATCH v1 2/2] Avoid including plpython.h in plpy_util.h

Adding plpython_base.h to avoid including plpython.h in plpy_util.h (that
produced a circular include).
---
 src/pl/plpython/plpy_util.h     |  2 +-
 src/pl/plpython/plpython.h      | 26 +--------------------
 src/pl/plpython/plpython_base.h | 41 +++++++++++++++++++++++++++++++++
 3 files changed, 43 insertions(+), 26 deletions(-)
 create mode 100644 src/pl/plpython/plpython_base.h

diff --git a/src/pl/plpython/plpy_util.h b/src/pl/plpython/plpy_util.h
index 6f491b0f95b..797692a9eec 100644
--- a/src/pl/plpython/plpy_util.h
+++ b/src/pl/plpython/plpy_util.h
@@ -6,7 +6,7 @@
 #ifndef PLPY_UTIL_H
 #define PLPY_UTIL_H
 
-#include "plpython.h"
+#include "plpython_base.h"
 
 extern PGDLLEXPORT PyObject *PLyUnicode_Bytes(PyObject *unicode);
 extern PGDLLEXPORT char *PLyUnicode_AsString(PyObject *unicode);
diff --git a/src/pl/plpython/plpython.h b/src/pl/plpython/plpython.h
index 06fc1a5440f..fddcc96acdc 100644
--- a/src/pl/plpython/plpython.h
+++ b/src/pl/plpython/plpython.h
@@ -12,31 +12,7 @@
 #ifndef PLPYTHON_H
 #define PLPYTHON_H
 
-/* postgres.h needs to be included before Python.h, as usual */
-#if !defined(POSTGRES_H)
-#error postgres.h must be included before plpython.h
-#elif defined(Py_PYTHON_H)
-#error Python.h must be included via plpython.h
-#endif
-
-/*
- * Enable Python Limited API
- *
- * XXX currently not enabled on MSVC because of build failures
- */
-#if !defined(_MSC_VER)
-#define Py_LIMITED_API 0x03020000
-#endif
-
-/*
- * Pull in Python headers via a wrapper header, to control the scope of
- * the system_header pragma therein.
- */
-#include "plpython_system.h"
-
-/* define our text domain for translations */
-#undef TEXTDOMAIN
-#define TEXTDOMAIN PG_TEXTDOMAIN("plpython")
+#include "plpython_base.h"
 
 /*
  * Used throughout, so it's easier to just include it everywhere.
diff --git a/src/pl/plpython/plpython_base.h b/src/pl/plpython/plpython_base.h
new file mode 100644
index 00000000000..7abbcb8b233
--- /dev/null
+++ b/src/pl/plpython/plpython_base.h
@@ -0,0 +1,41 @@
+/*-------------------------------------------------------------------------
+ *
+ * plpython_base.h - Python as a procedural language for PostgreSQL
+ *
+ * Portions Copyright (c) 1996-2025, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/pl/plpython/plpython_base.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef PLPYTHON_BASE_H
+#define PLPYTHON_BASE_H
+
+/* postgres.h needs to be included before Python.h, as usual */
+#if !defined(POSTGRES_H)
+#error postgres.h must be included before plpython_base.h
+#elif defined(Py_PYTHON_H)
+#error Python.h must be included via plpython_base.h
+#endif
+
+/*
+ * Enable Python Limited API
+ *
+ * XXX currently not enabled on MSVC because of build failures
+ */
+#if !defined(_MSC_VER)
+#define Py_LIMITED_API 0x03020000
+#endif
+
+/*
+ * Pull in Python headers via a wrapper header, to control the scope of
+ * the system_header pragma therein.
+ */
+#include "plpython_system.h"
+
+/* define our text domain for translations */
+#undef TEXTDOMAIN
+#define TEXTDOMAIN PG_TEXTDOMAIN("plpython")
+
+#endif							/* PLPYTHON_BASE_H */
-- 
2.34.1