Thread: Avoid circular header file dependency

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

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.

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.

> 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 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

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.
>
> 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 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

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

Hi,

On Sat, Apr 26, 2025 at 04:42:56PM +0900, Michael Paquier wrote:
> This one is my fault,

I do feel guilty too...

> 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?

Yeah, better. Done that way in the attached.

Regards,

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

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

After looking at this, I think the actual problem is that plpython.h
does this:

/*
 * Used throughout, so it's easier to just include it everywhere.
 */
#include "plpy_util.h"

which is exactly the sort of cowboy modularity violation that hurts
when you have to clean it up.  Taking that out requires having to
manually include plpy_util.h in all the relevant .c files, but on
the other hand we can remove their vestigial direct inclusions of
plpython.h.  It was always pretty silly to #include that after
including some plpy_foo.h files, so let's stop doing so.  The attached
patch therefore boils down in most places to s/plpython.h/plpy_util.h/.

(A small number of these files still compiled without that, indicating
that they're not actually using plpy_util.h today.  But I figured we
might as well just do it uniformly.)

            regards, tom lane

diff --git a/contrib/hstore_plpython/hstore_plpython.c b/contrib/hstore_plpython/hstore_plpython.c
index 8812fb3f3e4..e2bfc6da38e 100644
--- a/contrib/hstore_plpython/hstore_plpython.c
+++ b/contrib/hstore_plpython/hstore_plpython.c
@@ -3,7 +3,7 @@
 #include "fmgr.h"
 #include "hstore/hstore.h"
 #include "plpy_typeio.h"
-#include "plpython.h"
+#include "plpy_util.h"

 PG_MODULE_MAGIC_EXT(
                     .name = "hstore_plpython",
diff --git a/contrib/jsonb_plpython/jsonb_plpython.c b/contrib/jsonb_plpython/jsonb_plpython.c
index 680445a006f..9383615abbf 100644
--- a/contrib/jsonb_plpython/jsonb_plpython.c
+++ b/contrib/jsonb_plpython/jsonb_plpython.c
@@ -2,7 +2,7 @@

 #include "plpy_elog.h"
 #include "plpy_typeio.h"
-#include "plpython.h"
+#include "plpy_util.h"
 #include "utils/fmgrprotos.h"
 #include "utils/jsonb.h"
 #include "utils/numeric.h"
diff --git a/contrib/ltree_plpython/ltree_plpython.c b/contrib/ltree_plpython/ltree_plpython.c
index ba5926b8be6..0493aeb2423 100644
--- a/contrib/ltree_plpython/ltree_plpython.c
+++ b/contrib/ltree_plpython/ltree_plpython.c
@@ -2,7 +2,7 @@

 #include "fmgr.h"
 #include "ltree/ltree.h"
-#include "plpython.h"
+#include "plpy_util.h"

 PG_MODULE_MAGIC_EXT(
                     .name = "ltree_plpython",
diff --git a/src/pl/plpython/plpy_cursorobject.c b/src/pl/plpython/plpy_cursorobject.c
index 1c6be756120..37d7efca77c 100644
--- a/src/pl/plpython/plpy_cursorobject.c
+++ b/src/pl/plpython/plpy_cursorobject.c
@@ -16,7 +16,7 @@
 #include "plpy_planobject.h"
 #include "plpy_resultobject.h"
 #include "plpy_spi.h"
-#include "plpython.h"
+#include "plpy_util.h"
 #include "utils/memutils.h"

 static PyObject *PLy_cursor_query(const char *query);
diff --git a/src/pl/plpython/plpy_elog.c b/src/pl/plpython/plpy_elog.c
index 70de5ba13d7..ddf3573f0e7 100644
--- a/src/pl/plpython/plpy_elog.c
+++ b/src/pl/plpython/plpy_elog.c
@@ -10,7 +10,7 @@
 #include "plpy_elog.h"
 #include "plpy_main.h"
 #include "plpy_procedure.h"
-#include "plpython.h"
+#include "plpy_util.h"

 PyObject   *PLy_exc_error = NULL;
 PyObject   *PLy_exc_fatal = NULL;
diff --git a/src/pl/plpython/plpy_exec.c b/src/pl/plpython/plpy_exec.c
index 00747bb811b..28fbd443b98 100644
--- a/src/pl/plpython/plpy_exec.c
+++ b/src/pl/plpython/plpy_exec.c
@@ -17,7 +17,7 @@
 #include "plpy_main.h"
 #include "plpy_procedure.h"
 #include "plpy_subxactobject.h"
-#include "plpython.h"
+#include "plpy_util.h"
 #include "utils/fmgrprotos.h"
 #include "utils/rel.h"

diff --git a/src/pl/plpython/plpy_main.c b/src/pl/plpython/plpy_main.c
index 8f56155f006..f36eadbadc6 100644
--- a/src/pl/plpython/plpy_main.c
+++ b/src/pl/plpython/plpy_main.c
@@ -18,7 +18,7 @@
 #include "plpy_plpymodule.h"
 #include "plpy_procedure.h"
 #include "plpy_subxactobject.h"
-#include "plpython.h"
+#include "plpy_util.h"
 #include "utils/guc.h"
 #include "utils/memutils.h"
 #include "utils/rel.h"
diff --git a/src/pl/plpython/plpy_planobject.c b/src/pl/plpython/plpy_planobject.c
index 3e385555e5e..6044893afdd 100644
--- a/src/pl/plpython/plpy_planobject.c
+++ b/src/pl/plpython/plpy_planobject.c
@@ -9,7 +9,7 @@
 #include "plpy_cursorobject.h"
 #include "plpy_planobject.h"
 #include "plpy_spi.h"
-#include "plpython.h"
+#include "plpy_util.h"
 #include "utils/memutils.h"

 static void PLy_plan_dealloc(PLyPlanObject *self);
diff --git a/src/pl/plpython/plpy_plpymodule.c b/src/pl/plpython/plpy_plpymodule.c
index ea06d9a52b1..1f980b44b2a 100644
--- a/src/pl/plpython/plpy_plpymodule.c
+++ b/src/pl/plpython/plpy_plpymodule.c
@@ -14,7 +14,7 @@
 #include "plpy_resultobject.h"
 #include "plpy_spi.h"
 #include "plpy_subxactobject.h"
-#include "plpython.h"
+#include "plpy_util.h"
 #include "utils/builtins.h"

 HTAB       *PLy_spi_exceptions = NULL;
diff --git a/src/pl/plpython/plpy_procedure.c b/src/pl/plpython/plpy_procedure.c
index b494eeb474f..c176d24e801 100644
--- a/src/pl/plpython/plpy_procedure.c
+++ b/src/pl/plpython/plpy_procedure.c
@@ -13,7 +13,7 @@
 #include "plpy_elog.h"
 #include "plpy_main.h"
 #include "plpy_procedure.h"
-#include "plpython.h"
+#include "plpy_util.h"
 #include "utils/builtins.h"
 #include "utils/hsearch.h"
 #include "utils/memutils.h"
diff --git a/src/pl/plpython/plpy_resultobject.c b/src/pl/plpython/plpy_resultobject.c
index 80fa1d7accc..0d9997cbaa3 100644
--- a/src/pl/plpython/plpy_resultobject.c
+++ b/src/pl/plpython/plpy_resultobject.c
@@ -8,7 +8,7 @@

 #include "plpy_elog.h"
 #include "plpy_resultobject.h"
-#include "plpython.h"
+#include "plpy_util.h"

 static void PLy_result_dealloc(PLyResultObject *self);
 static PyObject *PLy_result_colnames(PyObject *self, PyObject *unused);
diff --git a/src/pl/plpython/plpy_spi.c b/src/pl/plpython/plpy_spi.c
index 77fbfd6c868..1e386aadcca 100644
--- a/src/pl/plpython/plpy_spi.c
+++ b/src/pl/plpython/plpy_spi.c
@@ -19,7 +19,7 @@
 #include "plpy_plpymodule.h"
 #include "plpy_resultobject.h"
 #include "plpy_spi.h"
-#include "plpython.h"
+#include "plpy_util.h"
 #include "utils/memutils.h"

 static PyObject *PLy_spi_execute_query(char *query, long limit);
diff --git a/src/pl/plpython/plpy_subxactobject.c b/src/pl/plpython/plpy_subxactobject.c
index ad24a9fc4b8..c2484a99b4a 100644
--- a/src/pl/plpython/plpy_subxactobject.c
+++ b/src/pl/plpython/plpy_subxactobject.c
@@ -9,7 +9,7 @@
 #include "access/xact.h"
 #include "plpy_elog.h"
 #include "plpy_subxactobject.h"
-#include "plpython.h"
+#include "plpy_util.h"
 #include "utils/memutils.h"

 List       *explicit_subtransactions = NIL;
diff --git a/src/pl/plpython/plpy_typeio.c b/src/pl/plpython/plpy_typeio.c
index 1d127ae3ffe..f6509a41902 100644
--- a/src/pl/plpython/plpy_typeio.c
+++ b/src/pl/plpython/plpy_typeio.c
@@ -14,7 +14,7 @@
 #include "plpy_elog.h"
 #include "plpy_main.h"
 #include "plpy_typeio.h"
-#include "plpython.h"
+#include "plpy_util.h"
 #include "utils/array.h"
 #include "utils/builtins.h"
 #include "utils/fmgroids.h"
diff --git a/src/pl/plpython/plpy_util.c b/src/pl/plpython/plpy_util.c
index 6d89b1cb60a..ef710aa371e 100644
--- a/src/pl/plpython/plpy_util.c
+++ b/src/pl/plpython/plpy_util.c
@@ -9,7 +9,6 @@
 #include "mb/pg_wchar.h"
 #include "plpy_elog.h"
 #include "plpy_util.h"
-#include "plpython.h"

 /*
  * Convert a Python unicode object to a Python string/bytes object in
diff --git a/src/pl/plpython/plpython.h b/src/pl/plpython/plpython.h
index 06fc1a5440f..118b3100840 100644
--- a/src/pl/plpython/plpython.h
+++ b/src/pl/plpython/plpython.h
@@ -2,6 +2,10 @@
  *
  * plpython.h - Python as a procedural language for PostgreSQL
  *
+ * Note: this file is #include'd by each of the sub-module header files
+ * (plpy_elog.h, etc).  It's therefore unnecessary for any plpython *.c
+ * files to include it directly.
+ *
  * Portions Copyright (c) 1996-2025, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
@@ -38,9 +42,4 @@
 #undef TEXTDOMAIN
 #define TEXTDOMAIN PG_TEXTDOMAIN("plpython")

-/*
- * Used throughout, so it's easier to just include it everywhere.
- */
-#include "plpy_util.h"
-
 #endif                            /* PLPYTHON_H */

Hi,

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

Thanks!

> I think the actual problem is that plpython.h
> does this:
> 
> /*
>  * Used throughout, so it's easier to just include it everywhere.
>  */
> #include "plpy_util.h"

Agree.

> which is exactly the sort of cowboy modularity violation that hurts
> when you have to clean it up.  Taking that out requires having to
> manually include plpy_util.h in all the relevant .c files, but on
> the other hand we can remove their vestigial direct inclusions of
> plpython.h.  It was always pretty silly to #include that after
> including some plpy_foo.h files, so let's stop doing so.  The attached
> patch therefore boils down in most places to s/plpython.h/plpy_util.h/.
> 
> (A small number of these files still compiled without that, indicating
> that they're not actually using plpy_util.h today.  But I figured we
> might as well just do it uniformly.)

Yeah, makes sense. I checked the s/plpython.h/plpy_util.h/ replacements and
the includes alphabetical ordering is still preserved. Also misc-header-include-cycle
is now happy so LGTM.

Regards,

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

Bertrand Drouvot <bertranddrouvot.pg@gmail.com> writes:
> On Sat, Apr 26, 2025 at 02:55:51PM -0400, Tom Lane wrote:
>> I think the actual problem is that plpython.h
>> does this:
>>
>> /*
>> * Used throughout, so it's easier to just include it everywhere.
>> */
>> #include "plpy_util.h"

> Agree.

>> which is exactly the sort of cowboy modularity violation that hurts
>> when you have to clean it up.  Taking that out requires having to
>> manually include plpy_util.h in all the relevant .c files, but on
>> the other hand we can remove their vestigial direct inclusions of
>> plpython.h.

> Yeah, makes sense. I checked the s/plpython.h/plpy_util.h/ replacements and
> the includes alphabetical ordering is still preserved. Also misc-header-include-cycle
> is now happy so LGTM.

Pushed, thanks for reviewing.  I'll leave the wait_event.h issue
to Michael.

            regards, tom lane

On Sat, Apr 26, 2025 at 08:31:32AM +0000, Bertrand Drouvot wrote:
> On Sat, Apr 26, 2025 at 04:42:56PM +0900, Michael Paquier wrote:
>> 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?
>
> Yeah, better. Done that way in the attached.

--- a/src/include/utils/wait_event.h
+++ b/src/include/utils/wait_event.h
@@ -10,21 +10,8 @@
[...]
+/* wait classes  */
+#include "utils/wait_classes.h"

This part is not required.  wait_event.h can survive the day even if
it does not know about that.

Note that it is true that wait_event.h could also work without
wait_event_types.h, but it is more useful to keep it in wait_event.h
as all the other code paths in need of the pgstat_report_* calls want
to know about the wait event enums.

I've reproduced your original report with clang-tidy on my end,
removed the include that was not required in wait_event.h, fixed one
comment, cross-checked that the result actually works, and applied the
result.  Thanks for the report!
--
Michael