Re: exposing wait events for non-backends (was: Tracking wait event for latches)

On Wed, Mar 15, 2017 at 9:14 PM, Kuntal Ghosh
<kuntalghosh(dot)2007(at)gmail(dot)com> wrote:
> I've attached the updated patches.

Thanks for the new versions. This begins to look really clear.

> In 0001-Infra-to-expose-all-backend-processes-in-pg_stat_get.patch,
> I've extended BackendStatusArray to to store auxiliary processes.
> Backends
> use slots indexed in the range from 1 to MaxBackends (inclusive), so
> we can use MaxBackends + AuxBackendType + 1 as the index of the slot
> for an auxiliary process. Also, I've added a backend_type to describe
> the type of the backend process. The type includes:
> * autovacuum launcher
> * autovacuum worker
> * background writer
> * bgworker
> * client backend
> * checkpointer
> * startup
> * walreceiver
> * walsender
> * walwriter
> In 0002-Expose-stats-for-all-backends.patch, I've added the required
> code for reporting activity of different auxiliary processes,
> autovacuum launcher and bgworker processes.
> In 0003-Add-backend_type-column-in-pg_stat_get_activity.patch, I've
> added a column named backend_type in pg_stat_get_activity to show the
> type of the process to user.
>
> There are some pg_stat_* functions where showing all the backends
> doesn't make much sense. For example,
> postgres=# SELECT pg_stat_get_backend_pid(s.backendid) AS pid,
> pg_stat_get_backend_activity(s.backendid) AS query
> FROM (SELECT pg_stat_get_backend_idset() AS backendid) AS s;
> pid | query
> -------+------------------------------------------------------------------
> 17300 | SELECT pg_stat_get_backend_pid(s.backendid) AS pid, +
> | pg_stat_get_backend_activity(s.backendid) AS query +
> | FROM (SELECT pg_stat_get_backend_idset() AS backendid) AS s;
> 16925 | <command string not enabled>
> 16927 | <command string not enabled>
> 16926 | <command string not enabled>
> 16929 | <command string not enabled>
> IMHO, this scenario can be easily avoided by filtering backends using
> backend_type. I'm not sure whether we should add any logic in the code
> for handling such cases. Thoughts?

Having some activity really depends on the backend type (see
autovacuum workers for example which fill in the query field), so my
2c here is that we let things as your patch proposes. If at some point
it makes sense to add something in the query field, we could always
discuss it separately and patch it accordingly.

+/* Total number of backends including auxiliary */
+#define NumBackendStatSlots (MaxBackends + NUM_AUXPROCTYPES)
+
This variable remains localized in pgstat.c, so let's define it there.

+ <literal>bgworker</>, <literal>background writer</>,
That's really bike-shedding, but we could say here "background worker"
and be consistent with the rest.

+/* Total number of backends including auxiliary */
+#define NumBackendStatSlots (MaxBackends + NUM_AUXPROCTYPES)
This could be a bit more precise, telling as well that MaxBackends
includes autovacuum workers and background workers.

- * ----------
+ *
+ * Each auxiliary process also maintains a PgBackendStatus struct in shared
+ * memory.
*/
Better to not delete this line, this prevents pgindent to touch this
comment block.

Did you try if this patch worked with EXEC_BACKEND? Sorry I don't have
a Windows workstation at hand now, but as AuxiliaryProcs is
NON_EXEC_STATIC...

+ /* We have userid for client-backends and wal-sender processes */
+ if (beentry->st_backendType == B_BACKEND ||
beentry->st_backendType == B_WAL_SENDER)
+ beentry->st_userid = GetSessionUserId();
+ else
+ beentry->st_userid = InvalidOid;
This can be true as well for bgworkers defining a role OID when
connecting with BackgroundWorkerInitializeConnection().

+ /*
+ * Before returning, report autovacuum launcher process in the
+ * PgBackendStatus array.
+ */
+ pgstat_bestart();
return;
Wouldn't that be better in AutoVacLauncherMain()?

+ /*
+ * Before returning, report the background worker process in the
+ * PgBackendStatus array.
+ */
+ if (!bootstrap)
+ pgstat_bestart();
Ditto with BackgroundWriterMain().

@@ -808,6 +836,7 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
nulls[12] = true;
nulls[13] = true;
nulls[14] = true;
+ nulls[23] = true;
}
That's not possible to have backend_type set as NULL, no?
--
Michael