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.paquier@gmail.com> wrote:
> On Wed, Mar 15, 2017 at 9:14 PM, Kuntal Ghosh
> <kuntalghosh.2007@gmail.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