remove lock protection on HeapTupleSatisfiesVacuum
Attached is a patch to remove the lock protection for
HeapTupleSatisfiesVacuum() in index code. The basic idea is: if we can do
a pin/lock/unlock sequence on a page, then without locking again, we are
gauranteed that there is no vacuum process acting on the same page.
According to buffer pool access rule #4, we then can examine/change the
hints bit safely.
Regards,
Qingqing
Attachments:
index.difftext/plain; charset=US-ASCII; name=index.diffDownload+7-15
Qingqing Zhou <zhouqq@cs.toronto.edu> writes:
Attached is a patch to remove the lock protection for
HeapTupleSatisfiesVacuum() in index code.
This patch scares the heck out of me. You need to offer some pretty
compelling performance reasons before I'd accept any part of it,
most especially this:
*** bufmgr.c 14 Apr 2006 03:38:55 -0000 1.207
--- bufmgr.c 6 Jun 2006 02:07:09 -0000
***************
*** 1686,1693 ****
bufHdr = &BufferDescriptors[buffer - 1]; Assert(PrivateRefCount[buffer - 1] > 0);
- /* here, either share or exclusive lock is OK */
- Assert(LWLockHeldByMe(bufHdr->content_lock));
/*
* This routine might get called many times on the same page, if we are
--- 1686,1691 ----Changing a buffer you hold no lock on is a recipe for disaster.
Where is the performance-boost evidence that suggests we should
even take the time to analyze whether this is safe?
regards, tom lane
"Tom Lane" <tgl@sss.pgh.pa.us> wrote
This patch scares the heck out of me. You need to offer some pretty
compelling performance reasons before I'd accept any part of it,
Changing a buffer you hold no lock on is a recipe for disaster.
Me too, in fact :-(.
The overall performance improvement might be marginal but why not if it is
right. What I cares is the correctness. As I understand, the orginal code
puts a shared lock (1) to prevent the vacuum process to move tuples around
so the hint bits change may happen in a wrong place; (2) to prevent other
operations holding EXCLUSIVE lock to change bits at the same time. This
patch makes sure that (1) can't happen. For (2), the original code has a
similar pitfalls -- if we only hold shared lock, then two process reach here
can change the bits at the same time.
Regards,
Qingqing
"Qingqing Zhou" <zhouqq@cs.toronto.edu> wrote
The overall performance improvement might be marginal but why not if it is
right. What I cares is the correctness. As I understand, the orginal code
puts a shared lock (1) to prevent the vacuum process to move tuples around
so the hint bits change may happen in a wrong place; (2) to prevent other
operations holding EXCLUSIVE lock to change bits at the same time.
I realized I made an aweful mistake. The shared lock also (3) to prevent
other operations holding EXCLUSIVE lock to change the xid fields at the
same. So the final conclusion is: the original code is right and my patch is
terriblly wrong :-(
Regards,
Qingqing
On Wed, Jun 07, 2006 at 09:34:47AM +0800, Qingqing Zhou wrote:
"Qingqing Zhou" <zhouqq@cs.toronto.edu> wrote
The overall performance improvement might be marginal but why not if it is
right. What I cares is the correctness. As I understand, the orginal code
puts a shared lock (1) to prevent the vacuum process to move tuples around
so the hint bits change may happen in a wrong place; (2) to prevent other
operations holding EXCLUSIVE lock to change bits at the same time.I realized I made an aweful mistake. The shared lock also (3) to prevent
other operations holding EXCLUSIVE lock to change the xid fields at the
same. So the final conclusion is: the original code is right and my patch is
terriblly wrong :-(
Maybe a comment patch would be in order to prevent future confusion?
--
Jim C. Nasby, Sr. Engineering Consultant jnasby@pervasive.com
Pervasive Software http://pervasive.com work: 512-231-6117
vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461