Make partition-lock-release coding more transparent in BufferAlloc().

Coverity complained that oldPartitionLock was possibly dereferenced after
having been set to NULL.  That actually can't happen, because we'd only use
it if (oldFlags & BM_TAG_VALID) is true.  But nonetheless Coverity is
justified in complaining, because at line 1275 we actually overwrite
oldFlags, and then still expect its BM_TAG_VALID bit to be a safe guide to
whether to release the oldPartitionLock.  Thus, the code would be incorrect
if someone else had changed the buffer's BM_TAG_VALID flag meanwhile.
That should not happen, since we hold pin on the buffer throughout this
sequence, but it's starting to look like a rather shaky chain of logic.
And there's no need for such assumptions, because we can simply replace
the (oldFlags & BM_TAG_VALID) tests with (oldPartitionLock != NULL),
which has identical results and makes it plain to all comers that we don't
dereference a null pointer.  A small side benefit is that the range of
liveness of oldFlags is greatly reduced, possibly allowing the compiler
to save a register.

This is just cleanup, not an actual bug fix, so there seems no need
for a back-patch.

diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index a2b2c66a86a7fb7b1c0a1f441fad2d3ac52ac1d8..939f5893432911d0674f9e52dc49574053f41020 100644 (file)
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -1198,9 +1198,10 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
                {
                        /* if it wasn't valid, we need only the new partition */
                        LWLockAcquire(newPartitionLock, LW_EXCLUSIVE);
-                       /* these just keep the compiler quiet about uninit variables */
+                       /* remember we have no old-partition lock or tag */
+                       oldPartitionLock = NULL;
+                       /* this just keeps the compiler quiet about uninit variables */
                        oldHash = 0;
-                       oldPartitionLock = 0;
                }
 
                /*
@@ -1223,7 +1224,7 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
                        UnpinBuffer(buf, true);
 
                        /* Can give up that buffer's mapping partition lock now */
-                       if ((oldFlags & BM_TAG_VALID) &&
+                       if (oldPartitionLock != NULL &&
                                oldPartitionLock != newPartitionLock)
                                LWLockRelease(oldPartitionLock);
 
@@ -1277,7 +1278,7 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
 
                UnlockBufHdr(buf, buf_state);
                BufTableDelete(&newTag, newHash);
-               if ((oldFlags & BM_TAG_VALID) &&
+               if (oldPartitionLock != NULL &&
                        oldPartitionLock != newPartitionLock)
                        LWLockRelease(oldPartitionLock);
                LWLockRelease(newPartitionLock);
@@ -1303,7 +1304,7 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
 
        UnlockBufHdr(buf, buf_state);
 
-       if (oldFlags & BM_TAG_VALID)
+       if (oldPartitionLock != NULL)
        {
                BufTableDelete(&oldTag, oldHash);
                if (oldPartitionLock != newPartitionLock)