Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit a0382e2

Browse files
committed
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.
1 parent 75c24d0 commit a0382e2

File tree

1 file changed

+6
-5
lines changed

1 file changed

+6
-5
lines changed

src/backend/storage/buffer/bufmgr.c

+6-5
Original file line numberDiff line numberDiff line change
@@ -1198,9 +1198,10 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
11981198
{
11991199
/* if it wasn't valid, we need only the new partition */
12001200
LWLockAcquire(newPartitionLock, LW_EXCLUSIVE);
1201-
/* these just keep the compiler quiet about uninit variables */
1201+
/* remember we have no old-partition lock or tag */
1202+
oldPartitionLock = NULL;
1203+
/* this just keeps the compiler quiet about uninit variables */
12021204
oldHash = 0;
1203-
oldPartitionLock = 0;
12041205
}
12051206

12061207
/*
@@ -1223,7 +1224,7 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
12231224
UnpinBuffer(buf, true);
12241225

12251226
/* Can give up that buffer's mapping partition lock now */
1226-
if ((oldFlags & BM_TAG_VALID) &&
1227+
if (oldPartitionLock != NULL &&
12271228
oldPartitionLock != newPartitionLock)
12281229
LWLockRelease(oldPartitionLock);
12291230

@@ -1277,7 +1278,7 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
12771278

12781279
UnlockBufHdr(buf, buf_state);
12791280
BufTableDelete(&newTag, newHash);
1280-
if ((oldFlags & BM_TAG_VALID) &&
1281+
if (oldPartitionLock != NULL &&
12811282
oldPartitionLock != newPartitionLock)
12821283
LWLockRelease(oldPartitionLock);
12831284
LWLockRelease(newPartitionLock);
@@ -1303,7 +1304,7 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
13031304

13041305
UnlockBufHdr(buf, buf_state);
13051306

1306-
if (oldFlags & BM_TAG_VALID)
1307+
if (oldPartitionLock != NULL)
13071308
{
13081309
BufTableDelete(&oldTag, oldHash);
13091310
if (oldPartitionLock != newPartitionLock)

0 commit comments

Comments
 (0)