On Thu, Aug 1, 2019 at 8:36 PM Andres Freund <andres@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 <andres@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.