Re: Fix pg_stat_get_backend_wait_event() for aux processes

> Come to think of it, why is wait_event_info stored in PGPROC in the
> first place, rather than in PgBackendStatus? All the other
> pg_stat_get_backend_*() functions just read the local PgBackendStatus copy.
>
> That point was debated when the wait events were introduced [1] [2].
> AFAICS the main motivation was that aux processes didn't have
> PgBackendStatus entries, and we wanted to expose wait events for aux
> processes too. That has changed since then, aux processes do have
> PgBackendStatus entries now, so that argument is moot.
>
> Because wait_event_info is fetched from PGPROC, it's not part of the
> "activity snapshot". So when you run "select * pg_stat_activity"
> repeatedly in the same transaction, the wait_events will change, even
> though the other fields are fetched once and frozen for the duration of
> the transaction. Tom pointed this out back then [1], but looks like that
> point was then forgotten, as we haven't documented that exception either.

Yeah, I agree with moving wait events to backend status.

There is also a discussion [0] about wait event/activity field inconsistency
with pg_stat_activity with a repro in [1]. This led to commit
f056f75dafd00, which
added a note section in the docs to highlight this behavior. I don't think
your proposed patch makes the note added in that commit invalid, but
I can see it fixing the behavior identified in [1]. So I am +1 for this change.

Here are some comments on 0001.

1/
     * Similarly, stop reporting wait events to MyProc->wait_event_info.
to

     * Similarly, stop reporting wait events to
PgBackendStatus->st_wait_event_info.

2/

+
+       /*
+        * Proc's wait information.  This *not* protected by the changecount
+        * mechanism, because reading and writing an uint32 is assumed
to atomic.
+        * This is updated very frequently, so we want to keep the overhead as
+        * small as possible.
+        */
+       uint32          st_wait_event_info;
+

Using or bypassing changecount meschanism occurs on access. Maybe we should say
"Proc's wait information. Since this is a uint32 and is assumed to be atomic, a
caller should not need to use the changecount mechanism to read/write."

What do you think?

3/

+ * pgstat_get_backend_type_by_proc_number() -
+ *
+ *     Return the type of the backend with the specified ProcNumber.
This looks
+ *     directly at the BackendStatusArray, so the return value may be
out of date.
+ *     The only current use of this function is in
pg_signal_backend(), which is
+ *     inherently racy, so we don't worry too much about this.
+ *
+ *     It is the caller's responsibility to use this wisely; at
minimum, callers
+ *     should ensure that procNumber is valid and perform the
required permissions
+ *     checks.
+ * ----------
+ */
+BackendType
+pgstat_get_backend_type_by_proc_number(ProcNumber procNumber)

+extern BackendType pgstat_get_backend_type_by_proc_number(ProcNumber
procNumber);


Maybe I am missing something, but I don't see
pgstat_get_backend_type_by_proc_number
being used.


--
Sami Imseih
Amazon Web Services

[0] https://www.postgresql.org/message-id/20220708.113925.694736747577500484.horikyota.ntt%40gmail.com
[1] https://www.postgresql.org/message-id/flat/20220708.113925.694736747577500484.horikyota.ntt%40gmail.com#b85744cec6fb75c5b038f39d7802e602
f