On Tue, 26 Jul 2022 at 09:20, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Mon, Jul 25, 2022 at 02:12:05PM +0300, Heikki Linnakangas wrote:
> > Among the two problems to solve at hand, the parts where the APIs are
> > changed and made more robust with unsigned types and where block data
> > is not overflowed with its 16-byte limit are committable, so I'd like
> > to do that first (still need to check its performance with some micro
> > benchmark on XLogRegisterBufData()).
> >
> > +1. I'm not excited about adding the "unlikely()" hints, though. We have a
> > pg_attribute_cold hint in ereport(), that should be enough.
>
> Okay, that makes sense. FWIW, I have been wondering about the
> addition of the extra condition in XLogRegisterBufData() and I did not
> see a difference on HEAD in terms of execution time or profile, with a
> micro-benchmark doing a couple of million calls in a row as of the
> following, roughly:
> [...]
Thanks for testing.
> > How large exactly is the maximum size that this gives? I'd prefer to set the
> > limit conservatively to 1020 MB, for example, with a compile-time static
> > assertion that AllocSizeIsValid(DecodeXLogRecordRequiredSpace(1020 MB)).
>
> Something like that would work, I guess.
I've gone over the patch and reviews again, and updated those places
that received comments:
- updated the MaxXLogRecordSize and XLogRecordLengthIsValid(len)
macros (now in xlogrecord.h), with a max length of the somewhat
arbitrary 1020MiB.
This leaves room for approx. 4MiB of per-record allocation overhead
before you'd hit MaxAllocSize, and also detaches the dependency on
memutils.h.
- Retained the check in XLogRegisterData, so that we check against
integer overflows in the registerdata code instead of only an assert
in XLogRecordAssemble where it might be too late.
- Kept the inline static elog-ing function (as per Andres' suggestion
on 2022-03-14; this decreases binary sizes)
- Dropped any changes in xlogreader.h/c
Kind regards,
Matthias van de Meent