Re: Atomics in localbuf.c

Andres Freund <andres(at)anarazel(dot)de> wrote:

> On March 5, 2020 12:42:06 PM PST, Antonin Houska <ah(at)cybertec(dot)at> wrote:
> >Andres Freund <andres(at)anarazel(dot)de> wrote:
> >
> >> On March 5, 2020 9:21:55 AM PST, Antonin Houska <ah(at)cybertec(dot)at>
> >wrote:
> >> >What's the reason to use pg_atomic...read_...() and
> >> >pg_atomic...write_...()
> >> >functions in localbuf.c?
> >> >
> >> >It looks like there was an intention not to use them
> >> >
> >>
> >>https://www.postgresql.org/message-id/CAPpHfdtfr3Aj7xJonXaKR8iY2p8uXOQ%2Be4BMpMDAM_5R4OcaDA%40mail.gmail.com
> >> >
> >> >but the following discussion does not explain the decision to use
> >them.
> >>
> >> Read/write don't trigger locked/atomic operations. They just
> >guarantee that
> >> you're not gonna read/write a torn value. Or a cached one. Since
> >> local/shared buffers share the buffer header definition, we still
> >have to
> >> use proper functions to access the atomic variables.
> >
> >Sure, the atomic operations are necessary for shared buffers, but I
> >still
> >don't understand why they are needed for *local* buffers - local
> >buffers their
> >headers (BufferDesc) in process local memory, so there should be no
> >concerns
> >about concurrent access.
> >
> >Another thing that makes me confused is this comment in
> >InitLocalBuffers():
> >
> > /*
> > * Intentionally do not initialize the buffer's atomic variable
> > * (besides zeroing the underlying memory above). That way we get
> > * errors on platforms without atomics, if somebody (re-)introduces
> > * atomic operations for local buffers.
> > */
> >
> >That sounds like there was an intention not to use the atomic functions
> >in
> >localbuf.c, but eventually they ended up there. Do I still miss
> >something?
>
> Again, the read/write functions do not imply atomic instructions.

ok. What I missed is that BufferDesc.state is declared as pg_atomic_uint32
rather than plain int, so the pg_atomic_...() functions should be used
regardless the buffer is shared or local. Sorry for the noise.

--
Antonin Houska
Web: https://www.cybertec-postgresql.com