On Mon, Jul 11, 2022 at 02:26:46PM +0200, Matthias van de Meent wrote:
> Thanks for reviewing.
I think that v6 is over-engineered because there should be no need to
add a check in xlogreader.c as long as the origin of the problem is
blocked, no? And the origin here is when the record is assembled. At
least this is the cleanest solution for HEAD, but not in the
back-branches if we'd care about doing something with records already
generated, and I am not sure that we need to care about other things
than HEAD, TBH. So it seems to me that there is no need to create a
XLogRecMaxLength which is close to a duplicate of
DecodeXLogRecordRequiredSpace().
@@ -519,7 +549,7 @@ XLogRecordAssemble(RmgrId rmid, uint8 info,
XLogRecPtr *fpw_lsn, int *num_fpi, bool *topxid_included)
{
XLogRecData *rdt;
- uint32 total_len = 0;
+ uint64 total_len = 0;
This has no need to change.
My suggestion from upthread was close to what you proposed, but I had
in mind something simpler, as of:
+ /*
+ * Ensure that xlogreader.c can read the record.
+ */
+ if (unlikely(!AllocSizeIsValid(DecodeXLogRecordRequiredSpace(total_len))))
+ elog(ERROR, "too much WAL data");
This would be the amount of data allocated by the WAL reader when it
is possible to allocate an oversized record, related to the business
of the circular buffer depending on if the read is blocking or not.
Among the two problems to solve at hand, the parts where the APIs are
changed and made more robust with unsigned types and where block data
is not overflowed with its 16-byte limit are committable, so I'd like
to do that first (still need to check its performance with some micro
benchmark on XLogRegisterBufData()). The second part to block the
creation of the assembled record is simpler, now
DecodeXLogRecordRequiredSpace() would make the path a bit hotter,
though we could inline it in the worst case?
--
Michael
