From: | Álvaro Herrera <alvherre(at)kurilemu(dot)de> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Peter Eisentraut <peter(at)eisentraut(dot)org>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: allow benign typedef redefinitions (C11) |
Date: | 2025-09-29 16:45:52 |
Message-ID: | 202509291356.o5t6ny2hoa3q@alvherre.pgsql |
Views: | Whole Thread | Raw Message | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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)
Attachment | Content-Type | Size |
---|---|---|
0001-Remove-genam.h-s-dependency-on-relcache.h.patch | text/x-diff | 2.7 KB |
0002-don-t-include-htup_details.h-in-tidbitmap.h.patch | text/x-diff | 1.0 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2025-09-29 17:08:30 | Re: allow benign typedef redefinitions (C11) |
Previous Message | Arseniy Mukhin | 2025-09-29 16:34:47 | Re: Recovering from detoast-related catcache invalidations |