On Fri, 31 Jul 2026 at 10:25, Michael Paquier <[email protected]> wrote: > > On Thu, Jul 30, 2026 at 01:36:00PM -0700, David K wrote: > > Thanks Matthias for the review. > > Please do not top-post, see this link, in line with the Postgres > mailing list style:
+1
> > Patch is attached with the following changes:
> > - keep XLogRecordMaxSize checks + Assert on reclength
> > - use add_size/mul_size instead of a bare size_t cast
> > - TYPEALIGN-equivalent roundup without an extra page when already aligned
> > - comment on the new oversized-length check
>
> I doubt that we want a full-fledged test module for this purpose
> integrated in the end result, even if it proves your point. The
> overall value does not seem to pile up from my perspective, just for a
> bunch of edge-case artistic checks to validate the API.
I agree that testing just these changes doesn't add much value, and
definitely not in the way currently implemented, but I do think there
would be value in a module that tests the various kinds of corruption
the XLogReader could encounter and should detect.
> + /*
> + * Cap xl_tot_len before contrecord reassembly so we never allocate
> or
> + * copy based on a garbage length from a recycled page.
> + */
> + if (total_len > XLogRecordMaxSize)
> + {
> + report_invalid_record(state,
> + "invalid record length at %X/%08X:
> expected at most %u, got %u",
> + LSN_FORMAT_ARGS(RecPtr),
> + XLogRecordMaxSize, total_len);
> + goto err;
> + }
>
> Saying that. Andres has reminded me not so long ago that we enforce
> this rule on the WAL insert side but we don't do so on the xlogreader
> side. Now, what's the point in having the same check two times? The
> proposed patch does it once we have read the first bytes of the record
> for the total record length, and a second time when validating the
> record header. It feels to me that we should just do it once. Or,
> wait, you have done it this way to map with total_len? In which case
> I can buy it.. It seems to me that this should be split as a patch of
> its own.
Note that the record header can be split across pages, and allocations
currently happen ahead of the validation of that split header. I think
the additional check makes sense here.
> On Thu, Jul 30, 2026 at 01:36:00PM -0700, David K wrote:
> > Patch is attached with the following changes:
> > - keep XLogRecordMaxSize checks + Assert on reclength
> > - use add_size/mul_size instead of a bare size_t cast
> > - TYPEALIGN-equivalent roundup without an extra page when already aligned
> > - comment on the new oversized-length check
I'd personally hoped the adjusted patch would've been more like
attached: In it, I've cleaned up allocate_recordbuf to avoid the
additional XLOG_BLCKSZ and adjusted a few comments. It also drops the
test module.
Kind regards,
Matthias van de Meent
v3-0001-Fix-XLogReader-mishandling-of-oversized-multi-pag.patch
Description: Binary data
