Subject: Re: some more include removal from headers

From: Andres Freund <[email-masked]>
Date: Fri, 13 Mar 2026 11:53:00 -0400

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?= <[email-masked]>
> 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

---

From: Andres Freund <[email-masked]>
Date: Fri, 13 Mar 2026 17:14:03 -0400

Hi,

On 2026-03-13 20:32:53 +0100, Álvaro Herrera wrote:
> On 2026-Mar-13, Andres Freund wrote:
> 
> 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.

> 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...

> Whoa, nice!

Thanks.

> 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.

> 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.

> 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

---

From: Álvaro Herrera <[email-masked]>
Date: Thu, 2 Apr 2026 12:52:13 +0200

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/

---

From: Andres Freund <[email-masked]>
Date: Thu, 2 Apr 2026 12:27:12 -0400

Hi,

> 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].

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].

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] https://postgr.es/m/vlcexdcimsmvu3aplt2yxpfndkgtuvjsrms2fdl46rbw3k2kug%40drspkoxlaije

---

From: Álvaro Herrera <[email-masked]>
Date: Thu, 2 Apr 2026 20:24:29 +0200

> complained about the issue in [1].

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

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

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/

---

From: Andres Freund <[email-masked]>
Date: Thu, 2 Apr 2026 16:44:41 -0400

Hi,

> needs the ReadBufferMode enum, so in order to do better, we'd have to

I think you may have forgotten to attach the changes?

Greetings,

Andres Freund

---

From: Álvaro Herrera <[email-masked]>
Date: Fri, 3 Apr 2026 00:14:51 +0200

Hello,

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