Hi,
In [1] I noticed that:
> FlushBuffer() is careful to not use a page's LSN unless it's
> BM_PERMANENT. However, GetVictimBuffer() is not:
>
> /* Read the LSN while holding buffer header lock */
> buf_state = LockBufHdr(buf_hdr);
> lsn = BufferGetLSN(buf_hdr);
> UnlockBufHdr(buf_hdr);
>
> if (XLogNeedsFlush(lsn)
> && StrategyRejectBuffer(strategy, buf_hdr,
> from_ring))
> {
> LockBuffer(buf, BUFFER_LOCK_UNLOCK);
> UnpinBuffer(buf_hdr);
> goto again;
> }
>
>
> If this is an unlogged table, the call to XLogNeedsFlush() will use a fake
> LSN, while XLogNeedsFlush() expects a real LSN.
>
> The consequences seem luckily limited, I don't think anything today uses
> bulkread strategies (which are the only ones where StrategyRejectBuffer()
> returns false if a flush is needed) with gist or such. But it'd be completely
> reasonable for XLogNeedsFlush() to have assertions about the LSN being in
> range, we just don't today.
I think we ought to fix it, even if it is currently harmless. It seems like
it'd not be too hard for this to go wrong in the future, it'd e.g. make sense
for XLogNeedsFlush() to XLogCtl->LogwrtRqst.Write and wake up walwriter. Which
would have bad consequences if it were done with a fake LSN.
I also think it might be good for XLogNeedsFlush() to have an assertion
verifying that the LSN in the realm of the possible. It's too easy to feed it
something entirely bogus right now and never notice.
Greetings,
Andres Freund
[1]
https://postgr.es/m/6blo6ebftb6eic6cadvkhn64zhxl5klf4fxgxfgbwgoijhxyiq%40mfz2kofehnjy
