diff options
| author | Tom Lane <tgl@sss.pgh.pa.us> | 2016-04-18 18:05:56 -0400 |
|---|---|---|
| committer | Tom Lane <tgl@sss.pgh.pa.us> | 2016-04-18 18:05:56 -0400 |
| commit | a0382e2d7e330de13e15cea0921a95faa9da3570 (patch) | |
| tree | 5b8759113f2cf07cfa320b61635e871cb4565199 | |
| parent | 75c24d0f7491f77dfbc0acdf6c18439f288353ef (diff) | |
| download | postgresql-a0382e2d7e330de13e15cea0921a95faa9da3570.tar.gz postgresql-a0382e2d7e330de13e15cea0921a95faa9da3570.zip | |
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.
| -rw-r--r-- | src/backend/storage/buffer/bufmgr.c | 11 |
1 files 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) |