public inbox for [email protected]help / color / mirror / Atom feed
Re: some more include removal from headers 7+ messages / 2 participants [nested] [flat]
* Re: some more include removal from headers @ 2026-03-13 15:53 Andres Freund <[email protected]> 1 sibling, 1 reply; 7+ messages in thread From: Andres Freund @ 2026-03-13 15:53 UTC (permalink / raw) To: Álvaro Herrera <[email protected]>; +Cc: Pg Hackers <[email protected]> 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 protected]> > 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 ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: some more include removal from headers @ 2026-03-13 21:14 Andres Freund <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 0 replies; 7+ messages in thread From: Andres Freund @ 2026-03-13 21:14 UTC (permalink / raw) To: Álvaro Herrera <[email protected]>; +Cc: Pg Hackers <[email protected]> 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 ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: some more include removal from headers @ 2026-04-02 10:52 Álvaro Herrera <[email protected]> 1 sibling, 1 reply; 7+ messages in thread From: Álvaro Herrera @ 2026-04-02 10:52 UTC (permalink / raw) To: Pg Hackers <[email protected]> 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/ ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: some more include removal from headers @ 2026-04-02 16:27 Andres Freund <[email protected]> parent: Álvaro Herrera <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Andres Freund @ 2026-04-02 16:27 UTC (permalink / raw) To: Álvaro Herrera <[email protected]>; +Cc: Pg Hackers <[email protected]> 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]. 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 ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: some more include removal from headers @ 2026-04-02 18:24 Álvaro Herrera <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Álvaro Herrera @ 2026-04-02 18:24 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Pg Hackers <[email protected]> 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/ ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: some more include removal from headers @ 2026-04-02 20:44 Andres Freund <[email protected]> parent: Álvaro Herrera <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Andres Freund @ 2026-04-02 20:44 UTC (permalink / raw) To: Álvaro Herrera <[email protected]>; +Cc: Pg Hackers <[email protected]> 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 ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: some more include removal from headers @ 2026-04-02 22:14 Álvaro Herrera <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 0 replies; 7+ messages in thread From: Álvaro Herrera @ 2026-04-02 22:14 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Pg Hackers <[email protected]> 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) ^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2026-04-02 22:14 UTC | newest] Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-03-13 15:53 ` Andres Freund <[email protected]> 2026-03-13 21:14 ` Andres Freund <[email protected]> 2026-04-02 10:52 ` Álvaro Herrera <[email protected]> 2026-04-02 16:27 ` Andres Freund <[email protected]> 2026-04-02 18:24 ` Álvaro Herrera <[email protected]> 2026-04-02 20:44 ` Andres Freund <[email protected]> 2026-04-02 22:14 ` Álvaro Herrera <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox