Feedback-ID: ie3de48e3:Fastmail
Date: Tue, 30 Sep 2025 13:16:50 +0200
From: =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de>
To: Peter Eisentraut <peter@eisentraut.org>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>,
	Peter Geoghegan <pg@bowt.ie>, Tom Lane <tgl@sss.pgh.pa.us>
Subject: Re: allow benign typedef redefinitions (C11)
Message-ID: <202509301059.okh22f35e46k@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="4faesyfxvssjzjzi"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <202509191927.uj2ijwmho7nv@alvherre.pgsql>
Archived-At: 
 <https://www.postgresql.org/message-id/202509301059.okh22f35e46k%40alvherre.pgsql>
Precedence: bulk


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

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

--4faesyfxvssjzjzi
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename="0001-Remove-brin-gin_tuple.h-from-tuplesort.h.patch"

From d37153de91e0dec508b19d2e6d8ae8be76f7c2a6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@kurilemu.de>
Date: Tue, 30 Sep 2025 12:36:49 +0200
Subject: [PATCH] Remove brin/gin_tuple.h from tuplesort.h

Because execnodes.h depends on struct definitions that were in genam.h,
which it no longer gets because of the aforementioned removal, split
those struct definitions to a separate header file.
---
 contrib/bloom/blscan.c                     |  1 +
 contrib/pageinspect/gistfuncs.c            |  1 +
 src/backend/access/gin/gininsert.c         |  3 +-
 src/backend/access/gist/gistget.c          |  1 +
 src/backend/access/hash/hashsearch.c       |  1 +
 src/backend/access/nbtree/nbtsearch.c      |  1 +
 src/backend/access/spgist/spgscan.c        |  1 +
 src/backend/catalog/pg_attrdef.c           |  1 +
 src/backend/catalog/pg_largeobject.c       |  1 +
 src/backend/executor/execReplication.c     |  1 +
 src/backend/parser/parse_expr.c            |  1 +
 src/backend/replication/logical/relation.c |  1 +
 src/backend/statistics/attribute_stats.c   |  1 +
 src/include/access/genam.h                 | 24 ++-----------
 src/include/executor/instrument_node.h     | 40 ++++++++++++++++++++++
 src/include/nodes/execnodes.h              |  2 ++
 src/include/utils/tuplesort.h              |  6 ++--
 17 files changed, 63 insertions(+), 24 deletions(-)
 create mode 100644 src/include/executor/instrument_node.h

diff --git a/contrib/bloom/blscan.c b/contrib/bloom/blscan.c
index d072f47fe28..5f8378d1f44 100644
--- a/contrib/bloom/blscan.c
+++ b/contrib/bloom/blscan.c
@@ -14,6 +14,7 @@
 
 #include "access/relscan.h"
 #include "bloom.h"
+#include "executor/instrument_node.h"
 #include "miscadmin.h"
 #include "pgstat.h"
 #include "storage/bufmgr.h"
diff --git a/contrib/pageinspect/gistfuncs.c b/contrib/pageinspect/gistfuncs.c
index 1b299374890..fab6e8d35ad 100644
--- a/contrib/pageinspect/gistfuncs.c
+++ b/contrib/pageinspect/gistfuncs.c
@@ -9,6 +9,7 @@
  */
 #include "postgres.h"
 
+#include "access/genam.h"
 #include "access/gist.h"
 #include "access/htup.h"
 #include "access/relation.h"
diff --git a/src/backend/access/gin/gininsert.c b/src/backend/access/gin/gininsert.c
index e9d4b27427e..43c007b6204 100644
--- a/src/backend/access/gin/gininsert.c
+++ b/src/backend/access/gin/gininsert.c
@@ -29,10 +29,11 @@
 #include "storage/bufmgr.h"
 #include "storage/predicate.h"
 #include "tcop/tcopprot.h"
+#include "utils/builtins.h"
 #include "utils/datum.h"
 #include "utils/memutils.h"
 #include "utils/rel.h"
-#include "utils/builtins.h"
+#include "utils/typcache.h"
 
 
 /* Magic numbers for parallel state sharing */
diff --git a/src/backend/access/gist/gistget.c b/src/backend/access/gist/gistget.c
index 387d9972345..aa56ef430a9 100644
--- a/src/backend/access/gist/gistget.c
+++ b/src/backend/access/gist/gistget.c
@@ -17,6 +17,7 @@
 #include "access/genam.h"
 #include "access/gist_private.h"
 #include "access/relscan.h"
+#include "executor/instrument_node.h"
 #include "lib/pairingheap.h"
 #include "miscadmin.h"
 #include "pgstat.h"
diff --git a/src/backend/access/hash/hashsearch.c b/src/backend/access/hash/hashsearch.c
index 92c15a65be2..00395b01f60 100644
--- a/src/backend/access/hash/hashsearch.c
+++ b/src/backend/access/hash/hashsearch.c
@@ -17,6 +17,7 @@
 #include "access/hash.h"
 #include "access/relscan.h"
 #include "miscadmin.h"
+#include "executor/instrument_node.h"
 #include "pgstat.h"
 #include "storage/predicate.h"
 #include "utils/rel.h"
diff --git a/src/backend/access/nbtree/nbtsearch.c b/src/backend/access/nbtree/nbtsearch.c
index d69798795b4..5c6dc3b183e 100644
--- a/src/backend/access/nbtree/nbtsearch.c
+++ b/src/backend/access/nbtree/nbtsearch.c
@@ -18,6 +18,7 @@
 #include "access/nbtree.h"
 #include "access/relscan.h"
 #include "access/xact.h"
+#include "executor/instrument_node.h"
 #include "miscadmin.h"
 #include "pgstat.h"
 #include "storage/predicate.h"
diff --git a/src/backend/access/spgist/spgscan.c b/src/backend/access/spgist/spgscan.c
index 25893050c58..abfb3555af5 100644
--- a/src/backend/access/spgist/spgscan.c
+++ b/src/backend/access/spgist/spgscan.c
@@ -18,6 +18,7 @@
 #include "access/genam.h"
 #include "access/relscan.h"
 #include "access/spgist_private.h"
+#include "executor/instrument_node.h"
 #include "miscadmin.h"
 #include "pgstat.h"
 #include "storage/bufmgr.h"
diff --git a/src/backend/catalog/pg_attrdef.c b/src/backend/catalog/pg_attrdef.c
index 1b6270b1213..4db4ffd657c 100644
--- a/src/backend/catalog/pg_attrdef.c
+++ b/src/backend/catalog/pg_attrdef.c
@@ -14,6 +14,7 @@
  */
 #include "postgres.h"
 
+#include "access/genam.h"
 #include "access/relation.h"
 #include "access/table.h"
 #include "catalog/dependency.h"
diff --git a/src/backend/catalog/pg_largeobject.c b/src/backend/catalog/pg_largeobject.c
index 89fc8102150..471f82f51a8 100644
--- a/src/backend/catalog/pg_largeobject.c
+++ b/src/backend/catalog/pg_largeobject.c
@@ -14,6 +14,7 @@
  */
 #include "postgres.h"
 
+#include "access/genam.h"
 #include "access/table.h"
 #include "catalog/catalog.h"
 #include "catalog/indexing.h"
diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c
index b409d4ecbf5..ab585aa9819 100644
--- a/src/backend/executor/execReplication.c
+++ b/src/backend/executor/execReplication.c
@@ -14,6 +14,7 @@
 
 #include "postgres.h"
 
+#include "access/amapi.h"
 #include "access/commit_ts.h"
 #include "access/genam.h"
 #include "access/gist.h"
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index e1979a80c19..eebea694dda 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -37,6 +37,7 @@
 #include "utils/fmgroids.h"
 #include "utils/lsyscache.h"
 #include "utils/timestamp.h"
+#include "utils/typcache.h"
 #include "utils/xml.h"
 
 /* GUC parameters */
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index f59046ad620..85f55b75c66 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -29,6 +29,7 @@
 #include "utils/inval.h"
 #include "utils/lsyscache.h"
 #include "utils/syscache.h"
+#include "utils/typcache.h"
 
 
 static MemoryContext LogicalRepRelMapContext = NULL;
diff --git a/src/backend/statistics/attribute_stats.c b/src/backend/statistics/attribute_stats.c
index 1db6a7f784c..4e8efcdf55b 100644
--- a/src/backend/statistics/attribute_stats.c
+++ b/src/backend/statistics/attribute_stats.c
@@ -29,6 +29,7 @@
 #include "utils/fmgroids.h"
 #include "utils/lsyscache.h"
 #include "utils/syscache.h"
+#include "utils/typcache.h"
 
 #define DEFAULT_NULL_FRAC      Float4GetDatum(0.0)
 #define DEFAULT_AVG_WIDTH      Int32GetDatum(0) /* unknown */
diff --git a/src/include/access/genam.h b/src/include/access/genam.h
index 9200a22bd9f..b18a7e4f5db 100644
--- a/src/include/access/genam.h
+++ b/src/include/access/genam.h
@@ -29,27 +29,9 @@ typedef struct TupleTableSlot TupleTableSlot;
 /* or relcache.h */
 typedef struct RelationData *Relation;
 
-
-/*
- * Struct for statistics maintained by amgettuple and amgetbitmap
- *
- * Note: IndexScanInstrumentation can't contain any pointers, since it is
- * copied into a SharedIndexScanInstrumentation during parallel scans
- */
-typedef struct IndexScanInstrumentation
-{
-	/* Index search count (incremented with pgstat_count_index_scan call) */
-	uint64		nsearches;
-} IndexScanInstrumentation;
-
-/*
- * Struct for every worker's IndexScanInstrumentation, stored in shared memory
- */
-typedef struct SharedIndexScanInstrumentation
-{
-	int			num_workers;
-	IndexScanInstrumentation winstrument[FLEXIBLE_ARRAY_MEMBER];
-} SharedIndexScanInstrumentation;
+/* XXX maybe include the other file here instead of this? */
+typedef struct IndexScanInstrumentation IndexScanInstrumentation;
+typedef struct SharedIndexScanInstrumentation SharedIndexScanInstrumentation;
 
 /*
  * Struct for statistics returned by ambuild
diff --git a/src/include/executor/instrument_node.h b/src/include/executor/instrument_node.h
new file mode 100644
index 00000000000..1af07c03a3e
--- /dev/null
+++ b/src/include/executor/instrument_node.h
@@ -0,0 +1,40 @@
+/*-------------------------------------------------------------------------
+ *
+ * instrument_node.h
+ *	  Definitions for node-specific instrumentation
+ *
+ *
+ * Portions Copyright (c) 1996-2025, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/executor/instrument_node.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef INSTRUMENT_NODE_H
+#define INSTRUMENT_NODE_H
+
+
+/*
+ * Struct for statistics maintained by amgettuple and amgetbitmap
+ *
+ * Note: IndexScanInstrumentation can't contain any pointers, since it is
+ * copied into a SharedIndexScanInstrumentation during parallel scans
+ */
+typedef struct IndexScanInstrumentation
+{
+	/* Index search count (incremented with pgstat_count_index_scan call) */
+	uint64		nsearches;
+} IndexScanInstrumentation;
+
+/*
+ * Struct for every worker's IndexScanInstrumentation, stored in shared memory
+ */
+typedef struct SharedIndexScanInstrumentation
+{
+	int			num_workers;
+	IndexScanInstrumentation winstrument[FLEXIBLE_ARRAY_MEMBER];
+} SharedIndexScanInstrumentation;
+
+
+#endif							/* INSTRUMENT_NODE_H */
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index a36653c37f9..4bf733bc8e1 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -29,8 +29,10 @@
 #ifndef EXECNODES_H
 #define EXECNODES_H
 
+#include "access/skey.h"
 #include "access/tupconvert.h"
 #include "executor/instrument.h"
+#include "executor/instrument_node.h"
 #include "fmgr.h"
 #include "lib/ilist.h"
 #include "lib/pairingheap.h"
diff --git a/src/include/utils/tuplesort.h b/src/include/utils/tuplesort.h
index ef79f259f93..8278beea5ec 100644
--- a/src/include/utils/tuplesort.h
+++ b/src/include/utils/tuplesort.h
@@ -21,8 +21,6 @@
 #ifndef TUPLESORT_H
 #define TUPLESORT_H
 
-#include "access/brin_tuple.h"
-#include "access/gin_tuple.h"
 #include "access/itup.h"
 #include "executor/tuptable.h"
 #include "storage/dsm.h"
@@ -31,6 +29,10 @@
 #include "utils/sortsupport.h"
 
 
+/* We don't want this file to depend on AM-specific header files */
+typedef struct BrinTuple BrinTuple;
+typedef struct GinTuple GinTuple;
+
 /*
  * Tuplesortstate and Sharedsort are opaque types whose details are not
  * known outside tuplesort.c.
-- 
2.47.3


--4faesyfxvssjzjzi--