Date: Sat, 26 Apr 2025 08:11:50 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Avoid circular header file dependency
Message-ID: <aAyVRuECq1JdiAlf@ip-10-97-1-34.eu-west-3.compute.internal>
References: <aAxQ6fcY5QQV1lo3@ip-10-97-1-34.eu-west-3.compute.internal>
 <2238517.1745644856@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="I+6O/PSKcvnJQ048"
Content-Disposition: inline
In-Reply-To: <2238517.1745644856@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/aAyVRuECq1JdiAlf%40ip-10-97-1-34.eu-west-3.compute.internal>
Precedence: bulk


--I+6O/PSKcvnJQ048
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Hi,

On Sat, Apr 26, 2025 at 01:20:56AM -0400, Tom Lane wrote:
> Bertrand Drouvot <bertranddrouvot.pg@gmail.com> writes:
> > 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.
> 
> 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.

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.

> > 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.
> 
> 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 had a look at it, what do you think about 0002 attached? (Not 100% sure
that's the best approach though).

> I am very glad to hear that there's a readily available tool to
> 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,

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

--I+6O/PSKcvnJQ048
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 2156bf212ce7aabc40e2843c77d796249d12d4f9 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 1/2] 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


--I+6O/PSKcvnJQ048
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v1-0002-Avoid-including-plpython.h-in-plpy_util.h.patch"

From 51ddf8b036dd167a29c56632b8855d27fd361838 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
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


--I+6O/PSKcvnJQ048--