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

On Fri, Mar 17, 2017 at 6:19 PM, Kuntal Ghosh
<kuntalghosh(dot)2007(at)gmail(dot)com> wrote:
> On Thu, Mar 16, 2017 at 1:18 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> + /* 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.

And so you have the following thing to initialize the statistics:
@@ -5484,6 +5487,9 @@ BackgroundWorkerInitializeConnectionByOid(Oid
dboid, Oid useroid)

InitPostgres(NULL, dboid, NULL, useroid, NULL);

+ /* report the background worker process in the PgBackendStatus array */
+ pgstat_bestart();

And that bit as well:
+ if (beentry->st_backendType == B_BACKEND
+ || beentry->st_backendType == B_WAL_SENDER
+ || beentry->st_backendType == B_BG_WORKER)
+ beentry->st_userid = GetSessionUserId();
Unfortunately this is true only for background workers that connect to
a database. And this would break for bgworkers that do not do that.
The point to fix is here:
+ if (MyBackendId != InvalidBackendId)
+ {
[...]
+ else if (IsBackgroundWorker)
+ {
+ /* bgworker */
+ beentry->st_backendType = B_BG_WORKER;
+ }
Your code is assuming that a bgworker will always be setting
MyBackendId which is not necessarily true, and you would trigger the
assertion down:
Assert(MyAuxProcType != NotAnAuxProcess);
So you need to rely on IsBackgroundWorker in priority I think. I would
suggest as well to give up calling pgstat_bestart() in
BackgroundWorkerInitializeConnection[ByOid] and let the workers do
this work by themselves. This gives more flexibility. You would need
to update the logical replication worker and worker_spi for that as
well.

If you want to test this configuration, feel free to use this background worker:
https://github.com/michaelpq/pg_plugins/tree/master/hello_world
This just prints an entry to the logs every 10s, and does not connect
to any database. Adding a call to pgstat_bestart() in hello_main
triggers the assertion.

>> @@ -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.

Yeah, hiding it may make sense...

Thanks for the updated versions/

/* The autovacuum launcher is done here */
if (IsAutoVacuumLauncherProcess())
+ {
return;
+ }
Unnecessary noise here.

Except for the two issues pointed out in this email, I am pretty cool
with this patch. Nice work.
--
Michael