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)
>From 75b7c0c7a270066ab8ccd448265b6dddd7224d09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <[email protected]> Date: Sun, 28 Sep 2025 11:15:51 +0200 Subject: [PATCH 1/2] Remove genam.h's dependency on relcache.h --- contrib/btree_gist/btree_bit.c | 1 + src/backend/utils/adt/network_spgist.c | 1 + src/include/access/amapi.h | 2 ++ src/include/access/genam.h | 4 +++- src/include/access/nbtree.h | 1 + 5 files changed, 8 insertions(+), 1 deletion(-) diff --git a/contrib/btree_gist/btree_bit.c b/contrib/btree_gist/btree_bit.c index 0df2ae20d8b..9199f886097 100644 --- a/contrib/btree_gist/btree_bit.c +++ b/contrib/btree_gist/btree_bit.c @@ -8,6 +8,7 @@ #include "utils/fmgrprotos.h" #include "utils/sortsupport.h" #include "utils/varbit.h" +#include "varatt.h" /* GiST support functions */ PG_FUNCTION_INFO_V1(gbt_bit_compress); diff --git a/src/backend/utils/adt/network_spgist.c b/src/backend/utils/adt/network_spgist.c index 602276a35c3..a84747d9275 100644 --- a/src/backend/utils/adt/network_spgist.c +++ b/src/backend/utils/adt/network_spgist.c @@ -37,6 +37,7 @@ #include "catalog/pg_type.h" #include "utils/fmgrprotos.h" #include "utils/inet.h" +#include "varatt.h" static int inet_spg_node_number(const inet *val, int commonbits); diff --git a/src/include/access/amapi.h b/src/include/access/amapi.h index 2b4482dc1e6..63dd41c1f21 100644 --- a/src/include/access/amapi.h +++ b/src/include/access/amapi.h @@ -15,6 +15,8 @@ #include "access/cmptype.h" #include "access/genam.h" #include "access/stratnum.h" +#include "nodes/nodes.h" +#include "nodes/pg_list.h" /* * We don't wish to include planner header files here, since most of an index diff --git a/src/include/access/genam.h b/src/include/access/genam.h index ac62f6a6abd..9200a22bd9f 100644 --- a/src/include/access/genam.h +++ b/src/include/access/genam.h @@ -20,13 +20,15 @@ #include "nodes/tidbitmap.h" #include "storage/buf.h" #include "storage/lockdefs.h" -#include "utils/relcache.h" #include "utils/snapshot.h" /* We don't want this file to depend on execnodes.h. */ typedef struct IndexInfo IndexInfo; typedef struct TupleTableSlot TupleTableSlot; +/* or relcache.h */ +typedef struct RelationData *Relation; + /* * Struct for statistics maintained by amgettuple and amgetbitmap diff --git a/src/include/access/nbtree.h b/src/include/access/nbtree.h index 9ab467cb8fd..db1345f54c8 100644 --- a/src/include/access/nbtree.h +++ b/src/include/access/nbtree.h @@ -22,6 +22,7 @@ #include "catalog/pg_index.h" #include "lib/stringinfo.h" #include "storage/bufmgr.h" +#include "storage/dsm.h" #include "storage/shm_toc.h" #include "utils/skipsupport.h" -- 2.47.3
>From 79dabe8fa189be1ee084e60a1c1b2bf68fbbed3e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <[email protected]> Date: Mon, 29 Sep 2025 15:52:06 +0200 Subject: [PATCH 2/2] don't include htup_details.h in tidbitmap.h --- src/backend/nodes/tidbitmap.c | 1 + src/include/nodes/tidbitmap.h | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c index 41031aa8f2f..fac2ba5d0ca 100644 --- a/src/backend/nodes/tidbitmap.c +++ b/src/backend/nodes/tidbitmap.c @@ -40,6 +40,7 @@ #include <limits.h> +#include "access/htup_details.h" #include "common/hashfn.h" #include "common/int.h" #include "nodes/bitmapset.h" diff --git a/src/include/nodes/tidbitmap.h b/src/include/nodes/tidbitmap.h index 99f795ceab5..f54e61c7190 100644 --- a/src/include/nodes/tidbitmap.h +++ b/src/include/nodes/tidbitmap.h @@ -22,7 +22,6 @@ #ifndef TIDBITMAP_H #define TIDBITMAP_H -#include "access/htup_details.h" #include "storage/itemptr.h" #include "utils/dsa.h" -- 2.47.3
