Re: Non-replayable WAL records through overflows and >MaxAllocSize lengths

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 <michael(at)paquier(dot)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