From: Tom Lane <tgl@sss.pgh.pa.us>
To: =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de>
cc: Peter Eisentraut <peter@eisentraut.org>,
        pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: allow benign typedef redefinitions (C11)
In-reply-to: <202509291356.o5t6ny2hoa3q@alvherre.pgsql>
References: <202509291356.o5t6ny2hoa3q@alvherre.pgsql>
Comments: In-reply-to =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de>
	message dated "Mon, 29 Sep 2025 18:45:52 +0200"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <214790.1759165710.1@sss.pgh.pa.us>
Date: Mon, 29 Sep 2025 13:08:30 -0400
Message-ID: <214791.1759165710@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/214791.1759165710%40sss.pgh.pa.us>
Precedence: bulk

=?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de> writes:
> I was thinking about your (Tom's) idea of having some sort of header
> inclusion policy.  Our current situation is that cross-module inclusions
> are quite widespread and the dependencies can probably be seen as a
> tight web (modules probably being defined as our subdirectories inside
> src/include).  A blanket prohibition of inclusion of headers of other
> modules would certainly not work, but if we imagine that within each
> module we have a hierarchy of sorts, then it would make sense to have a
> policy that other modules can include high level headers of other
> modules, but not lower-level headers.  For instance, it's okay if
> files in src/include/executor include high-level brin.h, but it's not
> okay if it has to include brin_tuple.h which is supposed to be
> lower-level.

Yeah, I've been thinking about that off-and-on, and don't have
anything to present yet either.

> With that in mind, I gave another long stare to the doxygen reports for
> some of these files.  It now seems to me that removing tidbitmap.h from
> genam.h is somewhat bogus, because after all tidbitmap.h is a legitimate
> "library" which is okay to be included in other headers, and the real
> bug here is the fact that only very recently (commit bfe56cdf9a4e in Feb
> 2025) it acquired htup_details.h just in order to be able to define
> TBM_MAX_TUPLES_PER_PAGE.  If we remove htup_details.h from tidbitmap.h,
> and we also remove the inclusion of relcache.h by adding a typedef for
> Relation, then genam.h is a much better behaved header than before.
> Hence the attached patches.

These patches seem fine to me.

> Another thing we should look into is splitting the ObjectType enum out
> of parsenodes.h into a new file of its own.  We have objectaddress.h
> depending on the whole of parsenodes.h just to have that enum, for
> instance.  I think that would be useful.

+1.  I think that primnodes.h and parsenodes.h are always going to
need to be included by a whole lot of things, but to the extent that
we can remove those inclusions from relatively low-level headers,
it can't hurt.

			regards, tom lane