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

On Thu, Mar 16, 2017 at 1:18 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> 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.
Thanks again for the review.

> 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.
+1.

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

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

> +/* 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.
Done.

> - * ----------
> + *
> + * 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.
Good to know. Fixed.

> 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...
Thanks to Ashutosh for testing the patches on Windows. It's working fine.

> + /* 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().
Fixed.

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

> + /*
> + * Before returning, report the background worker process in the
> + * PgBackendStatus array.
> + */
> + if (!bootstrap)
> + pgstat_bestart();
> Ditto with BackgroundWriterMain().
Perhaps you meant BackgroundWorkerInitializeConnection and
BackgroundWorkerInitializeConnectionByOid. Done.

> @@ -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?
Yes, backend_type can't be null. But, I'm not sure whether it should
be visible to a user with insufficient privileges. Anyway, I've made
it visible to all user for now.

Please find the updated patches in the attachment.

--
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com