Thread: BUG #13487: GetBufferFromRing code bug

BUG #13487: GetBufferFromRing code bug

From
jingwei_5107@qq.com
Date:
The following bug has been logged on the website:

Bug reference:      13487
Logged by:          will
Email address:      jingwei_5107@qq.com
PostgreSQL version: 9.4.4
Operating system:   It does not matter
Description:

In GetBufferFromRing method(/src/backend/storage/buffer/freelist.c),I think
the following code:
506     buf = &BufferDescriptors[bufnum - 1];
507    LockBufHdr(buf);
508    if (buf->refcount == 0 && buf->usage_count <= 1)
509    {
510        strategy->current_was_in_ring = true;
511        return buf;
512    }
513    UnlockBufHdr(buf);

should be changed to(Adding line 510):

506     buf = &BufferDescriptors[bufnum - 1];
507    LockBufHdr(buf);
508    if (buf->refcount == 0 && buf->usage_count <= 1)
509    {
510             UnlockBufHdr(buf);// add this line
511        strategy->current_was_in_ring = true;
512        return buf;
513    }
514    UnlockBufHdr(buf);

It should Unlock the buf before returning buf, after exclusively accessing
the buf's refcount&usage_count member.

Am I right? Any reply is appreciated if I'm wrong.

Re: BUG #13487: GetBufferFromRing code bug

From
Thomas Munro
Date:
On Sun, Jul 5, 2015 at 3:12 AM,  <jingwei_5107@qq.com> wrote:
> The following bug has been logged on the website:
>
> Bug reference:      13487
> Logged by:          will
> Email address:      jingwei_5107@qq.com
> PostgreSQL version: 9.4.4
> Operating system:   It does not matter
> Description:
>
> In GetBufferFromRing method(/src/backend/storage/buffer/freelist.c),I think
> the following code:
> 506     buf = &BufferDescriptors[bufnum - 1];
> 507     LockBufHdr(buf);
> 508     if (buf->refcount == 0 && buf->usage_count <= 1)
> 509     {
> 510             strategy->current_was_in_ring = true;
> 511             return buf;
> 512     }
> 513     UnlockBufHdr(buf);
>
> should be changed to(Adding line 510):
>
> 506     buf = &BufferDescriptors[bufnum - 1];
> 507     LockBufHdr(buf);
> 508     if (buf->refcount == 0 && buf->usage_count <= 1)
> 509     {
> 510             UnlockBufHdr(buf);// add this line
> 511             strategy->current_was_in_ring = true;
> 512             return buf;
> 513     }
> 514     UnlockBufHdr(buf);
>
> It should Unlock the buf before returning buf, after exclusively accessing
> the buf's refcount&usage_count member.
>
> Am I right? Any reply is appreciated if I'm wrong.

From the function's documentation:

 * The bufhdr spin lock is held on the returned buffer.

From the documentation of StrategyGetBuffer (which calls it and
returns the buffer if not NULL):

 *      To ensure that no one else can pin the buffer before we do, we must
 *      return the buffer with the buffer header spinlock still held.

--
Thomas Munro
http://www.enterprisedb.com