On 13/07/2022 08:54, Michael Paquier wrote:
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.
The way this is written, it would change whenever we add/remove fields in DecodedBkpBlock, for example. That's fragile; if you added a field in a back-branch, you could accidentally make the new minor version unable to read maximum-sized WAL records generated with an older version. I'd like the maximum to be more explicit.
How large exactly is the maximum size that this gives? I'd prefer to set the limit conservatively to 1020 MB, for example, with a compile-time static assertion that AllocSizeIsValid(DecodeXLogRecordRequiredSpace(1020 MB)).
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()).
+1. I'm not excited about adding the "unlikely()" hints, though. We have a pg_attribute_cold hint in ereport(), that should be enough.
- Heikki
