Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit a0382e2

Browse filesBrowse files
tglsfdctglsfdc
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
Copy full SHA for a0382e2

File tree

Expand file treeCollapse file tree

1 file changed

+6
-5
lines changed
Filter options
Expand file treeCollapse file tree

1 file changed

+6
-5
lines changed

‎src/backend/storage/buffer/bufmgr.c

Copy file name to clipboardExpand all lines: src/backend/storage/buffer/bufmgr.c
+6-5Lines changed: 6 additions & 5 deletions
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 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.