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