localbuf: Fix dangerous coding pattern in GetLocalVictimBuffer()

If PinLocalBuffer() were to modify the buf_state, the buf_state in
GetLocalVictimBuffer() would be out of date. Currently that does not happen,
as PinLocalBuffer() only modifies the buf_state if adjust_usagecount=true and
GetLocalVictimBuffer() passes false.

However, it's easy to make this not the case anymore - it cost me a few hours
to debug the consequences.

The minimal fix would be to just refetch the buf_state after after calling
PinLocalBuffer(), but the same danger exists in later parts of the
function. Instead, declare buf_state in the narrower scopes and re-read the
state in conditional branches.  Besides being safer, it also fits well with
an upcoming set of cleanup patches that move the contents of the conditional
branches in GetLocalVictimBuffer() into helper functions.

I "broke" this in 794f2594479.

Arguably this should be backpatched, but as the relevant functions are not
exported and there is no actual misbehaviour, I chose to not backpatch, at
least for now.

Reviewed-by: Melanie Plageman <[email protected]>
Discussion: https://postgr.es/m/CAAKRu_b9anbWzEs5AAF9WCvcEVmgz-1AkHSQ-CLLy-p7WHzvFw@mail.gmail.com

diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.c
index 919f600ed417bcb2e42ce981aa65e92d20a231ef..456a2232f22bdaf87013dea5d37727d7e6f57d57 100644 (file)
--- a/src/backend/storage/buffer/localbuf.c
+++ b/src/backend/storage/buffer/localbuf.c
@@ -178,7 +178,6 @@ GetLocalVictimBuffer(void)
 {
    int         victim_bufid;
    int         trycounter;
-   uint32      buf_state;
    BufferDesc *bufHdr;
 
    ResourceOwnerEnlarge(CurrentResourceOwner);
@@ -199,7 +198,7 @@ GetLocalVictimBuffer(void)
 
        if (LocalRefCount[victim_bufid] == 0)
        {
-           buf_state = pg_atomic_read_u32(&bufHdr->state);
+           uint32      buf_state = pg_atomic_read_u32(&bufHdr->state);
 
            if (BUF_STATE_GET_USAGECOUNT(buf_state) > 0)
            {
@@ -233,8 +232,9 @@ GetLocalVictimBuffer(void)
     * this buffer is not referenced but it might still be dirty. if that's
     * the case, write it out before reusing it!
     */
-   if (buf_state & BM_DIRTY)
+   if (pg_atomic_read_u32(&bufHdr->state) & BM_DIRTY)
    {
+       uint32      buf_state = pg_atomic_read_u32(&bufHdr->state);
        instr_time  io_start;
        SMgrRelation oreln;
        Page        localpage = (char *) LocalBufHdrGetBlock(bufHdr);
@@ -267,8 +267,9 @@ GetLocalVictimBuffer(void)
    /*
     * Remove the victim buffer from the hashtable and mark as invalid.
     */
-   if (buf_state & BM_TAG_VALID)
+   if (pg_atomic_read_u32(&bufHdr->state) & BM_TAG_VALID)
    {
+       uint32      buf_state = pg_atomic_read_u32(&bufHdr->state);
        LocalBufferLookupEnt *hresult;
 
        hresult = (LocalBufferLookupEnt *)