Thread: Atomics in localbuf.c
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. -- Antonin Houska Web: https://www.cybertec-postgresql.com
Hi On March 5, 2020 9:21:55 AM PST, Antonin Houska <ah@cybertec.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. Ora cached one. Since local/shared buffers share the buffer header definition, we still have to use proper functions to accessthe atomic variables. Regards, Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Andres Freund <andres@anarazel.de> wrote: > On March 5, 2020 9:21:55 AM PST, Antonin Houska <ah@cybertec.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? -- Antonin Houska Web: https://www.cybertec-postgresql.com
Hi On March 5, 2020 12:42:06 PM PST, Antonin Houska <ah@cybertec.at> wrote: >Andres Freund <andres@anarazel.de> wrote: > >> On March 5, 2020 9:21:55 AM PST, Antonin Houska <ah@cybertec.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. Ants -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Andres Freund <andres@anarazel.de> wrote: > On March 5, 2020 12:42:06 PM PST, Antonin Houska <ah@cybertec.at> wrote: > >Andres Freund <andres@anarazel.de> wrote: > > > >> On March 5, 2020 9:21:55 AM PST, Antonin Houska <ah@cybertec.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
On Fri, Mar 6, 2020 at 2:04 AM Antonin Houska <ah@cybertec.at> wrote: > 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. Right. I thought, though, that your question was why we did it that way instead of just declaring them as uint32. I'm not sure it's very important, but I think that question hasn't really been answered. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Hi, On 2020-03-06 11:26:41 -0500, Robert Haas wrote: > On Fri, Mar 6, 2020 at 2:04 AM Antonin Houska <ah@cybertec.at> wrote: > > 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. > > Right. I thought, though, that your question was why we did it that > way instead of just declaring them as uint32. I'm not sure it's very > important, but I think that question hasn't really been answered. I tried, at least: > Since local/shared buffers share the buffer header definition, we still have to use proper functions to access > the atomic variables. There's only one struct BufferDesc. We could separate them out / introduce a union or such. But that'd add some complexity / potential for mistakes too. Greetings, Andres Freund
On Fri, Mar 6, 2020 at 2:06 PM Andres Freund <andres@anarazel.de> wrote: > > Since local/shared buffers share the buffer header definition, we still have to use proper functions to access > > the atomic variables. > > There's only one struct BufferDesc. We could separate them out / > introduce a union or such. But that'd add some complexity / potential > for mistakes too. OK, got it. Thanks. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company