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

Qingqing Zhou <zhouqq(at)cs(dot)toronto(dot)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(at)sss(dot)pgh(dot)pa(dot)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(at)cs(dot)toronto(dot)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(at)cs(dot)toronto(dot)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(at)pervasive(dot)com
Pervasive Software http://pervasive.com work: 512-231-6117
vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461

""Jim C. Nasby"" <jnasby(at)pervasive(dot)com> wrote
>
> Maybe a comment patch would be in order to prevent future confusion?
>

Not really needed because the buffer/README access rule#1 has already said
that ...

Regards,
Qingqing