On Thu, Aug 1, 2019 at 8:36 PM Andres Freund <and...@anarazel.de> wrote: > > On 2019-08-01 08:45:45 +0200, Julien Rouhaud wrote: > > On Wed, Jul 31, 2019 at 11:59 PM Andres Freund <and...@anarazel.de> wrote: > > > 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. > > Hm. I'm not convinced that really is the case? You can just access > MyBEentry, and read and update it?
Sure, but it requires extra wrapper functions, and the st_changecount dance when writing the new value. > I mean, we do so at a frequency > roughtly as high as high as the new queryid updates for things like > pgstat_report_activity(). pgstat_report_activity() is only called for top-level statement. For the queryid we need to track it down to all nested statements, which could be way higher. But pgstat_progress_update_param() is called way more than that. > Reading the value of your own backend you'd > not need to follow the changecount algorithm, I think, because it's only > updated from the current backend. If reading were a problem, you > trivially just could have a cache in a local variable, to avoid > accessing shared memory. Yes definitely, except for pgstat_get_activity(), all reads are backend local and should be totally safe to read as is.
