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
