On Mon, Jul 25, 2022 at 02:12:05PM +0300, Heikki Linnakangas wrote:
> 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.
That's a good point.
> 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)).
Something like that would work, I guess.
> 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.
Okay, that makes sense. FWIW, I have been wondering about the
addition of the extra condition in XLogRegisterBufData() and I did not
see a difference on HEAD in terms of execution time or profile, with a
micro-benchmark doing a couple of million calls in a row as of the
following, roughly:
// Can be anything, really..
rel = relation_open(RelationRelationId, AccessShareLock);
buffer = ReadBuffer(rel, 0);
for (i = 0 ; i < WAL_MAX_CALLS ; i++)
{
XLogBeginInsert();
XLogRegisterBuffer(0, buffer, REGBUF_STANDARD);
XLogRegisterBufData(0, buf, 10);
XLogResetInsertion();
}
ReleaseBuffer(buffer);
relation_close(rel, AccessShareLock);
--
Michael
