On Mon, Jun 20, 2022 at 11:01:51AM +0200, Matthias van de Meent wrote: > On Mon, 20 Jun 2022 at 07:02, Michael Paquier <mich...@paquier.xyz> wrote: >> + if (unlikely(!AllocSizeIsValid(total_len))) >> + XLogErrorDataLimitExceeded(); >> Rather than a single check at the end of XLogRecordAssemble(), you'd >> better look after that each time total_len is added up? > > I was doing so previously, but there were some good arguments against that: > > - Performance of XLogRecordAssemble should be impacted as little as > possible. XLogRecordAssemble is in many hot paths, and it is highly > unlikely this check will be hit, because nobody else has previously > reported this issue. Any check, however unlikely, will add some > overhead, so removing check counts reduces overhead of this patch.
Some macro-benchmarking could be in place here, and this would most likely become noticeable when assembling a bunch of little records? > - The user or system is unlikely to care about which specific check > was hit, and only needs to care _that_ the check was hit. An attached > debugger will be able to debug the internals of the xlog machinery and > find out the specific reasons for the error, but I see no specific > reason why the specific reason would need to be reported to the > connection. Okay. + /* + * Ensure that xlogreader.c can read the record by ensuring that the + * data section of the WAL record can be allocated. + */ + if (unlikely(!AllocSizeIsValid(total_len))) + XLogErrorDataLimitExceeded(); By the way, while skimming through the patch, the WAL reader seems to be a bit more pessimistic than this estimation, calculating the amount to allocate as of DecodeXLogRecordRequiredSpace(), based on the xl_tot_len given by a record. -- Michael
signature.asc
Description: PGP signature
