On Fri, 31 Jul 2026 at 10:25, Michael Paquier <michael@paquier.xyz> 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
Thanks. That seems fine to me so is there anything else I need to do on my end?
