Re: split TOAST support out of postgres.h

On Tue, Jan 10, 2023 at 12:00:46PM -0500, Robert Haas wrote:
> On Tue, Jan 10, 2023 at 9:46 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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).