Horton

Thread

Commits

  1. Move instrumentation-related structs to instrument_node.h

  2. Don't include access/htup_details.h in executor/tuptable.h

  3. Don't include execnodes.h in brin.h or gin.h

  4. Do a tiny bit of header file maintenance

  5. Don't include execnodes.h in replication/conflict.h

  6. Update some more forward declarations to use typedef

  7. Change fmgr.h typedefs to use original names

  8. Improve ExplainState type handling in header files

  9. Remove hbaPort type

  10. Remove workarounds against repeat typedefs

  11. Update various forward declarations to use typedef

  12. Allow redeclaration of typedef yyscan_t

  13. Improve pgbench definition of yyscan_t

  14. Delay extraction of TIDBitmap per page offsets

  15. Remove pgrminclude and associated scripts

  16. Allow to use HeapTupleData embedded in [Buffer]HeapTupleTableSlot.

  1. allow benign typedef redefinitions (C11)

    Peter Eisentraut <peter@eisentraut.org> — 2025-08-29T12:45:54Z

    Here is the first of several (to come at a later date) patch sets to 
take some advantage of C11 features.

This is, I hope, a very gentle start that shouldn't stress even older 
compilers very much, and should bring some tangible benefits that had 
already been asked for around here.

In C11, typedef redefinitions are allowed, as long as they are the same.  So

typedef int foo;
typedef int foo;

is allowed, where the second occurrence would have led to a diagnostic 
in previous C versions.

(C++ also allows this, so this will preserve C++ compatibility of the 
headers.)

What is not allowed is something like this of course:

typedef int foo;
typedef double foo;

This will continue to be an error.

This facility is often useful to untangle dependencies between header 
files.  So instead of having one header include another, now the first 
header can just make its own typedef of whatever types it needs.  If the 
two headers are later included together, then this will not (any more) 
be a conflict.  This often works together with declaring incomplete 
types using struct.

The PostgreSQL code already contains a couple of places that wanted to 
do something like this but had to install manual workarounds with #ifdef 
guards.  These can now be removed.  There are also a bunch of struct 
forward declarations that can be "upgraded" to full typedefs.  This 
makes the function prototypes look more consistent.

In this patch set, 0001 is a prerequisite for 0002, 0002 and 0003 remove 
some existing workarounds, the remaining patches are additional 
opportunities for cleanup and simplification.

All of this is only notational, there are no include changes or API 
changes or changes in behavior.

(After this, it would probably be possible to undertake some deeper 
efforts to untangle header files, with the help of IWYU.  But that is a 
separate project.)

  2. Re: allow benign typedef redefinitions (C11)

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-08-29T13:57:33Z

    Peter Eisentraut <peter@eisentraut.org> writes:
> Here is the first of several (to come at a later date) patch sets to 
> take some advantage of C11 features.

I haven't checked this line-by-line, but +1 for concept (assuming the
build farm is actually happy with it).  0006 and 0007, in particular,
clean up some very messy hacks.

			regards, tom lane

  3. Re: allow benign typedef redefinitions (C11)

    Chao Li <li.evan.chao@gmail.com> — 2025-09-01T03:32:11Z

    
> On Aug 29, 2025, at 20:45, Peter Eisentraut <peter@eisentraut.org> wrote:
> 
> (After this, it would probably be possible to undertake some deeper efforts to untangle header files, with the help of IWYU.  But that is a separate project.)
> <0001-Improve-pgbench-definition-of-yyscan_t.patch><0002-Allow-redeclaration-of-typedef-yyscan_t.patch><0003-Remove-workarounds-against-repeat-typedefs.patch><0004-Improve-ExplainState-type-handling-in-header-files.patch><0005-Update-various-forward-declarations-to-use-typedef.patch><0006-Remove-hbaPort-type.patch><0007-Change-fmgr.h-typedefs-to-use-original-names.patch>



I applied the patch files and build passed without any error and warning on my MacBook.

Just a few small comments:

1. For 0001, I think “#ifdef YY_TYPEDEF_YY_SCANNER_T” is not needed, but I see you have removed that in 0002, so this is not a comment now.

2. For 0002, looks like som duplicate code for definitions of

Union YYSTYPE;
#define YYLTYPE int
Typedef void *yyscan_t;

Can we put them into a separate header file say src/include/yy_types.h?

3. For 0002, this is not your change, I am just pointing it out: “#define YYSTYPE char *”, where “YYSTYPE” is defined as “union” everywhere else. Can we rename it to something like “YYSTYPE_CHARPTR” here to avoid confusion? I tried to rename it and build still passed.

4. For 0004

diff --git a/src/include/commands/explain_dr.h b/src/include/commands/explain_dr.h
index 55da63d66bd..ce424aa2a55 100644
--- a/src/include/commands/explain_dr.h
+++ b/src/include/commands/explain_dr.h
@@ -16,7 +16,7 @@
 #include "executor/instrument.h"
 #include "tcop/dest.h"

-struct ExplainState;                   /* avoid including explain.h here */
+typedef struct ExplainState ExplainState;      /* avoid including explain.h here */

“Struct ExplainState” is defined in explain_state.h, so the comment here should be “avoid including explain_state.h here”.

Same thing applies to explain_format.h and fdwapi.h.

5. For 0005

diff --git a/src/include/optimizer/optimizer.h b/src/include/optimizer/optimizer.h
index 03b214755c2..d490ef3b506 100644
--- a/src/include/optimizer/optimizer.h
+++ b/src/include/optimizer/optimizer.h
@@ -35,9 +35,9 @@ typedef struct IndexOptInfo IndexOptInfo;
 typedef struct SpecialJoinInfo SpecialJoinInfo;

 /* It also seems best not to include plannodes.h, params.h, or htup.h here */
-struct PlannedStmt;
-struct ParamListInfoData;
-struct HeapTupleData;
+typedef struct PlannedStmt PlannedStmt;
+typedef struct ParamListInfoData ParamListInfoData;
+typedef struct HeapTupleData *HeapTuple;

Why don’t define ParamListInfo in the same way as HeapTuple like “typedef structure ParamListInfoData *ParamListInfo”. I tried that in my local and build still passed.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  4. Re: allow benign typedef redefinitions (C11)

    Peter Eisentraut <peter@eisentraut.org> — 2025-09-10T06:28:27Z

    On 01.09.25 05:32, Chao Li wrote:
> 1. For 0001, I think “#ifdef YY_TYPEDEF_YY_SCANNER_T” is not needed, but 
> I see you have removed that in 0002, so this is not a comment now.

Yes, this is just so that the code after applying patch 0001 still 
works, even if some code is later removed.

> 2. For 0002, looks like som duplicate code for definitions of
> 
> Union YYSTYPE;
> #define YYLTYPE int
> Typedef void *yyscan_t;
> 
> Can we put them into a separate header file say src/include/yy_types.h?

Each scanner/parser might have its own idea of what type it wants to 
use, so this might not work.  It's better not to get into the 
complications of this for this patch set.

> 3. For 0002, this is not your change, I am just pointing it out: 
> “#define YYSTYPE char *”, where “YYSTYPE” is defined as “union” 
> everywhere else. Can we rename it to something like “YYSTYPE_CHARPTR” 
> here to avoid confusion? I tried to rename it and build still passed.

The name YYSTYPE is required by bison.  I'm not sure how it still works 
without it.  But anyway, see above, it's probably better not to crack 
open the bison/flex mess again here.

> 4. For 0004
> 
> diff --git a/src/include/commands/explain_dr.h b/src/include/commands/ 
> explain_dr.h
> index 55da63d66bd..ce424aa2a55 100644
> --- a/src/include/commands/explain_dr.h
> +++ b/src/include/commands/explain_dr.h
> @@ -16,7 +16,7 @@
>   #include "executor/instrument.h"
>   #include "tcop/dest.h"
> 
> -struct ExplainState;                   /* avoid including explain.h here */
> +typedef struct ExplainState ExplainState;      /* avoid including 
> explain.h here */
> 
> “Struct ExplainState” is defined in explain_state.h, so the comment here 
> should be “avoid including explain_state.h here”.
> 
> Same thing applies to explain_format.h and fdwapi.h.

Yes, good catch.  The original patch that introduced these comments 
actually did replace an include of explain.h, but arguably even that was 
already too broad.  So I agree we should fix these comments.

> 5. For 0005
> 
> diff --git a/src/include/optimizer/optimizer.h b/src/include/optimizer/ 
> optimizer.h
> index 03b214755c2..d490ef3b506 100644
> --- a/src/include/optimizer/optimizer.h
> +++ b/src/include/optimizer/optimizer.h
> @@ -35,9 +35,9 @@ typedef struct IndexOptInfo IndexOptInfo;
>   typedef struct SpecialJoinInfo SpecialJoinInfo;
> 
>   /* It also seems best not to include plannodes.h, params.h, or htup.h 
> here */
> -struct PlannedStmt;
> -struct ParamListInfoData;
> -struct HeapTupleData;
> +typedef struct PlannedStmt PlannedStmt;
> +typedef struct ParamListInfoData ParamListInfoData;
> +typedef struct HeapTupleData *HeapTuple;
> 
> Why don’t define ParamListInfo in the same way as HeapTuple like 
> “typedef structure ParamListInfoData *ParamListInfo”. I tried that in my 
> local and build still passed.

Yes, that seems more correct.

Thanks.  New patch set attached.

  5. Re: allow benign typedef redefinitions (C11)

    Chao Li <li.evan.chao@gmail.com> — 2025-09-10T12:08:10Z

    
> On Sep 10, 2025, at 14:28, Peter Eisentraut <peter@eisentraut.org> wrote:
> 
> Thanks.  New patch set attached.
> <v2-0001-Improve-pgbench-definition-of-yyscan_t.patch><v2-0002-Allow-redeclaration-of-typedef-yyscan_t.patch><v2-0003-Remove-workarounds-against-repeat-typedefs.patch><v2-0004-Improve-ExplainState-type-handling-in-header-file.patch><v2-0005-Update-various-forward-declarations-to-use-typede.patch><v2-0006-Remove-hbaPort-type.patch><v2-0007-Change-fmgr.h-typedefs-to-use-original-names.patch>


v5 looks good to me.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  6. Re: allow benign typedef redefinitions (C11)

    Peter Eisentraut <peter@eisentraut.org> — 2025-09-15T09:25:31Z

    On 10.09.25 14:08, Chao Li wrote:
>> On Sep 10, 2025, at 14:28, Peter Eisentraut <peter@eisentraut.org> wrote:
>>
>> Thanks.  New patch set attached.
>> <v2-0001-Improve-pgbench-definition-of-yyscan_t.patch><v2-0002-Allow- 
>> redeclaration-of-typedef-yyscan_t.patch><v2-0003-Remove-workarounds- 
>> against-repeat-typedefs.patch><v2-0004-Improve-ExplainState-type- 
>> handling-in-header-file.patch><v2-0005-Update-various-forward- 
>> declarations-to-use-typede.patch><v2-0006-Remove-hbaPort- 
>> type.patch><v2-0007-Change-fmgr.h-typedefs-to-use-original-names.patch>
> 
> v5 looks good to me.

Committed, thanks.

  7. Re: allow benign typedef redefinitions (C11)

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-09-19T10:52:02Z

    Here's a few more forward struct declarations turned into typedefs, for
cleanliness sake.

struct VacuumCutoffs could use a typedef, by the way ...

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"La espina, desde que nace, ya pincha" (Proverbio africano)

  8. Re: allow benign typedef redefinitions (C11)

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-09-19T19:58:53Z

    I happened to realize that brin_tuple.h (a somewhat nasty header which
was only supposed to be used by BRIN itself and stuff like amcheck)
recently somehow snuck into tuplesort.h with some nefarious
consequences: that one has polluted execnodes.h, which means that header
now depends on struct definitions that appear in genam.h.  We should fix
this.  This patch is not the full solution, just a way to show the
problem; for a definitive solution, IMO the definitions of structs
IndexScanInstrumentation and SharedIndexScanInstrumentation should be
elsewhere so that execnodes.h doesn't have to include genam.h.

(gin_tuple.h is also there, but that one's much less of a problem.)

Oh, and replication/conflict.h including all of execnodes.h is kind of
ridiculous.  Looks like that's easily solved with something like this
though:

diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index f402b17295c..7bcb4c68e18 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -11,6 +11,7 @@
 #ifndef PGSTAT_H
 #define PGSTAT_H

+#include "access/transam.h"
 #include "datatype/timestamp.h"
 #include "portability/instr_time.h"
 #include "postmaster/pgarch.h" /* for MAX_XFN_CHARS */
diff --git a/src/include/replication/conflict.h b/src/include/replication/conflict.h
index e516caa5c73..e71b3c8f3d3 100644
--- a/src/include/replication/conflict.h
+++ b/src/include/replication/conflict.h
@@ -9,9 +9,14 @@
 #ifndef CONFLICT_H
 #define CONFLICT_H

-#include "nodes/execnodes.h"
+#include "access/xlogdefs.h"
+#include "nodes/pg_list.h"
 #include "utils/timestamp.h"

+typedef struct EState EState;
+typedef struct ResultRelInfo ResultRelInfo;
+typedef struct TupleTableSlot TupleTableSlot;
+
 /*
  * Conflict types that could occur while applying remote changes.
  *


-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"La espina, desde que nace, ya pincha" (Proverbio africano)

  9. Re: allow benign typedef redefinitions (C11)

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-09-20T01:17:24Z

    =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de> writes:
> Oh, and replication/conflict.h including all of execnodes.h is kind of
> ridiculous.

Sooner or later we need to work out a subsystem hierarchy that
can define which headers should be allowed to include which
other ones.  I have no clear ideas about what that should look
like, but I think the need for it gets ever more urgent.

			regards, tom lane

  10. Re: allow benign typedef redefinitions (C11)

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-09-20T13:13:24Z

    On 2025-Sep-19, Tom Lane wrote:

> Sooner or later we need to work out a subsystem hierarchy that
> can define which headers should be allowed to include which
> other ones.  I have no clear ideas about what that should look
> like, but I think the need for it gets ever more urgent.

I agree, we should do that.  In the meantime, it would help if people
were a tiny bit more careful with what headers they add to other
headers.

Here's a proposed fix.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
<Schwern> It does it in a really, really complicated way
<crab> why does it need to be complicated?
<Schwern> Because it's MakeMaker.

  11. Re: allow benign typedef redefinitions (C11)

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-09-20T21:21:19Z

    =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de> writes:
> Here's a proposed fix.

I didn't test this, but it passes an eyeball sanity check
and looks like an improvement overall.

			regards, tom lane

  12. Re: allow benign typedef redefinitions (C11)

    Peter Eisentraut <peter@eisentraut.org> — 2025-09-23T08:01:03Z

    On 19.09.25 12:52, Álvaro Herrera wrote:
> Here's a few more forward struct declarations turned into typedefs, for
> cleanliness sake.

Two comments; the rest looks okay to me.

* src/include/access/heapam.h

You are removing the declaration of struct TupleTableSlot and then go
on to use just TupleTableSlot; apparently that works, but that creates
an implicit dependency on typedef TupleTableSlot from somewhere else.
It might be better to change the existing struct TupleTableSlot
declaration into a typedef and keep it.

* src/include/nodes/execnodes.h

In this file, you are changing several times struct ScanKeyData to
just ScanKeyData, but there is no typedef of ScanKeyData in this file.
The struct ScanKeyData can exist by itself (it becomes declared as
soon as you use it), but the typedef needs to come from somewhere.  So
this change introduces a new dependency between this header and some
other header that provides that typedef.  I suggest you fix that by
adding a typedef in this file.

  13. Re: allow benign typedef redefinitions (C11)

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-09-25T12:36:33Z

    On 2025-Sep-23, Peter Eisentraut wrote:

> On 19.09.25 12:52, Álvaro Herrera wrote:
> > Here's a few more forward struct declarations turned into typedefs, for
> > cleanliness sake.
> 
> Two comments; the rest looks okay to me.

Thanks, pushed.


-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"After a quick R of TFM, all I can say is HOLY CR** THAT IS COOL! PostgreSQL was
amazing when I first started using it at 7.2, and I'm continually astounded by
learning new features and techniques made available by the continuing work of
the development team."
Berend Tober, http://archives.postgresql.org/pgsql-hackers/2007-08/msg01009.php

  14. Re: allow benign typedef redefinitions (C11)

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-09-25T12:53:55Z

    On 2025-Sep-20, Tom Lane wrote:

> =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de> writes:
> > Here's a proposed fix.
> 
> I didn't test this, but it passes an eyeball sanity check
> and looks like an improvement overall.

Thanks, pushed now.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"El miedo atento y previsor es la madre de la seguridad" (E. Burke)

  15. Re: allow benign typedef redefinitions (C11)

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-09-28T09:23:51Z

    Hello,

Here's another simple piece that I noticed while fooling around with the doxygen report -- genam.h was including tidbitmap.h and relcache.h for the sake of one typedefs each, and this was bleeding in other places in a rather random fashion, particularly through amapi.h.  I propose the following to fix it.  The changes to inet.h and varbit.h are surprising, but required.  (It's possible that we should be including varatt.h in more datatype headers than just those two, but at least these are the ones that complained.  So another option would be to include varatt.h in the files that include those headers. However, that seems to me to be the wrong direction).


-- 
Álvaro Herrera

  16. Re: allow benign typedef redefinitions (C11)

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-09-28T14:35:19Z

    =?UTF-8?Q?=C3=81lvaro_Herrera?= <alvherre@kurilemu.de> writes:
> The changes to inet.h and varbit.h are surprising, but required.

Why?  Those headers compile now, and you've not removed any includes
that they might indirectly depend on.

> (It's possible that we should be including varatt.h in more datatype headers than just those two, but at least these are the ones that complained.  So another option would be to include varatt.h in the files that include those headers. However, that seems to me to be the wrong direction).

If what you mean is that some of the macros in these headers require
varatt.h to be used, I do not agree that that means we should put
varatt.h into these headers.  We've frequently relied on exactly
that property of macro definitions to avoid excessive or circular
inclusions.  Moreover, even if we wanted to change that policy,
I know of no simple way to check it.

Furthermore, this whole business of using "typedef struct foo foo;"
to avoid including foo.h is predicated on the assumption that the
ultimate consumer (some .c file) will itself include foo.h, because
otherwise it's most likely gonna have problems with the incomplete
typedef.  So changing the rules about macros seems to me to be going
in quite the wrong direction.

I think what we're trying to do here is reduce the #includes of
header files to be just enough to compile the headers themselves.
That will bleed out into more #include's required in .c files,
but that's fine by me.

			regards, tom lane

  17. Re: allow benign typedef redefinitions (C11)

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-09-28T16:54:23Z

    On 2025-Sep-28, Tom Lane wrote:

> =?UTF-8?Q?=C3=81lvaro_Herrera?= <alvherre@kurilemu.de> writes:
> > The changes to inet.h and varbit.h are surprising, but required.
> 
> Why?  Those headers compile now, and you've not removed any includes
> that they might indirectly depend on.

That's true.

> If what you mean is that some of the macros in these headers require
> varatt.h to be used, I do not agree that that means we should put
> varatt.h into these headers.  We've frequently relied on exactly
> that property of macro definitions to avoid excessive or circular
> inclusions.  Moreover, even if we wanted to change that policy,
> I know of no simple way to check it.

We used to have a script called pgdefine that would create a .c file
that would expand all macros defined in a header, and then ensure that
the C file compiled.  It was removed only recently (Dec. 2024), in
commit 5af699066f81, though it's true that apparently nobody had used in
a while.

> I think what we're trying to do here is reduce the #includes of
> header files to be just enough to compile the headers themselves.
> That will bleed out into more #include's required in .c files,
> but that's fine by me.

Ok, that requires adding varatt.h to just two .c files -- attached.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

  18. Re: allow benign typedef redefinitions (C11)

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-09-29T16:45:52Z

    Hello,

I was thinking about your (Tom's) idea of having some sort of header
inclusion policy.  Our current situation is that cross-module inclusions
are quite widespread and the dependencies can probably be seen as a
tight web (modules probably being defined as our subdirectories inside
src/include).  A blanket prohibition of inclusion of headers of other
modules would certainly not work, but if we imagine that within each
module we have a hierarchy of sorts, then it would make sense to have a
policy that other modules can include high level headers of other
modules, but not lower-level headers.  For instance, it's okay if
files in src/include/executor include high-level brin.h, but it's not
okay if it has to include brin_tuple.h which is supposed to be
lower-level.

With that in mind, I gave another long stare to the doxygen reports for
some of these files.  It now seems to me that removing tidbitmap.h from
genam.h is somewhat bogus, because after all tidbitmap.h is a legitimate
"library" which is okay to be included in other headers, and the real
bug here is the fact that only very recently (commit bfe56cdf9a4e in Feb
2025) it acquired htup_details.h just in order to be able to define
TBM_MAX_TUPLES_PER_PAGE.  If we remove htup_details.h from tidbitmap.h,
and we also remove the inclusion of relcache.h by adding a typedef for
Relation, then genam.h is a much better behaved header than before.
Hence the attached patches.


I've been looking at removing some includes from a few more headers, but
I'm not happy with the overall achievement, or at least I don't have a
good metric to present as a win.  So I'll leave that for another day and
maybe present a different way to look at the problem.  One thing we
should certainly do is fix the inclusion of brin_tuple.h and gin_tuple.h
into tuplesort.h, as I mentioned upthread.  That is definitely a mess,
but I think it requires a new file.

Another thing we should look into is splitting the ObjectType enum out
of parsenodes.h into a new file of its own.  We have objectaddress.h
depending on the whole of parsenodes.h just to have that enum, for
instance.  I think that would be useful.  I have the basic patch for
that and I kinda like it, but I want to analyze it a bit more before
posting to avoid rushing to conclusions.

Thanks

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
Thou shalt study thy libraries and strive not to reinvent them without
cause, that thy code may be short and readable and thy days pleasant
and productive. (7th Commandment for C Programmers)

  19. Re: allow benign typedef redefinitions (C11)

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-09-29T17:08:30Z

    =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de> writes:
> I was thinking about your (Tom's) idea of having some sort of header
> inclusion policy.  Our current situation is that cross-module inclusions
> are quite widespread and the dependencies can probably be seen as a
> tight web (modules probably being defined as our subdirectories inside
> src/include).  A blanket prohibition of inclusion of headers of other
> modules would certainly not work, but if we imagine that within each
> module we have a hierarchy of sorts, then it would make sense to have a
> policy that other modules can include high level headers of other
> modules, but not lower-level headers.  For instance, it's okay if
> files in src/include/executor include high-level brin.h, but it's not
> okay if it has to include brin_tuple.h which is supposed to be
> lower-level.

Yeah, I've been thinking about that off-and-on, and don't have
anything to present yet either.

> With that in mind, I gave another long stare to the doxygen reports for
> some of these files.  It now seems to me that removing tidbitmap.h from
> genam.h is somewhat bogus, because after all tidbitmap.h is a legitimate
> "library" which is okay to be included in other headers, and the real
> bug here is the fact that only very recently (commit bfe56cdf9a4e in Feb
> 2025) it acquired htup_details.h just in order to be able to define
> TBM_MAX_TUPLES_PER_PAGE.  If we remove htup_details.h from tidbitmap.h,
> and we also remove the inclusion of relcache.h by adding a typedef for
> Relation, then genam.h is a much better behaved header than before.
> Hence the attached patches.

These patches seem fine to me.

> Another thing we should look into is splitting the ObjectType enum out
> of parsenodes.h into a new file of its own.  We have objectaddress.h
> depending on the whole of parsenodes.h just to have that enum, for
> instance.  I think that would be useful.

+1.  I think that primnodes.h and parsenodes.h are always going to
need to be included by a whole lot of things, but to the extent that
we can remove those inclusions from relatively low-level headers,
it can't hurt.

			regards, tom lane

  20. Re: allow benign typedef redefinitions (C11)

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-09-30T11:16:50Z

    On 2025-Sep-19, Álvaro Herrera wrote:

> I happened to realize that brin_tuple.h (a somewhat nasty header which
> was only supposed to be used by BRIN itself and stuff like amcheck)
> recently somehow snuck into tuplesort.h with some nefarious
> consequences: that one has polluted execnodes.h, which means that header
> now depends on struct definitions that appear in genam.h.  We should fix
> this.  This patch is not the full solution, just a way to show the
> problem; for a definitive solution, IMO the definitions of structs
> IndexScanInstrumentation and SharedIndexScanInstrumentation should be
> elsewhere so that execnodes.h doesn't have to include genam.h.
> 
> (gin_tuple.h is also there, but that one's much less of a problem.)

Here's a proposed fix for this issue, wherein I move the
IndexScanInstrumentation and SharedIndexScanInstrumentation struct
definitions from genam.h to a new file executor/instrument_node.h.
I think there's room to argue that we should also move
BitmapHeapScanInstrumentation and SharedBitmapHeapInstrumentation
(currently defined in execnodes.h) to the new file; any opinions on
this?  I think these structs belong together, so I'm leaning towards
yes.

Also, does anybody strongly dislike the proposed name?  I admit it looks
unlike the existing names there:

$ ls src/include/executor
execAsync.h            nodeFunctionscan.h         nodeSamplescan.h
execdebug.h            nodeGather.h               nodeSeqscan.h
execdesc.h             nodeGatherMerge.h          nodeSetOp.h
execExpr.h             nodeGroup.h                nodeSort.h
execParallel.h         nodeHash.h                 nodeSubplan.h
execPartition.h        nodeHashjoin.h             nodeSubqueryscan.h
execScan.h             nodeIncrementalSort.h      nodeTableFuncscan.h
executor.h             nodeIndexonlyscan.h        nodeTidrangescan.h
functions.h            nodeIndexscan.h            nodeTidscan.h
hashjoin.h             nodeLimit.h                nodeUnique.h
instrument.h           nodeLockRows.h             nodeValuesscan.h
instrument_node.h      nodeMaterial.h             nodeWindowAgg.h
nodeAgg.h              nodeMemoize.h              nodeWorktablescan.h
nodeAppend.h           nodeMergeAppend.h          spi.h
nodeBitmapAnd.h        nodeMergejoin.h            spi_priv.h
nodeBitmapHeapscan.h   nodeModifyTable.h          tablefunc.h
nodeBitmapIndexscan.h  nodeNamedtuplestorescan.h  tqueue.h
nodeBitmapOr.h         nodeNestloop.h             tstoreReceiver.h
nodeCtescan.h          nodeProjectSet.h           tuptable.h
nodeCustom.h           nodeRecursiveunion.h
nodeForeignscan.h      nodeResult.h

It's not the *first* name lacking camelCase or with an underscore, but
almost.  Note that there's already an instrument.h, which has a
completely unrelated charter.

I also left an XXX comment on genam.h about including instrument_node.h
there or adding the typedefs.  I think I prefer the typedefs myself.

CC'ing Peter G because of 0fbceae841cb and David because of
5a1e6df3b84c.

Thanks,

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Small aircraft do not crash frequently ... usually only once!"
                                  (ponder, http://thedailywtf.com/)

  21. Re: allow benign typedef redefinitions (C11)

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-09-30T14:48:00Z

    I happened to realize that we still have one small "layering violation"
in executor/tuptable.h which is currently including htup_details.h for
no reason (I suspect it was a simple mistake in commit 5408e233f066),
and this bleeds via execnodes.h to a lot of places.  Patch 0002 here
removes that and fixes the affected .c files in the minimal way
required.  Patch 0001 is the same as before.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

  22. Re: allow benign typedef redefinitions (C11)

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-10-02T13:18:16Z

    On 2025-Sep-29, Álvaro Herrera wrote:

> I've been looking at removing some includes from a few more headers, but
> I'm not happy with the overall achievement, or at least I don't have a
> good metric to present as a win.  So I'll leave that for another day and
> maybe present a different way to look at the problem.

Okay, I couldn't put this down.  I wrote a script that greps for all the
"#include" lines in headers and in backend .c files, and put them in a
database.  With that I can figure out the complete list of headers used
by each source file (recursively), which allows to me know how many
source files are including each header file.  The script is attached, in
case anyone is curious.  I didn't bother to optimize it, because I
wasn't going to run it many times anyway.

Using the numbers so obtained and the patches I've been looking at, I
can quantify the reduction in total header usage for each commit, so I
know whether any particular change is quantitatively worthwhile or not,
beyond qualitative modularity considerations.

So, for patch 0001, we can see a massive decrease of .c files that use
each of a bunch of .h files (the first column is the number of .c files
that include that particular .h file before patch; the second column is
the same number, after patch; third column is the difference):

        include         │ orig # included │ final # included │ difference 
────────────────────────┼─────────────────┼──────────────────┼────────────
 nodes/nodes.h          │            5501 │             5130 │        371
 nodes/pg_list.h        │            4463 │             4092 │        371
 storage/off.h          │            3943 │             3424 │        519
 common/relpath.h       │            3917 │             3600 │        317
 nodes/bitmapset.h      │            3888 │             3562 │        326
 storage/itemptr.h      │            3638 │             3119 │        519
 catalog/pg_attribute.h │            3575 │             3207 │        368
 access/tupdesc.h       │            3573 │             3205 │        368
 access/htup.h          │            3297 │             2778 │        519
 storage/dsm.h          │            3273 │             1972 │       1301
 utils/relcache.h       │            2767 │             2389 │        378
 port/atomics.h         │            2734 │             1751 │        983
 lib/pairingheap.h      │            2211 │             1629 │        582
 utils/dsa.h            │            2163 │             1082 │       1081
 utils/snapshot.h       │            1863 │             1282 │        581
 nodes/tidbitmap.h      │            1508 │              924 │        584
 utils/sortsupport.h    │            1489 │             1076 │        413
 access/skey.h          │            1020 │              909 │        111
 access/genam.h         │             976 │              393 │        583
 access/amapi.h         │             725 │              140 │        585
 utils/typcache.h       │             667 │               85 │        582
 access/brin_internal.h │             607 │               21 │        586
 access/ginblock.h      │             606 │               20 │        586
 access/brin_tuple.h    │             601 │               15 │        586
 access/gin_tuple.h     │             588 │                2 │        586
(25 filas)

Patch 0002 doesn't cause a reduction in as many header files, but it's
still quite impressive that htup_details.h is no longer included by a
thousand header files:

        include         │ orig # included │ final # included │ difference 
────────────────────────┼─────────────────┼──────────────────┼────────────
 nodes/nodes.h          │            5130 │             5050 │         80
 nodes/pg_list.h        │            4092 │             4007 │         85
 storage/off.h          │            3424 │             2957 │        467
 catalog/pg_attribute.h │            3207 │             3110 │         97
 access/tupdesc.h       │            3205 │             3108 │         97
 storage/itemptr.h      │            3119 │             2636 │        483
 access/htup.h          │            2778 │             2283 │        495
 access/transam.h       │            2540 │             1923 │        617
 storage/bufpage.h      │            1882 │             1268 │        614
 access/tupmacs.h       │            1692 │             1077 │        615
 access/htup_details.h  │            1375 │              500 │        875

Patch 0003 has a negiglible impact though (which makes sense given what
it does):

       include       │ orig # included │ final # included │ difference 
─────────────────────┼─────────────────┼──────────────────┼────────────
 storage/off.h       │            2957 │             2954 │          3
 storage/itemptr.h   │            2636 │             2633 │          3
 access/htup.h       │            2283 │             2280 │          3
 executor/tuptable.h │            1115 │             1112 │          3
(4 filas)

But patch 0004 again removes inclusion of a large number of headers,
though from not as many .c files:

           include            │ orig # included │ final # included │ difference 
──────────────────────────────┼─────────────────┼──────────────────┼────────────
 nodes/nodes.h                │            5050 │             5008 │         42
 nodes/pg_list.h              │            4007 │             3964 │         43
 common/relpath.h             │            3600 │             3574 │         26
 nodes/bitmapset.h            │            3562 │             3531 │         31
 catalog/pg_attribute.h       │            3110 │             3067 │         43
 access/tupdesc.h             │            3108 │             3065 │         43
 storage/off.h                │            2954 │             2913 │         41
 storage/itemptr.h            │            2633 │             2592 │         41
 utils/relcache.h             │            2389 │             2350 │         39
 access/htup.h                │            2280 │             2243 │         37
 storage/spin.h               │            2217 │             2177 │         40
 nodes/primnodes.h            │            1975 │             1965 │         10
 storage/dsm.h                │            1972 │             1952 │         20
 storage/fd.h                 │            1948 │             1863 │         85
 port/atomics.h               │            1751 │             1746 │          5
 lib/pairingheap.h            │            1629 │             1606 │         23
 storage/proclist_types.h     │            1508 │             1492 │         16
 utils/snapshot.h             │            1282 │             1275 │          7
 storage/bufpage.h            │            1268 │             1246 │         22
 executor/tuptable.h          │            1112 │             1090 │         22
 utils/dsa.h                  │            1082 │             1077 │          5
 access/tupmacs.h             │            1077 │             1055 │         22
 utils/sortsupport.h          │            1076 │             1033 │         43
 storage/fileset.h            │            1073 │             1030 │         43
 nodes/memnodes.h             │            1028 │             1007 │         21
 storage/sharedfileset.h      │            1027 │              984 │         43
 utils/memutils.h             │            1026 │             1005 │         21
 nodes/tidbitmap.h            │             924 │              917 │          7
 utils/queryenvironment.h     │             913 │              892 │         21
 access/skey.h                │             909 │              902 │          7
 access/itup.h                │             785 │              763 │         22
 nodes/plannodes.h            │             694 │              672 │         22
 storage/condition_variable.h │             676 │              654 │         22
 utils/tuplestore.h           │             635 │              613 │         22
 executor/instrument.h        │             601 │              579 │         22
 nodes/miscnodes.h            │             600 │              578 │         22
 access/attmap.h              │             593 │              571 │         22
 utils/logtape.h              │             588 │              566 │         22
 utils/tuplesort.h            │             585 │              563 │         22
 lib/simplehash.h             │             581 │              559 │         22
 access/tupconvert.h          │             575 │              553 │         22
 utils/sharedtuplestore.h     │             573 │              551 │         22
 nodes/execnodes.h            │             570 │              548 │         22
(43 filas)


My inclination at this point is to apply all four, because the
modularity wins are pretty obvious.  Of course there are other ways to
look at the data (in particular: number of .h files that each other .h
cascades to), but I'm going to stop here.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Oh, great altar of passive entertainment, bestow upon me thy discordant images
at such speed as to render linear thought impossible" (Calvin a la TV)

  23. Re: allow benign typedef redefinitions (C11)

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-10-05T17:23:21Z

    I have applied 0002 and 0004, which are the straightforward ones.
I decided not to pursue 0003 because I think a much better idea is to
remove the inclusion of tupconvert.h in execnodes.h.

Here's 0001 again.  I mentioned a possible move of a few instrumentation
structs from elsewhere into the new file, just so that they're all
together in one place.  The new 0002 here does that, but I'm feeling
quite unsure about it.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Uno puede defenderse de los ataques; contra los elogios se esta indefenso"