Feedback-ID: id4a34324:Fastmail
Date: Fri, 13 Mar 2026 17:14:03 -0400
From: Andres Freund <andres@anarazel.de>
To: =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de>
Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: some more include removal from headers
Message-ID: <qox6yqkety63epa4puqc7qm2f4dzs72s6ejtxbf277yamb43ne@rd2u267htgzm>
References: <j32gzsc32267k4phni2zf5yjikpyfjbqvup22bvloqga6svvkz@ao3c6ygpmz5p>
 <202603131615.hwsjm4aq77pq@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <202603131615.hwsjm4aq77pq@alvherre.pgsql>
Archived-At: 
 <https://www.postgresql.org/message-id/qox6yqkety63epa4puqc7qm2f4dzs72s6ejtxbf277yamb43ne%40rd2u267htgzm>
Precedence: bulk

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