Re: Move PinBuffer and UnpinBuffer to atomics

On 2016-03-29 20:22:00 +0300, Alexander Korotkov wrote:
> > > + while (true)
> > > {
> > > - if (buf->usage_count == 0)
> > > - buf->usage_count = 1;
> > > + /* spin-wait till lock is free */
> > > + while (state & BM_LOCKED)
> > > + {
> > > + make_spin_delay(&delayStatus);
> > > + state = pg_atomic_read_u32(&buf->state);
> > > + oldstate = state;
> > > + }
> >
> > Maybe we should abstract that to pg_atomic_wait_bit_unset_u32()? It
> > seems quite likely we need this in other places (e.g. lwlock.c itself).
> >
>
> I have some doubts about such function. At first, I can't find a place for
> it in lwlock.c. It doesn't run explicit spin delays yet. Is it related to
> some changes you're planning for lwlock.c.

Yes, see http://www.postgresql.org/message-id/20160328130904.4mhugvkf4f3wg4qb@awork2.anarazel.de

> > > lsn = XLogSaveBufferForHint(buffer, buffer_std);
> > > }
> > >
> > > - LockBufHdr(bufHdr);
> > > - Assert(bufHdr->refcount > 0);
> > > - if (!(bufHdr->flags & BM_DIRTY))
> > > + state = LockBufHdr(bufHdr);
> > > +
> > > + Assert(BUF_STATE_GET_REFCOUNT(state) > 0);
> > > +
> > > + if (!(state & BM_DIRTY))
> > > {
> > > dirtied = true; /* Means "will be dirtied
> > by this action" */
> >
> > It's again worthwhile to try make this work without taking the lock.
> >
>
> Comment there claims.
>
> /*
> * Set the page LSN if we wrote a backup block. We aren't supposed
> * to set this when only holding a share lock but as long as we
> * serialise it somehow we're OK. We choose to set LSN while
> * holding the buffer header lock, which causes any reader of an
> * LSN who holds only a share lock to also obtain a buffer header
> * lock before using PageGetLSN(), which is enforced in
> * BufferGetLSNAtomic().
>
> Thus, buffer header lock is used for write serialization. I don't think it
> would be correct to remove the lock here.

Gah, I forgot about this uglyness. Man, the checksumming commit sure
made a number of things really ugly.

Greetings,

Andres Freund