postgres.git / summary / log / commit / refs

commit    a0382e2d7e330de13e15cea0921a95faa9da3570
Author:   Tom Lane <tgl@sss.pgh.pa.us>
Date:     Mon Apr 18 22:05:56 2016 +0000
First released in: 9.6.0

    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.


 src/backend/storage/buffer/bufmgr.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index a2b2c66a86a..939f5893432 100644
--- 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)


[parent: 75c24d0f7491]