Feedback-ID: ie3de48e3:Fastmail
Date: Mon, 29 Sep 2025 18:45:52 +0200
From: =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Peter Eisentraut <peter@eisentraut.org>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: allow benign typedef redefinitions (C11)
Message-ID: <202509291356.o5t6ny2hoa3q@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="o5h4duhcv5ktxckg"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <202509281643.jcbwanjuy7yh@alvherre.pgsql>
Archived-At: 
 <https://www.postgresql.org/message-id/202509291356.o5t6ny2hoa3q%40alvherre.pgsql>
Precedence: bulk


--o5h4duhcv5ktxckg
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

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)

--o5h4duhcv5ktxckg
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename="0001-Remove-genam.h-s-dependency-on-relcache.h.patch"

From 75b7c0c7a270066ab8ccd448265b6dddd7224d09 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@kurilemu.de>
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


--o5h4duhcv5ktxckg
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename="0002-don-t-include-htup_details.h-in-tidbitmap.h.patch"

From 79dabe8fa189be1ee084e60a1c1b2bf68fbbed3e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@kurilemu.de>
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


--o5h4duhcv5ktxckg--