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

Hi,

On 2019-07-31 23:51:40 +0200, Julien Rouhaud wrote:
> On Wed, Jul 31, 2019 at 10:55 AM Evgeny Efimkin <efimkin(at)yandex-team(dot)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).

Greetings,

Andres Freund