Horton

Thread

Commits

  1. Remove circular #include's between wait_event.h and wait_event_types.h

  2. Remove circular #include's between plpython.h and plpy_util.h.

  1. Avoid circular header file dependency

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-04-26T03:20:09Z

    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

  2. Re: Avoid circular header file dependency

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-04-26T05:20:56Z

    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

  3. Re: Avoid circular header file dependency

    Michael Paquier <michael@paquier.xyz> — 2025-04-26T07:42:56Z

    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

  4. Re: Avoid circular header file dependency

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-04-26T08:11:50Z

    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

  5. Re: Avoid circular header file dependency

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-04-26T08:31:32Z

    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

  6. Re: Avoid circular header file dependency

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-04-26T18:55:51Z

    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

  7. Re: Avoid circular header file dependency

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-04-27T08:17:11Z

    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

  8. Re: Avoid circular header file dependency

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-04-27T15:46:10Z

    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

  9. Re: Avoid circular header file dependency

    Michael Paquier <michael@paquier.xyz> — 2025-04-28T00:12:30Z

    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

  10. Re: Avoid circular header file dependency

    John Naylor <johncnaylorls@gmail.com> — 2025-07-07T06:58:18Z

    On Mon, Apr 28, 2025 at 7:12 AM Michael Paquier <michael@paquier.xyz> wrote:
> 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!

With that, can this CF entry be closed?

-- 
John Naylor
Amazon Web Services

  11. Re: Avoid circular header file dependency

    Michael Paquier <michael@paquier.xyz> — 2025-07-07T07:04:41Z

    On Mon, Jul 07, 2025 at 01:58:18PM +0700, John Naylor wrote:
> With that, can this CF entry be closed?

I've missed that there was a CF entry.  Closed now.
--
Michael