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

Hi,

On 2022-03-11 22:42:42 +0200, Heikki Linnakangas wrote:
> Have you been able to create a test case for that? The largest record I can
> think of is a commit record with a huge number of subtransactions, dropped
> relations, and shared inval messages. I'm not sure if you can overflow a
> uint32 with that, but exceeding MaxAllocSize seems possible.

MaxAllocSize is pretty easy:
SELECT pg_logical_emit_message(false, long, long) FROM repeat(repeat(' ', 1024), 1024*1023) as l(long);

on a standby:

2022-03-11 16:41:59.336 PST [3639744][startup][1/0:0] LOG:  record length 2145386550 at 0/3000060 too long



> I wonder if these checks hurt performance. These are very cheap, but then
> again, this codepath is very hot. It's probably fine, but it still worries
> me a little. Maybe some of these could be Asserts.

I wouldn't expect the added branch itself to hurt much in XLogRegisterData() -
it should be statically predicted to be not taken with the unlikely. I don't
think it's quite inner-loop enough for the instructions or the number of
"concurrently out of order branches" to be a problem.

FWIW, often the added elog()s are worse, because they require a decent amount
of code and restrict the optimizer somewhat (e.g. no sibling calls, more local
variables etc). They can't even be deduplicated because of the line-numbers
embedded.

So maybe just collapse the new elog() with the previous elog, with a common
unlikely()?


> > @@ -734,6 +744,10 @@ XLogRecordAssemble(RmgrId rmid, uint8 info,
> >  		if (needs_data)
> >  		{
> > +			/* protect against overflow */
> > +			if (unlikely(regbuf->rdata_len > UINT16_MAX))
> > +				elog(ERROR, "too much WAL data for registered buffer");
> > +
> >  			/*
> >  			 * Link the caller-supplied rdata chain for this buffer to the
> >  			 * overall list.

FWIW, this branch I'm a tad more concerned about - it's in a loop body where
plausibly a lot of branches could be outstanding at the same time.

ISTM that this could just be an assert?

Greetings,

Andres Freund