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

On Wed, 27 Jul 2022 at 11:09, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Tue, Jul 26, 2022 at 06:58:02PM +0200, Matthias van de Meent wrote:
> > - 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.
>
> Why? The record has not been inserted yet. I would tend to keep only
> the check at the bottom of XLogRecordAssemble(), for simplicity, and
> call it a day.

Because the sum value main_rdatalen can easily overflow in both the
current and the previous APIs, which then corrupts the WAL - one of
the two issues that I mentioned when I started the thread.

We don't re-summarize the lengths of all XLogRecData segments for the
main record data when assembling a record to keep the performance of
RecordAssemble (probably to limit the complexity when many data
segments are registered), and because I didn't want to add more
changes than necessary this check will need to be done in the place
where the overflow may occur, which is in XLogRegisterData.

> > - Kept the inline static elog-ing function (as per Andres' suggestion
> > on 2022-03-14; this decreases binary sizes)
>
> I am not really convinced that this one is worth doing.

I'm not married to that change, but I also don't see why this can't be
updated while this code is already being touched.

> +#define MaxXLogRecordSize (1020 * 1024 * 1024)
> +
> +#define XLogRecordLengthIsValid(len) ((len) >= 0 && (len) < MaxXLogRecordSize)
>
> These are used only in xloginsert.c, so we could keep them isolated.

They might be only used in xloginsert right now, but that's not the
point. This is now advertised as part of the record API spec: A record
larger than 1020MB is explicitly not supported. If it was kept
internal to xloginsert, that would be implicit and other people might
start hitting issues similar to those we're hitting right now -
records that are too large to read. Although PostgreSQL is usually the
only one generating WAL, we do support physical replication from
arbitrary PG-compatible WAL streams, which means that any compatible
WAL source could be the origin of our changes - and those need to be
aware of the assumptions we make about the WAL format.

I'm fine with also updating xlogreader.c to check this while reading
records to clarify the limits there as well, if so desired.

> + * To accommodate some overhead, hhis MaxXLogRecordSize value allows for
> s/hhis/this/.

Will be included in the next update..

> For now, I have extracted from the patch the two API changes and the
> checks for the block information for uint16, and applied this part.
> That's one less thing to worry about.

Thanks.

Kind regards,

Matthias van de Meent