database/postgres
database/postgres
1
0
Fork 0
mirror of https://github.com/postgres/postgres.git synced 2025-05-03 22:24:49 +03:00
Code

Fix two off-by-one errors in bufmgr.c.

Browse Source 
In 4b4b680c I passed a buffer index number (starting from 0) instead of
a proper Buffer id (which start from 1 for shared buffers) in two
places.

This wasn't noticed so far as one of those locations isn't compiled at
all (PrintPinnedBufs) and the other one (InvalidBuffer) requires a
unlikely, but possible, set of circumstances to trigger a symptom.

To reduce the likelihood of such incidents a bit also convert existing
open coded mappings from buffer descriptors to buffer ids with
BufferDescriptorGetBuffer().

Author: Qingqing Zhou
Reported-By: Qingqing Zhou
Discussion: CAJjS0u2ai9ooUisKtkV8cuVUtEkMTsbK8c7juNAjv8K11zeCQg@mail.gmail.com
Backpatch: 9.5 where the private ref count infrastructure was introduced
This commit is contained in:
Andres Freund 2015-08-12 17:09:34 +02:00
parent c5bfcc18a0
commit 43a8ed26c9
1 changed files with 19 additions and 17 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

36
src/backend/storage/buffer/bufmgr.c
View File

@ -1273,7 +1273,7 @@ retry:
UnlockBufHdr(buf); UnlockBufHdr(buf);
LWLockRelease(oldPartitionLock); LWLockRelease(oldPartitionLock);
/* safety check: should definitely not be our *own* pin */ /* safety check: should definitely not be our *own* pin */
if (GetPrivateRefCount(buf->buf_id) > 0) if (GetPrivateRefCount(BufferDescriptorGetBuffer(buf)) > 0)
elog(ERROR, "buffer is pinned in InvalidateBuffer"); elog(ERROR, "buffer is pinned in InvalidateBuffer");
WaitIO(buf); WaitIO(buf);
goto retry; goto retry;
@ -1426,16 +1426,16 @@ ReleaseAndReadBuffer(Buffer buffer,
static bool static bool
PinBuffer(volatile BufferDesc *buf, BufferAccessStrategy strategy) PinBuffer(volatile BufferDesc *buf, BufferAccessStrategy strategy)
{ {
int b = buf->buf_id; Buffer b = BufferDescriptorGetBuffer(buf);
bool result; bool result;
PrivateRefCountEntry *ref; PrivateRefCountEntry *ref;
ref = GetPrivateRefCountEntry(b + 1, true); ref = GetPrivateRefCountEntry(b, true);
if (ref == NULL) if (ref == NULL)
{ {
ReservePrivateRefCountEntry(); ReservePrivateRefCountEntry();
ref = NewPrivateRefCountEntry(b + 1); ref = NewPrivateRefCountEntry(b);
LockBufHdr(buf); LockBufHdr(buf);
buf->refcount++; buf->refcount++;
@ -1460,8 +1460,7 @@ PinBuffer(volatile BufferDesc *buf, BufferAccessStrategy strategy)
ref->refcount++; ref->refcount++;
Assert(ref->refcount > 0); Assert(ref->refcount > 0);
ResourceOwnerRememberBuffer(CurrentResourceOwner, ResourceOwnerRememberBuffer(CurrentResourceOwner, b);
BufferDescriptorGetBuffer(buf));
return result; return result;
} }
@ -1489,23 +1488,24 @@ PinBuffer(volatile BufferDesc *buf, BufferAccessStrategy strategy)
static void static void
PinBuffer_Locked(volatile BufferDesc *buf) PinBuffer_Locked(volatile BufferDesc *buf)
{ {
int b = buf->buf_id; Buffer b;
PrivateRefCountEntry *ref; PrivateRefCountEntry *ref;
/* /*
* As explained, We don't expect any preexisting pins. That allows us to * As explained, We don't expect any preexisting pins. That allows us to
* manipulate the PrivateRefCount after releasing the spinlock * manipulate the PrivateRefCount after releasing the spinlock
*/ */
Assert(GetPrivateRefCountEntry(b + 1, false) == NULL); Assert(GetPrivateRefCountEntry(BufferDescriptorGetBuffer(buf), false) == NULL);
buf->refcount++; buf->refcount++;
UnlockBufHdr(buf); UnlockBufHdr(buf);
ref = NewPrivateRefCountEntry(b + 1); b = BufferDescriptorGetBuffer(buf);
ref = NewPrivateRefCountEntry(b);
ref->refcount++; ref->refcount++;
ResourceOwnerRememberBuffer(CurrentResourceOwner, ResourceOwnerRememberBuffer(CurrentResourceOwner, b);
BufferDescriptorGetBuffer(buf));
} }
/* /*
@ -1520,14 +1520,14 @@ static void
UnpinBuffer(volatile BufferDesc *buf, bool fixOwner) UnpinBuffer(volatile BufferDesc *buf, bool fixOwner)
{ {
PrivateRefCountEntry *ref; PrivateRefCountEntry *ref;
Buffer b = BufferDescriptorGetBuffer(buf);
/* not moving as we're likely deleting it soon anyway */ /* not moving as we're likely deleting it soon anyway */
ref = GetPrivateRefCountEntry(buf->buf_id + 1, false); ref = GetPrivateRefCountEntry(b, false);
Assert(ref != NULL); Assert(ref != NULL);
if (fixOwner) if (fixOwner)
ResourceOwnerForgetBuffer(CurrentResourceOwner, ResourceOwnerForgetBuffer(CurrentResourceOwner, b);
BufferDescriptorGetBuffer(buf));
Assert(ref->refcount > 0); Assert(ref->refcount > 0);
ref->refcount--; ref->refcount--;
@ -2739,6 +2739,7 @@ PrintBufferDescs(void)
for (i = 0; i < NBuffers; ++i) for (i = 0; i < NBuffers; ++i)
{ {
volatile BufferDesc *buf = GetBufferDescriptor(i); volatile BufferDesc *buf = GetBufferDescriptor(i);
Buffer b = BufferDescriptorGetBuffer(buf);
/* theoretically we should lock the bufhdr here */ /* theoretically we should lock the bufhdr here */
elog(LOG, elog(LOG,
@ -2747,7 +2748,7 @@ PrintBufferDescs(void)
i, buf->freeNext, i, buf->freeNext,
relpathbackend(buf->tag.rnode, InvalidBackendId, buf->tag.forkNum), relpathbackend(buf->tag.rnode, InvalidBackendId, buf->tag.forkNum),
buf->tag.blockNum, buf->flags, buf->tag.blockNum, buf->flags,
buf->refcount, GetPrivateRefCount(i)); buf->refcount, GetPrivateRefCount(b));
} }
} }
#endif #endif
@ -2761,8 +2762,9 @@ PrintPinnedBufs(void)
for (i = 0; i < NBuffers; ++i) for (i = 0; i < NBuffers; ++i)
{ {
volatile BufferDesc *buf = GetBufferDescriptor(i); volatile BufferDesc *buf = GetBufferDescriptor(i);
Buffer b = BufferDescriptorGetBuffer(buf);
if (GetPrivateRefCount(i + 1) > 0) if (GetPrivateRefCount(b) > 0)
{ {
/* theoretically we should lock the bufhdr here */ /* theoretically we should lock the bufhdr here */
elog(LOG, elog(LOG,
@ -2771,7 +2773,7 @@ PrintPinnedBufs(void)
i, buf->freeNext, i, buf->freeNext,
relpathperm(buf->tag.rnode, buf->tag.forkNum), relpathperm(buf->tag.rnode, buf->tag.forkNum),
buf->tag.blockNum, buf->flags, buf->tag.blockNum, buf->flags,
buf->refcount, GetPrivateRefCount(i + 1)); buf->refcount, GetPrivateRefCount(b));
} }
} }
} }