Re: Non-replayable WAL records through overflows and >MaxAllocSize lengths

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com>
Cc: David Zhang <david(dot)zhang(at)highgo(dot)ca>, Andres Freund <andres(at)anarazel(dot)de>, Robert Haas <robertmhaas(at)gmail(dot)com>, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Non-replayable WAL records through overflows and >MaxAllocSize lengths
Date: 2022-07-13 05:54:18
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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

@@ -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?

Attachment Content-Type Size
v7-0001-Add-protections-in-xlog-record-APIs-against-large_michael.patch text/x-diff 4.4 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Simon Riggs 2022-07-13 06:07:46 Re: Make name optional in CREATE STATISTICS
Previous Message 2022-07-13 05:48:45 RE: Perform streaming logical transactions by background workers and parallel apply