On Tue, Jan 10, 2023 at 12:00:46PM -0500, Robert Haas wrote: > On Tue, Jan 10, 2023 at 9:46 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > > Now, there is a fair question whether splitting this code out of > > postgres.h is worth any trouble at all. TBH my initial reaction > > had been "no". But once we found that only 40-ish backend files > > need to read this new header, I became a "yes" vote because it > > seems clear that there will be a total-compilation-time benefit.
A time claim with no benchmarks is a red flag. I've chosen to run one: export CCACHE_DISABLE=1 change=d952373a987bad331c0e499463159dd142ced1ef for commit in $change $change^; do echo === git checkout $commit git checkout $commit for n in `seq 1 200`; do make -j20 clean; env time make -j20; done done Results: commit median mean count d952373a987bad331c0e499463159dd142ced1ef 49.35 49.37 200 d952373a987bad331c0e499463159dd142ced1ef^ 49.33 49.36 200 That is to say, the patch made the build a bit slower, not faster. That's with GCC 4.8.5 (RHEL 7). I likely should have interleaved the run types, but in any case the speed win didn't show up. > I wasn't totally about this, either, but I think on balance it's > probably a smart thing to do. I always found it kind of weird that we > put that stuff in postgres.h. It seems to privilege the TOAST > mechanism to an undue degree; what's the argument, for example, that > TOAST macros are more generally relevant than CHECK_FOR_INTERRUPTS() > or LWLockAcquire or HeapTuple? It felt to me like we'd just decided > that one subsystem gets to go into the main header file and everybody > else just had to have their own headers. > > One thing that's particularly awkward about that is that if you want > to write some front-end code that knows about how varlenas are stored > on disk, it was very awkward with the old structure. You're not > supposed to include "postgres.h" into frontend code, but if the stuff > you need is defined there then what else can you do? So I generally > think that anything that's in postgres.h should have a strong claim to > be not only widely-needed in the backend, but also never needed at all > in any other executable. If the patch had just made postgres.h include varatt.h, like it does elog.h, I'd consider that change a nonnegative. Grouping things is nice, even if it makes compilation a bit slower. That also covers your frontend use case. How about that? I agree fixing any one extension is easy enough. Thinking back to the htup_details.h refactor, I found the aggregate pain unreasonable relative to alleged benefits, even though each individual extension wasn't too bad. SET_VARSIZE is used more (74 pgxn distributions) than htup_details.h (62).