Horton

Re: allow benign typedef redefinitions (C11)

Tom Lane <tgl@sss.pgh.pa.us>

View thread View on postgr.es
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Álvaro Herrera <alvherre@kurilemu.de>
Cc: Peter Eisentraut <peter@eisentraut.org>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2025-09-29T17:08:30Z
Lists: pgsql-hackers 
=?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

Commits

  1. Move instrumentation-related structs to instrument_node.h

  2. Don't include access/htup_details.h in executor/tuptable.h

  3. Don't include execnodes.h in brin.h or gin.h

  4. Do a tiny bit of header file maintenance

  5. Don't include execnodes.h in replication/conflict.h

  6. Update some more forward declarations to use typedef

  7. Change fmgr.h typedefs to use original names

  8. Improve ExplainState type handling in header files

  9. Remove hbaPort type

  10. Remove workarounds against repeat typedefs

  11. Update various forward declarations to use typedef

  12. Allow redeclaration of typedef yyscan_t

  13. Improve pgbench definition of yyscan_t

  14. Delay extraction of TIDBitmap per page offsets

  15. Remove pgrminclude and associated scripts

  16. Allow to use HeapTupleData embedded in [Buffer]HeapTupleTableSlot.