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