On Fri, Jul 31, 2026 at 2:34 AM Matthias van de Meent < [email protected]> wrote:
> 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 > Thanks. That seems fine to me so is there anything else I need to do on my end?
