From a0382e2d7e330de13e15cea0921a95faa9da3570 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 18 Apr 2016 18:05:56 -0400 Subject: [PATCH] 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 a2b2c66a86..939f589343 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)