Horton

Re: allow benign typedef redefinitions (C11)

Álvaro Herrera <alvherre@kurilemu.de>

View thread View on postgr.es
From: Álvaro Herrera <alvherre@kurilemu.de>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Peter Eisentraut <peter@eisentraut.org>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2025-09-29T16:45:52Z
Lists: pgsql-hackers

Attachments

Hello,

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.

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.


I've been looking at removing some includes from a few more headers, but
I'm not happy with the overall achievement, or at least I don't have a
good metric to present as a win.  So I'll leave that for another day and
maybe present a different way to look at the problem.  One thing we
should certainly do is fix the inclusion of brin_tuple.h and gin_tuple.h
into tuplesort.h, as I mentioned upthread.  That is definitely a mess,
but I think it requires a new file.

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.  I have the basic patch for
that and I kinda like it, but I want to analyze it a bit more before
posting to avoid rushing to conclusions.

Thanks

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
Thou shalt study thy libraries and strive not to reinvent them without
cause, that thy code may be short and readable and thy days pleasant
and productive. (7th Commandment for C Programmers)

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.