On Wed, Jul 31, 2019 at 11:59 PM Andres Freund <[email protected]> wrote: > > On 2019-07-31 23:51:40 +0200, Julien Rouhaud wrote: > > On Wed, Jul 31, 2019 at 10:55 AM Evgeny Efimkin <[email protected]> > > wrote: > > > What reason to use pg_atomic_uint64? > > > > The queryid is read and written without holding any lock on the PGPROC > > entry, so the pg_atomic_uint64 will guarantee that we get a consistent > > value in pg_stat_get_activity(). Other reads shouldn't be a problem > > as far as I remember. > > Hm, I don't think that's necessary in this case. That's what the > st_changecount protocol is trying to ensure, no? > > /* > * To avoid locking overhead, we use the following protocol: a backend > * increments st_changecount before modifying its entry, and again > after > * finishing a modification. A would-be reader should note the value > of > * st_changecount, copy the entry into private memory, then check > * st_changecount again. If the value hasn't changed, and if it's > even, > * the copy is valid; otherwise start over. This makes updates cheap > * while reads are potentially expensive, but that's the tradeoff we > want. > * > * The above protocol needs memory barriers to ensure that the > apparent > * order of execution is as it desires. Otherwise, for example, the > CPU > * might rearrange the code so that st_changecount is incremented > twice > * before the modification on a machine with weak memory ordering. > Hence, > * use the macros defined below for manipulating st_changecount, > rather > * than touching it directly. > */ > int st_changecount; > > > And if it were necessary, why wouldn't any of the other fields in > PgBackendStatus need it? There's plenty of other fields written to > without a lock, and several of those are also 8 bytes (so it's not a > case of assuming that 8 byte reads might not be atomic, but for byte > reads are).
This patch is actually storing the queryid in PGPROC, not in PgBackendStatus, thus the need for an atomic. I used PGPROC because the value needs to be available in log_line_prefix() and spi.c, so pgstat.c / PgBackendStatus didn't seem like the best interface in that case. Is widening PGPROC is too expensive for this purpose?
