Re: MarkBufferDirtyHint() and LSN update

On Wed, Oct 30, 2019 at 02:44:18PM +0100, Antonin Houska wrote:
>Please consider this scenario (race conditions):
>
>1. FlushBuffer() has written the buffer but hasn't yet managed to clear the
>BM_DIRTY flag (however BM_JUST_DIRTIED could be cleared by now).
>
>2. Another backend modified a hint bit and called MarkBufferDirtyHint().
>
>3. In MarkBufferDirtyHint(), if XLogHintBitIsNeeded() evaluates to true
>(e.g. due to checksums enabled), new LSN is computed, however it's not
>assigned to the page because the buffer is still dirty:
>
>    if (!(buf_state & BM_DIRTY))
>    {
>        ...
>
>        if (!XLogRecPtrIsInvalid(lsn))
>            PageSetLSN(page, lsn);
>    }
>
>4. MarkBufferDirtyHint() completes.
>
>5. In the first session, FlushBuffer()->TerminateBufferIO() will not clear
>BM_DIRTY because MarkBufferDirtyHint() has eventually set
>BM_JUST_DIRTIED. Thus the hint bit change itself will be written by the next
>call of FlushBuffer(). However page LSN is hasn't been updated so the
>requirement that WAL must be flushed first is not met.
>
>I think that PageSetLSN() should be called regardless BM_DIRTY. Do I miss any
>subtle detail?
>

Isn't this prevented by locking of the buffer header? Both FlushBuffer
and MarkBufferDirtyHint do obtain that lock. I see MarkBufferDirtyHint
does a bit of work before, but that's related to BM_PERMANENT.

If there really is a race condition, it shouldn't be that difficult to
trigger it by adding a sleep or a breakpoint, I guess.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services