some more include removal from headers

Hi,

On 2026-03-13 14:05:24 +0100, Álvaro Herrera wrote:

execnodes.h is a very large source of other headers for no very good
reasons anymore. Fortunately there's a few of the files it includes
that we can remove very easily with just a small number of additional
typedefs. Some proposed patches attached; it's all very
straightforward, just add typedefs for structs
Tuplesortstate
Tuplestorestate
TupleConversionMap
TupleTableSlot
TupleTableSlotOps
TIDBitmap
This also requires to add some headers to a bunch of .c files, which is
a good indicator that we're making progress.

It's especially nice when the new #include line we have to add in some
.c file is not the one that was removed from the .h file -- for instance
in 0001 we have to add pg_type_d.h when removing tuplestore.h/
tuplesort.h, and if you look at the chart here
https://doxygen.postgresql.org/tuplesort_8h.html it becomes very clear
we're saving quite a lot of useless indirect inclusions. (This is also
seen in 0004: we remove tuptable.h and have to add sysattr.h to three .c
files).

Nice.

From 47d5e30b00ce8c0c37c8b905223f1b70a5020bfa Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@kurilemu.de>
Date: Thu, 5 Mar 2026 18:00:54 +0100
Subject: [PATCH 1/6] remove tuplestore.h and tuplesort.h from execnodes.h

diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index 31e19fbc697..ada782f98f5 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -29,6 +29,7 @@
#include "utils/builtins.h"
#include "utils/fmgroids.h"
#include "utils/rel.h"
+#include "utils/tuplestore.h"

PG_FUNCTION_INFO_V1(verify_heapam);

Kinda wonder if funcapi.h should export tuplestore.h. That'd avoid needing
adding the dedicated tuplestore.h in so many places, and as the use of
tuplestore is basically required for funcapi.h users, it seems like it'd be
fine semantically?

But it also doesn't really matter.

If we could go back in time I'd wrap the tuplestore_puttuple() for the SRF use
case into an SRF specific wrapper, but my time travel capabilities have not
developed, despite no lack of trying.

Subject: [PATCH 2/6] remove tupconvert.h from execnodes.h

diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
index b259c4141ed..e475e278aa8 100644
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -14,6 +14,7 @@
#ifndef INDEX_H
#define INDEX_H

+#include "access/attmap.h"
#include "catalog/objectaddress.h"
#include "nodes/execnodes.h"

A bit sad to include attmap.h here. Looks like it'd not be hard to instead
forward declare AttrMap instead?

I've attached a version of your patches that does so in a followup commit.

diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 41ac0259b32..8c03498180f 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -30,9 +30,9 @@
#define EXECNODES_H

#include "access/skey.h"
-#include "access/tupconvert.h"
#include "executor/instrument.h"
#include "executor/instrument_node.h"
+#include "executor/tuptable.h"
#include "fmgr.h"
#include "lib/ilist.h"
#include "lib/pairingheap.h"
@@ -58,6 +58,7 @@ typedef struct ExprState ExprState;
typedef struct ExprContext ExprContext;
typedef struct Tuplesortstate Tuplesortstate;
typedef struct Tuplestorestate Tuplestorestate;
+typedef struct TupleConversionMap TupleConversionMap;

I was about to complain about growing that tuptable.h include, but I see
that's transient...

LGTM.

Subject: [PATCH 3/6] don't include sharedtuplestore.h in execnodes.h

LGTM, certainly the missing fd.h includes seem like structurally better.

Subject: [PATCH 4/6] don't include tuptable.h in execnodes.h

LGTM. Smaller after the patch to not include attmap.h that I added above, as
that also triggered needing to add those sysattr.h includes.

Subject: [PATCH 5/6] don't include tidbitmap.h in execnodes.h

diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 82bd5dcb683..652cc316067 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -39,10 +39,10 @@
#include "nodes/miscnodes.h"
#include "nodes/params.h"
#include "nodes/plannodes.h"
-#include "nodes/tidbitmap.h"
#include "partitioning/partdefs.h"
#include "storage/buf.h"
#include "storage/condition_variable.h"
+#include "utils/dsa.h"
#include "utils/hsearch.h"
#include "utils/queryenvironment.h"
#include "utils/reltrigger.h"
@@ -56,6 +56,7 @@ typedef struct PlanState PlanState;
typedef struct ExecRowMark ExecRowMark;
typedef struct ExprState ExprState;
typedef struct ExprContext ExprContext;
+typedef struct TIDBitmap TIDBitmap;
typedef struct Tuplesortstate Tuplesortstate;
typedef struct Tuplestorestate Tuplestorestate;
typedef struct TupleConversionMap TupleConversionMap;
-- 
2.47.3

Sad to add dsa.h this widely. But I don't immediately see a better way, at
least not as part of this commit.

LGTM.

The need for dsa.h and condition_variable.h just is from
ParallelBitmapHeapState - which isn't actually an executor node and never
needed outside of nodeBitmapHeapscan.c - so it seems better to move it there?

Added a commit for that.

Your patch numbering says 5/6, but there's only 5 attached, I assume that was
intentional?

I couldn't help myself to slim down execnodes.h further. Not sure if all of
them are quite worth it.

With all the commits combined very little low-level stuff is still
included. The worst is probably instr_time.h.

Greetings,

Andres Freund

Hi,

On 2026-Mar-13, Andres Freund wrote:

Kinda wonder if funcapi.h should export tuplestore.h. That'd avoid needing
adding the dedicated tuplestore.h in so many places, and as the use of
tuplestore is basically required for funcapi.h users, it seems like it'd be
fine semantically?

Hmm. I think there are plenty of SRF functions that use the
value-per-call mechanics instead of a tuplestore (which don't need
tuplestore.h), but I wouldn't be opposed to doing that. I was going to
complain about that change dragging tuptable.h into funcapi.h -- until I
realized that funcapi.h already includes tuptable.h directly itself. So
I don't see any downsides.

If we could go back in time I'd wrap the tuplestore_puttuple() for the SRF use
case into an SRF specific wrapper, but my time travel capabilities have not
developed, despite no lack of trying.

hah! (We could still change this now, and while it wouldn't change
older code or existing third party extensions, it might definitely make
things better going forward; the future being longer than the past, this
might be good anyhow. But that's a matter for a separate thread.)

The need for dsa.h and condition_variable.h just is from
ParallelBitmapHeapState - which isn't actually an executor node and never
needed outside of nodeBitmapHeapscan.c - so it seems better to move it there?

Added a commit for that.

Whoa, nice!

Your patch numbering says 5/6, but there's only 5 attached, I assume that was
intentional?

Yeah, the last one was about removing tidbitmap.h from genam.h -- I left
it out at the last minute because it's unrelated to execnodes.h.
Attached here.

I couldn't help myself to slim down execnodes.h further. Not sure if all of
them are quite worth it.

The relpath.h one is definitely a good win. Not sure about
stringinfo.h, which doesn't change very often and doesn't include
anything else. I'm doubtful about the bitmapset.h removal gaining us
much either (because the really nasty one below, nodetags.h, is
unavoidable anyway.)

With all the commits combined very little low-level stuff is still
included. The worst is probably instr_time.h.

Hm, that one is worth thinking about. But with only this much, it's
already a huge win.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"No nos atrevemos a muchas cosas porque son difíciles,
pero son difíciles porque no nos atrevemos a hacerlas" (Séneca)

Hi,

On 2026-03-13 20:32:53 +0100, Álvaro Herrera wrote:

On 2026-Mar-13, Andres Freund wrote:

Kinda wonder if funcapi.h should export tuplestore.h. That'd avoid needing
adding the dedicated tuplestore.h in so many places, and as the use of
tuplestore is basically required for funcapi.h users, it seems like it'd be
fine semantically?

Hmm. I think there are plenty of SRF functions that use the
value-per-call mechanics instead of a tuplestore (which don't need
tuplestore.h), but I wouldn't be opposed to doing that. I was going to
complain about that change dragging tuptable.h into funcapi.h -- until I
realized that funcapi.h already includes tuptable.h directly itself. So
I don't see any downsides.

Cool.

If we could go back in time I'd wrap the tuplestore_puttuple() for the SRF use
case into an SRF specific wrapper, but my time travel capabilities have not
developed, despite no lack of trying.

hah! (We could still change this now, and while it wouldn't change
older code or existing third party extensions, it might definitely make
things better going forward; the future being longer than the past, this
might be good anyhow. But that's a matter for a separate thread.)

Agreed...

The need for dsa.h and condition_variable.h just is from
ParallelBitmapHeapState - which isn't actually an executor node and never
needed outside of nodeBitmapHeapscan.c - so it seems better to move it there?

Added a commit for that.

Whoa, nice!

Thanks.

Your patch numbering says 5/6, but there's only 5 attached, I assume that was
intentional?

Yeah, the last one was about removing tidbitmap.h from genam.h -- I left
it out at the last minute because it's unrelated to execnodes.h.
Attached here.

Nice improvement. LGTM.

I couldn't help myself to slim down execnodes.h further. Not sure if all of
them are quite worth it.

The relpath.h one is definitely a good win.

Yea.

Not sure about stringinfo.h, which doesn't change very often and doesn't
include anything else.

It seemed worth it because it's not used by plannodes.h, it's a leftover from
before a05dc4d7fd5.

I'm doubtful about the bitmapset.h removal gaining
us much either (because the really nasty one below, nodetags.h, is
unavoidable anyway.)

Yea, I was very much on the fence with that one.

With all the commits combined very little low-level stuff is still
included. The worst is probably instr_time.h.

Hm, that one is worth thinking about. But with only this much, it's
already a huge win.

Agreed, we can tackle that one separately. There's multiple paths for it too,
e.g. a separate header for the instr_time type or moving towards not not
needing to include instrument[_node].h.

How would you like to work towards committing these? I'm am more than happy
for you to commit everything, but I could also help.

Greetings,

Andres Freund

Hello,

On 2026-Mar-13, Andres Freund wrote:

On 2026-03-13 20:32:53 +0100, Álvaro Herrera wrote:

Not sure about stringinfo.h, which doesn't change very often and doesn't
include anything else.

It seemed worth it because it's not used by plannodes.h, it's a leftover from
before a05dc4d7fd5.

Oh, interesting. It's fine with me of course.

I'm doubtful about the bitmapset.h removal gaining
us much either (because the really nasty one below, nodetags.h, is
unavoidable anyway.)

Yea, I was very much on the fence with that one.

Let's leave it out for now. We can always think about it later.

Agreed, we can tackle that one separately. There's multiple paths for it too,
e.g. a separate header for the instr_time type or moving towards not not
needing to include instrument[_node].h.

Right ...

How would you like to work towards committing these? I'm am more than happy
for you to commit everything, but I could also help.

I can try to find time over the weekend to push it all, assuming a
single commit is okay. If we want more than one then I'm afraid it'd
have to wait until sometime next week. But I don't think that is
justified.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"I love the Postgres community. It's all about doing things _properly_. :-)"
(David Garamond)

Hi,

This is what it would look like as a single commit. It passes
headerscheck and compiles clean for me. CI run in progress:
https://cirrus-ci.com/build/4615771982135296

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"La rebeldía es la virtud original del hombre" (Arthur Schopenhauer)

On 2026-Mar-15, Álvaro Herrera wrote:

Hi,

This is what it would look like as a single commit. It passes
headerscheck and compiles clean for me. CI run in progress:
https://cirrus-ci.com/build/4615771982135296

Failed in an interesting fashion in macOS and FreeBSD:

[09:14:52.868] ccache cc -Isrc/backend/postgres_lib.a.p -Isrc/include -I../src/include -I/usr/local/include -I/usr/local/include/libxml2 -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -O0 -g -fno-strict-aliasing -fwrapv -fexcess-precision=standard -Wmissing-prototypes -Wpointer-arith -Werror=vla -Werror=unguarded-availability-new -Wendif-labels -Wmissing-format-attribute -Wcast-function-type -Wformat-security -Wimplicit-fallthrough -Wdeclaration-after-statement -Wmissing-variable-declarations -Wno-unused-command-line-argument -Wno-compound-token-split-by-macro -Wno-format-truncation -Wno-cast-function-type-strict -Og -ggdb -DRELCACHE_FORCE_RELEASE -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS -fPIC -pthread -DBUILDING_DLL -MD -MQ src/backend/postgres_lib.a.p/storage_ipc_shmem.c.o -MF src/backend/postgres_lib.a.p/storage_ipc_shmem.c.o.d -o src/backend/postgres_lib.a.p/storage_ipc_shmem.c.o -c ../src/backend/storage/ipc/shmem.c
[09:14:52.869] ../src/backend/storage/ipc/shmem.c:748:17: warning: call to undeclared function 'sysconf'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
[09:14:52.869] 748 | os_page_size = sysconf(_SC_PAGESIZE);
[09:14:52.869] | ^
[09:14:52.869] ../src/backend/storage/ipc/shmem.c:748:25: error: use of undeclared identifier '_SC_PAGESIZE'
[09:14:52.869] 748 | os_page_size = sysconf(_SC_PAGESIZE);
[09:14:52.869] | ^
[09:14:52.869] 1 warning and 1 error generated.

AFAICS this is because shmem.c used to get #include <unistd.h>
indirectly through pg_iovec.h and no longer does. Added that. Next run:
https://cirrus-ci.com/build/5651549315137536

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"It takes less than 2 seconds to get to 78% complete; that's a good sign.
A few seconds later it's at 90%, but it seems to have stuck there. Did
somebody make percentages logarithmic while I wasn't looking?"
http://smylers.hates-software.com/2005/09/08/1995c749.html

I pushed this now, after also removing the #include instrument.h from
execnodes.h.

One I purposefully didn't remove from execnodes is reltrigger.h, though
it's quite an easy change. I think what we should do here is rethink
the relationship between rel.h, trigger.h and reltrigger.h. It looks
weird to me that structs like TriggerData (used for firing triggers) are
in a header file under src/include/commands. Maybe revert b93f5a5673b4
entirely.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Pido que me den el Nobel por razones humanitarias" (Nicanor Parra)

Hello,

Here's low-hanging fruit I noticed while eyeballing a patch in the
commitfest. This removes pg_publication.h from utils/rel.h, which is
really nice because pg_publication includes objectaddress.h which in
turn includes parsenodes.h (an absolute disaster we have there, harder
to cleanup; not in the mood). But this here is nice and well contained.

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

Hi,

On 2026-04-02 12:52:13 +0200, Álvaro Herrera wrote:

Here's low-hanging fruit I noticed while eyeballing a patch in the
commitfest. This removes pg_publication.h from utils/rel.h, which is
really nice because pg_publication includes objectaddress.h which in
turn includes parsenodes.h (an absolute disaster we have there, harder
to cleanup; not in the mood). But this here is nice and well contained.

These days including rel.h (indirectly) including objectaddress.h is
quite the disaster, because objectaddress.h recently grew an include of
syscache.h, as I complain about in [1]/messages/by-id/vlcexdcimsmvu3aplt2yxpfndkgtuvjsrms2fdl46rbw3k2kug@drspkoxlaije.

I got pretty annoyed this cycle with how much rebuilding a simple change to
bufmgr.h triggers (due to hacking on it a lot). I started to write a series
to improve that, but didn't get around to posting that yet due to encountering
the issue 771fe0948ca fixed while improving the situation.

During that I encountered the objectaddress.h include, as part of which I then
complained about the issue in [1]/messages/by-id/vlcexdcimsmvu3aplt2yxpfndkgtuvjsrms2fdl46rbw3k2kug@drspkoxlaije.

After the attached patches, a change to bufmgr.h triggers rebuilding 213
files, before it was 323. Not perfect, but better.

Would be nice to get rid of the bufmgr.h includes in access/nbtree.h and such,
but it looks like that'd be a bit more work.

I included your catalog/publication.h in it, as my version had surprisingly
extensive bitrot...

Greetings,

Andres Freund

[1]: /messages/by-id/vlcexdcimsmvu3aplt2yxpfndkgtuvjsrms2fdl46rbw3k2kug@drspkoxlaije

On 2026-Apr-02, Andres Freund wrote:

I got pretty annoyed this cycle with how much rebuilding a simple change to
bufmgr.h triggers (due to hacking on it a lot). I started to write a series
to improve that, but didn't get around to posting that yet due to encountering
the issue 771fe0948ca fixed while improving the situation.

During that I encountered the objectaddress.h include, as part of which I then
complained about the issue in [1].

Ah yeah, I noticed and was annoyed by that too.

After the attached patches, a change to bufmgr.h triggers rebuilding 213
files, before it was 323. Not perfect, but better.

Would be nice to get rid of the bufmgr.h includes in access/nbtree.h and such,
but it looks like that'd be a bit more work.

I included your catalog/publication.h in it, as my version had surprisingly
extensive bitrot...

I ran each patch individually under headerscheck and a full tree
compile; they all pass for me. Also, each change is sensible on its own.

Looking at what else includes bufmgr.h, I think the minimum it can
reduced to is compiling 157 files when you change bufmgr.h, per the
patches attached here. Most of them are direct inclusions, so reducing
further is tough. The only one we could blame is xlogutils.h, but it
needs the ReadBufferMode enum, so in order to do better, we'd have to
split bufmgr.h in two.

(A "fun" one is logicalproto.h being included by walreceiver.h, in turn
being included by slot.h, in turn being included by logical.h, in turn
being included by decode.h.)

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/

Hi,

On 2026-04-02 20:24:29 +0200, Álvaro Herrera wrote:

On 2026-Apr-02, Andres Freund wrote:

After the attached patches, a change to bufmgr.h triggers rebuilding 213
files, before it was 323. Not perfect, but better.

Would be nice to get rid of the bufmgr.h includes in access/nbtree.h and such,
but it looks like that'd be a bit more work.

I included your catalog/publication.h in it, as my version had surprisingly
extensive bitrot...

I ran each patch individually under headerscheck and a full tree
compile; they all pass for me. Also, each change is sensible on its own.

Looking at what else includes bufmgr.h, I think the minimum it can
reduced to is compiling 157 files when you change bufmgr.h, per the
patches attached here. Most of them are direct inclusions, so reducing
further is tough. The only one we could blame is xlogutils.h, but it
needs the ReadBufferMode enum, so in order to do better, we'd have to
split bufmgr.h in two.

I think you may have forgotten to attach the changes?

Greetings,

Andres Freund

Hello,

On 2026-Apr-02, Andres Freund wrote:

On 2026-04-02 20:24:29 +0200, Álvaro Herrera wrote:

Looking at what else includes bufmgr.h, I think the minimum it can
reduced to is compiling 157 files when you change bufmgr.h, per the
patches attached here. Most of them are direct inclusions, so reducing
further is tough. The only one we could blame is xlogutils.h, but it
needs the ReadBufferMode enum, so in order to do better, we'd have to
split bufmgr.h in two.

I think you may have forgotten to attach the changes?

Hm, so I did. Here they are.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Las mujeres son como hondas: mientras más resistencia tienen,
más lejos puedes llegar con ellas" (Jonas Nightingale, Leap of Faith)