Re: Move PinBuffer and UnpinBuffer to atomics

On 2015-09-11 19:33:26 +0300, YUriy Zhuravlev wrote:
> On Friday 11 September 2015 18:14:21 Andres Freund wrote:
> > This way we can leave the for (;;) loop
> > in BufferAlloc() thinking that the buffer is unused (and can't be further
> > pinned because of the held spinlock!)
> 
> We lost lock after PinBuffer_Locked in BufferAlloc. Therefore, in essence, 
> nothing has changed.

The relevant piece of code is:    /*     * Need to lock the buffer header too in order to change its tag.     */
LockBufHdr(buf);
    /*     * Somebody could have pinned or re-dirtied the buffer while we were     * doing the I/O and making the new
hashtableentry.  If so, we can't     * recycle this buffer; we must undo everything we've done and start     * over
witha new victim buffer.     */    oldFlags = buf->flags;    if (buf->refcount == 1 && !(oldFlags & BM_DIRTY))
break;
    UnlockBufHdr(buf);    BufTableDelete(&newTag, newHash);    if ((oldFlags & BM_TAG_VALID) &&        oldPartitionLock
!=newPartitionLock)        LWLockRelease(oldPartitionLock);    LWLockRelease(newPartitionLock);    UnpinBuffer(buf,
true);}
/* * Okay, it's finally safe to rename the buffer. * * Clearing BM_VALID here is necessary, clearing the dirtybits is
just* paranoia.  We also reset the usage_count since any recency of use of * the old content is no longer relevant.
(Theusage_count starts out at * 1 so that the buffer can survive one clock-sweep pass.) */buf->tag = newTag;buf->flags
&=~(BM_VALID | BM_DIRTY | BM_JUST_DIRTIED | BM_CHECKPOINT_NEEDED | BM_IO_ERROR | BM_PERMANENT);if (relpersistence ==
RELPERSISTENCE_PERMANENT)   buf->flags |= BM_TAG_VALID | BM_PERMANENT;else    buf->flags |=
BM_TAG_VALID;buf->usage_count= 1;
 
UnlockBufHdr(buf);

so unless I'm missing something, no, we haven't lost the lock.

Greetings,

Andres Freund