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

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.

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?

> - 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.

> 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 = &registered_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).

Greetings,

Andres Freund