Re: buffer README is out of date

On Sat, Aug 29, 2015 at 1:27 PM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:

> On 8/29/15 2:21 PM, Jeff Janes wrote:
>
>> The buffer/README section on buffer clean up locks never got updated
>> for the creation of Heap Only Tuples and their associated compaction
>> logic.
>>
>> I've attached a patch to change the explanation. I'm sure someone
>> can word it better than I have.
>>
>
> ! Obtaining the necessary lock is done by the bufmgr routines
>> ! LockBufferForCleanup() or ConditionalLockBufferForCleanup().
>> ! They first get an exclusive lock and then check to see if the shared pin
>> ! count is currently 1. If not, ConditionalLockBufferForCleanup()
>> releases
>> ! the exclusive lock and then returns false, while LockBufferForCleanup()
>> ! releases the exclusive lock (but not the caller's pin) and waits until
>> ! signaled by another backend, whereupon it tries again. The signal will
>> ! occur when UnpinBuffer decrements the shared pin count to 1. As
>>
>
> I don't think that's true. If 2 other backends have a pin then AFAIK you'd
> wake up twice. There's also this comment in LockBufferForCleanup:
>
> /*
> * Remove flag marking us as waiter. Normally this will not be set
> * anymore, but ProcWaitForSignal() can return for other signals as
> * well. We take care to only reset the flag if we're the waiter, as
> * theoretically another backend could have started waiting. That's
> * impossible with the current usages due to table level locking, but
> * better be safe.
> */
>

If 2 other backends have a pin, only the last one to drop it should do the
waking. I don't see a way they could both try to do the waking, the
interlock on the buffer header seems to prevent that. But if it did, that
would just be another way you could have a spurious wake-up, which can
already happen for other reasons. I don't think the spurious wake up needs
to be documented in the higher level README file.

>
> ! indicated above, this operation might have to wait a good while before
>> ! it acquires lock, but that shouldn't matter much for concurrent VACUUM.
>> ! The current implementation only supports a single waiter for pin-count-1
>> ! on any particular shared buffer. This is enough for VACUUM's use, since
>> ! we don't allow multiple VACUUMs concurrently on a single relation
>> anyway.
>> ! Anyone wishing to obtain a clean up lock outside of a VACUUM must use
>> ! the conditional variant of the function.
>>
>
> That last statement should possibly be worded more strongly or just
> removed. If someone thinks they want to use this mechanism for something
> other than VACUUM there's probably a lot of other things to consider beyond
> just buffer locking.

But the point of this is that HOT updates are **already** doing this
outside of VACUUM. That is why the README is out of date.

Cheers,

Jeff