Re: Non-replayable WAL records through overflows and >MaxAllocSize lengths - Mailing list pgsql-hackers

On Mon, 14 Mar 2022 at 18:14, Andres Freund <andres@anarazel.de> wrote:
>
> Hi
>
> A random thought I had while thinking about the size limits: We could use the
> low bits of the length and xl_prev to store XLR_SPECIAL_REL_UPDATE |
> XLR_CHECK_CONSISTENCY and give rmgrs the full 8 bit of xl_info. Which would
> allow us to e.g. get away from needing Heap2. Which would aestethically be
> pleasing.

That would be interesting; though out of scope for this bug I'm trying to fix.

> On 2022-03-14 17:57:23 +0100, Matthias van de Meent wrote:
> > I'm not sure whether or not to include this in the test suite, though,
> > as this would require a machine with at least 1GB of memory available
> > for this test alone, and I don't know the current requirements for
> > running the test suite.
>
> We definitely shouldn't require this much RAM for the tests.
>
> It might be worth adding tests exercising edge cases around segment boundaries
> (and perhaps page boundaries) though. E.g. record headers split across pages
> and segments.
>
>
>
> > --- a/src/backend/access/transam/xloginsert.c
> > +++ b/src/backend/access/transam/xloginsert.c
> > @@ -338,10 +338,16 @@ XLogRegisterData(char *data, int len)
> >  {
> >       XLogRecData *rdata;
> >
> > -     Assert(begininsert_called);
> > +     Assert(begininsert_called && len >= 0 && AllocSizeIsValid(len));
>
> Shouldn't we just make the length argument unsigned?

I've applied that in the attached revision; but I'd like to note that
this makes the fix less straightforward to backpatch; as the changes
to the public function signatures shouldn't be applied in older
versions.

> > -     if (num_rdatas >= max_rdatas)
> > +     /*
> > +      * Check against max_rdatas; and ensure we don't fill a record with
> > +      * more data than can be replayed
> > +      */
> > +     if (unlikely(num_rdatas >= max_rdatas ||
> > +                              !AllocSizeIsValid((uint64) mainrdata_len + (uint64) len)))
> >               elog(ERROR, "too much WAL data");
> > +
> >       rdata = &rdatas[num_rdatas++];
>
> Personally I'd write it as unlikely(num_rdatas >= max_rdatas) || unlikely(...)
> but I doubt if it makes an actual difference to the compiler.

Agreed, updated.

> >       rdata->data = data;
> > @@ -377,7 +383,7 @@ XLogRegisterBufData(uint8 block_id, char *data, int len)
> >       registered_buffer *regbuf;
> >       XLogRecData *rdata;
> >
> > -     Assert(begininsert_called);
> > +     Assert(begininsert_called && len >= 0 && len <= UINT16_MAX);
> >
> >       /* find the registered buffer struct */
> >       regbuf = ®istered_buffers[block_id];
> > @@ -385,8 +391,14 @@ XLogRegisterBufData(uint8 block_id, char *data, int len)
> >               elog(ERROR, "no block with id %d registered with WAL insertion",
> >                        block_id);
> >
> > -     if (num_rdatas >= max_rdatas)
> > +     /*
> > +      * Check against max_rdatas; and ensure we don't register more data per
> > +      * buffer than can be handled by the physical record format.
> > +      */
> > +     if (unlikely(num_rdatas >= max_rdatas ||
> > +                              regbuf->rdata_len + len > UINT16_MAX))
> >               elog(ERROR, "too much WAL data");
> > +
> >       rdata = &rdatas[num_rdatas++];
>
> Given the repeated check it might be worth to just put it in a static inline
> used from the relevant places (which'd generate less code because the same
> line number would be used for all the checks).

The check itself is slightly different in those 3 places; but the
error message is shared. Do you mean to extract the elog() into a
static inline function (as attached), or did I misunderstand?


-Matthias