On Tue, 26 Jul 2022 at 09:20, Michael Paquier <mich...@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
v8-0001-Add-protections-in-xlog-record-APIs-against-large.patch
Description: Binary data
