Re: [HACKERS] exposing wait events for non-backends (was: Trackingwait event for latches) - Mailing list pgsql-hackers

On Fri, Mar 10, 2017 at 2:36 PM, Kuntal Ghosh
<kuntalghosh.2007@gmail.com> wrote:
> On Fri, Mar 10, 2017 at 3:11 AM, Andres Freund <andres@anarazel.de> wrote:
>> On 2017-03-09 16:37:29 -0500, Tom Lane wrote:
>>> Robert Haas <robertmhaas@gmail.com> writes:
>>> > On Thu, Mar 9, 2017 at 2:30 PM, Peter Eisentraut
>>> > <peter.eisentraut@2ndquadrant.com> wrote:
>>> >> In practice, I think it's common to do a quick select * from
>>> >> pg_stat_activity to determine whether a database instance is in use.
>>>
>>> > I thought of the same kind of thing, and it was discussed upthread.
>>> > There seemed to be more votes for keeping it all in one view, but that
>>> > could change if more people vote.
>>>
>>> I've not been paying much attention to this thread, but it seems like
>>> something that would help Peter's use-case and have other uses as well
>>> is a new column that distinguishes different process types --- user
>>> session, background worker, autovacuum worker, etc.
>>
>> The patches upthread add precisely such a column.
>>
> The patch exposes auxiliary processes, autovacuum launcher and
> bgworker along with other backends in pg_stat_activity. It also adds
> an extra column, named proc_type (suggested by Craig
> and Robert), to indicate the type of process in pg_stat_activity view.
> proc_type includes:
>
> * client backend
> * autovacuum launcher
> * wal sender
> * bgworker
> * writer
> * checkpointer
> * wal writer
> * wal receiver

"writer" would be better if defined as "background writer" instead?
You are forgetting in this list autovacuum workers and the startup
process, the latter is important for nodes in recovery.

> Here is the present output with the relevant columns.
> postgres=# SELECT wait_event_type, wait_event, state, proc_type FROM
> pg_stat_activity;
>  wait_event_type |     wait_event      | state  |      proc_type
> -----------------+---------------------+--------+---------------------
>  Activity        | AutoVacuumMain      | idle   | autovacuum launcher
>  Activity        | LogicalLauncherMain | idle   | bgworker
>  Activity        | WalSenderMain       | idle   | wal sender
>                  |                     | active | client backend
>  Activity        | BgWriterMain        | idle   | writer
>  Activity        | CheckpointerMain    | idle   | checkpointer
>  Activity        | WalWriterMain       | idle   | wal writer
> (7 rows)

Here is a first pass on this patch.

+     <entry>Type of the current server process. Possible types are:
+      autovacuum launcher, bgworker, checkpointer, client backend,
+      wal receiver, wal sender, wal writer and writer.
+     </entry>
There should be markups around those terms. Shouldn't "wal writer" and
"wal sender" and "wal receiver" not use any space? On HEAD a WAL
sender is defined as "walsender".

+ * Each auxliliary process also maintains a PgBackendStatus struct in shared
+ * memory.
s/auxliliary/auxiliary/.
void
-pgstat_bestart(void)
+pgstat_procstart(void)
I would not have thought that this patch justifies potentially
breaking extensions.

For WAL senders, the "query" field is still filled with "walsender".
This should be removed.

@@ -365,6 +368,8 @@ CheckpointerMain(void)        */       AbsorbFsyncRequests();

+       pgstat_report_activity(STATE_RUNNING, NULL);
+       if (got_SIGHUP)
It seems to me that what we want to know here are only the wait
events, so I think that you had better drop this portion of the patch.
It is hard to decide if an auxiliary process is idle or running, and
this routine is a concept that applies to database backends running
queries.

-static LocalPgBackendStatus *localBackendStatusTable = NULL;
+
+/* Status for backends and auxiliary processes */
+static LocalPgBackendStatus *localProcStatusTable = NULL;
I don't quite understand why you need to have two layers here,
wouldn't it be more simple to just extend localBackendStatusTable with
enough slots for the system processes? That would be also less
bug-prone. You need to be careful to have a correct mapping to
include:
- auxiliary processes, up to 4 slots used.
- bgworker processes, decided by GUC.
- autovacuum workers, decided by GUC.

+PgBackendStatus *
+pgstat_fetch_stat_procentry(int procid)
+{
+   pgstat_read_current_status();
This function is used nowhere.
-- 
Michael