fix oversight converting buf_id to Buffer

Started by Qingqing Zhouover 10 years ago5 messages
#1Qingqing Zhou
zhouqq.postgres@gmail.com
1 attachment(s)

Attached patch fixes oversights converting buf_id to Buffer in
PrintBufferDescs() and InvalidateBuffer(). Especially for the latter,
the reason we haven't seen any reports of the issue might be that it
needs certain concurrent conditions to be true.

Along the line, it also changes all direct maths against buf_id to use
BufferDescriptorGetBuffer() instead.

Regards,
Qingqing

Attachments:

bufmgr.difftext/plain; charset=US-ASCII; name=bufmgr.diffDownload
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
new file mode 100644
index e4b2558..2e9a7c7
*** a/src/backend/storage/buffer/bufmgr.c
--- b/src/backend/storage/buffer/bufmgr.c
*************** retry:
*** 1273,1279 ****
  		UnlockBufHdr(buf);
  		LWLockRelease(oldPartitionLock);
  		/* safety check: should definitely not be our *own* pin */
! 		if (GetPrivateRefCount(buf->buf_id) > 0)
  			elog(ERROR, "buffer is pinned in InvalidateBuffer");
  		WaitIO(buf);
  		goto retry;
--- 1273,1279 ----
  		UnlockBufHdr(buf);
  		LWLockRelease(oldPartitionLock);
  		/* safety check: should definitely not be our *own* pin */
! 		if (GetPrivateRefCount(BufferDescriptorGetBuffer(buf)) > 0)
  			elog(ERROR, "buffer is pinned in InvalidateBuffer");
  		WaitIO(buf);
  		goto retry;
*************** ReleaseAndReadBuffer(Buffer buffer,
*** 1426,1441 ****
  static bool
  PinBuffer(volatile BufferDesc *buf, BufferAccessStrategy strategy)
  {
! 	int			b = buf->buf_id;
  	bool		result;
  	PrivateRefCountEntry *ref;
  
! 	ref = GetPrivateRefCountEntry(b + 1, true);
  
  	if (ref == NULL)
  	{
  		ReservePrivateRefCountEntry();
! 		ref = NewPrivateRefCountEntry(b + 1);
  
  		LockBufHdr(buf);
  		buf->refcount++;
--- 1426,1441 ----
  static bool
  PinBuffer(volatile BufferDesc *buf, BufferAccessStrategy strategy)
  {
! 	Buffer		b = BufferDescriptorGetBuffer(buf);
  	bool		result;
  	PrivateRefCountEntry *ref;
  
! 	ref = GetPrivateRefCountEntry(b, true);
  
  	if (ref == NULL)
  	{
  		ReservePrivateRefCountEntry();
! 		ref = NewPrivateRefCountEntry(b);
  
  		LockBufHdr(buf);
  		buf->refcount++;
*************** PinBuffer(volatile BufferDesc *buf, Buff
*** 1460,1467 ****
  
  	ref->refcount++;
  	Assert(ref->refcount > 0);
! 	ResourceOwnerRememberBuffer(CurrentResourceOwner,
! 								BufferDescriptorGetBuffer(buf));
  	return result;
  }
  
--- 1460,1466 ----
  
  	ref->refcount++;
  	Assert(ref->refcount > 0);
! 	ResourceOwnerRememberBuffer(CurrentResourceOwner, b);
  	return result;
  }
  
*************** PinBuffer(volatile BufferDesc *buf, Buff
*** 1489,1511 ****
  static void
  PinBuffer_Locked(volatile BufferDesc *buf)
  {
! 	int			b = buf->buf_id;
  	PrivateRefCountEntry *ref;
  
  	/*
  	 * As explained, We don't expect any preexisting pins. That allows us to
  	 * manipulate the PrivateRefCount after releasing the spinlock
  	 */
! 	Assert(GetPrivateRefCountEntry(b + 1, false) == NULL);
  
  	buf->refcount++;
  	UnlockBufHdr(buf);
  
! 	ref = NewPrivateRefCountEntry(b + 1);
  	ref->refcount++;
  
! 	ResourceOwnerRememberBuffer(CurrentResourceOwner,
! 								BufferDescriptorGetBuffer(buf));
  }
  
  /*
--- 1488,1509 ----
  static void
  PinBuffer_Locked(volatile BufferDesc *buf)
  {
! 	Buffer		b = BufferDescriptorGetBuffer(buf);
  	PrivateRefCountEntry *ref;
  
  	/*
  	 * As explained, We don't expect any preexisting pins. That allows us to
  	 * manipulate the PrivateRefCount after releasing the spinlock
  	 */
! 	Assert(GetPrivateRefCountEntry(b, false) == NULL);
  
  	buf->refcount++;
  	UnlockBufHdr(buf);
  
! 	ref = NewPrivateRefCountEntry(b);
  	ref->refcount++;
  
! 	ResourceOwnerRememberBuffer(CurrentResourceOwner, b);
  }
  
  /*
*************** static void
*** 1520,1533 ****
  UnpinBuffer(volatile BufferDesc *buf, bool fixOwner)
  {
  	PrivateRefCountEntry *ref;
  
  	/* not moving as we're likely deleting it soon anyway */
! 	ref = GetPrivateRefCountEntry(buf->buf_id + 1, false);
  	Assert(ref != NULL);
  
  	if (fixOwner)
! 		ResourceOwnerForgetBuffer(CurrentResourceOwner,
! 								  BufferDescriptorGetBuffer(buf));
  
  	Assert(ref->refcount > 0);
  	ref->refcount--;
--- 1518,1531 ----
  UnpinBuffer(volatile BufferDesc *buf, bool fixOwner)
  {
  	PrivateRefCountEntry *ref;
+ 	Buffer		b = BufferDescriptorGetBuffer(buf);
  
  	/* not moving as we're likely deleting it soon anyway */
! 	ref = GetPrivateRefCountEntry(b, false);
  	Assert(ref != NULL);
  
  	if (fixOwner)
! 		ResourceOwnerForgetBuffer(CurrentResourceOwner, b);
  
  	Assert(ref->refcount > 0);
  	ref->refcount--;
*************** PrintBufferDescs(void)
*** 2739,2753 ****
  	for (i = 0; i < NBuffers; ++i)
  	{
  		volatile BufferDesc *buf = GetBufferDescriptor(i);
  
  		/* theoretically we should lock the bufhdr here */
  		elog(LOG,
  			 "[%02d] (freeNext=%d, rel=%s, "
  			 "blockNum=%u, flags=0x%x, refcount=%u %d)",
  			 i, buf->freeNext,
! 		  relpathbackend(buf->tag.rnode, InvalidBackendId, buf->tag.forkNum),
  			 buf->tag.blockNum, buf->flags,
! 			 buf->refcount, GetPrivateRefCount(i));
  	}
  }
  #endif
--- 2737,2753 ----
  	for (i = 0; i < NBuffers; ++i)
  	{
  		volatile BufferDesc *buf = GetBufferDescriptor(i);
+ 		Buffer		b = BufferDescriptorGetBuffer(buf);
  
  		/* theoretically we should lock the bufhdr here */
+ 		Assert (b == i + 1);
  		elog(LOG,
  			 "[%02d] (freeNext=%d, rel=%s, "
  			 "blockNum=%u, flags=0x%x, refcount=%u %d)",
  			 i, buf->freeNext,
! 			 relpathbackend(buf->tag.rnode, InvalidBackendId, buf->tag.forkNum),
  			 buf->tag.blockNum, buf->flags,
! 			 buf->refcount, GetPrivateRefCount(b));
  	}
  }
  #endif
*************** PrintPinnedBufs(void)
*** 2761,2768 ****
  	for (i = 0; i < NBuffers; ++i)
  	{
  		volatile BufferDesc *buf = GetBufferDescriptor(i);
  
! 		if (GetPrivateRefCount(i + 1) > 0)
  		{
  			/* theoretically we should lock the bufhdr here */
  			elog(LOG,
--- 2761,2770 ----
  	for (i = 0; i < NBuffers; ++i)
  	{
  		volatile BufferDesc *buf = GetBufferDescriptor(i);
+ 		Buffer		b = BufferDescriptorGetBuffer(buf);
  
! 		Assert (b == i + 1);
! 		if (GetPrivateRefCount(b) > 0)
  		{
  			/* theoretically we should lock the bufhdr here */
  			elog(LOG,
*************** PrintPinnedBufs(void)
*** 2771,2777 ****
  				 i, buf->freeNext,
  				 relpathperm(buf->tag.rnode, buf->tag.forkNum),
  				 buf->tag.blockNum, buf->flags,
! 				 buf->refcount, GetPrivateRefCount(i + 1));
  		}
  	}
  }
--- 2773,2779 ----
  				 i, buf->freeNext,
  				 relpathperm(buf->tag.rnode, buf->tag.forkNum),
  				 buf->tag.blockNum, buf->flags,
! 				 buf->refcount, GetPrivateRefCount(b));
  		}
  	}
  }
#2Andres Freund
andres@anarazel.de
In reply to: Qingqing Zhou (#1)
Re: fix oversight converting buf_id to Buffer

Hi,

That's a very nice catch! Did you trigger the error or just found it
when reading the code?

On 2015-08-10 12:12:01 -0700, Qingqing Zhou wrote:

Attached patch fixes oversights converting buf_id to Buffer in
PrintBufferDescs() and InvalidateBuffer(). Especially for the latter,
the reason we haven't seen any reports of the issue might be that it
needs certain concurrent conditions to be true.

PrintBufferDescs() is dead code, so bit is not surprising. I'm also not
surprised that the wrong buffer in InvalidateBuffer()'s check doesn't
trigger. You'd have to have a cursor open in the current backend that
currently pins the off-by-one buffer.

I'm too tired right now to look at this, but it generally looked sane.

Regards,

Andres

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#3Qingqing Zhou
zhouqq.postgres@gmail.com
In reply to: Andres Freund (#2)
Re: fix oversight converting buf_id to Buffer

On Mon, Aug 10, 2015 at 4:15 PM, Andres Freund <andres@anarazel.de> wrote:

That's a very nice catch! Did you trigger the error or just found it
when reading the code?

My fellow colleagues hit the issue during some stress: I am not clear
the exact repro but from the failed assertion, the cause is kinda
clear.

Regards,
Qingqing

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#4Andres Freund
andres@anarazel.de
In reply to: Andres Freund (#2)
Re: fix oversight converting buf_id to Buffer

Hi,

On 2015-08-11 01:15:37 +0200, Andres Freund wrote:

I'm too tired right now to look at this, but it generally looked sane.

Pushed your fix to master and 9.5, with two very minor changes:
1) I moved the BufferDescriptorGetBuffer() call in PinBuffer_Locked() to
after the spinlock release. It's rather minor, but there seems
little reason to do it before except the assert, which isn't compiled
in production.
2) I removed the two asserts you added. They essentially asserted that
i + 1 == i + 1.

Thanks again for the catch and patch.

Greetings,

Andres Freund

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#5Qingqing Zhou
zhouqq.postgres@gmail.com
In reply to: Andres Freund (#4)
Re: fix oversight converting buf_id to Buffer

All look good to me,

Thank you,
Qingqing

On Wed, Aug 12, 2015 at 8:37 AM, Andres Freund <andres@anarazel.de> wrote:

Hi,

On 2015-08-11 01:15:37 +0200, Andres Freund wrote:

I'm too tired right now to look at this, but it generally looked sane.

Pushed your fix to master and 9.5, with two very minor changes:
1) I moved the BufferDescriptorGetBuffer() call in PinBuffer_Locked() to
after the spinlock release. It's rather minor, but there seems
little reason to do it before except the assert, which isn't compiled
in production.
2) I removed the two asserts you added. They essentially asserted that
i + 1 == i + 1.

Thanks again for the catch and patch.

Greetings,

Andres Freund

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers