Re: Feature improvement: can we add queryId for pg_catalog.pg_stat_activityview?

On Wed, Jul 31, 2019 at 11:59 PM Andres Freund <andres@anarazel.de> wrote:
>
> On 2019-07-31 23:51:40 +0200, Julien Rouhaud wrote:
> > On Wed, Jul 31, 2019 at 10:55 AM Evgeny Efimkin <efimkin@yandex-team.ru> 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?