From: | Kuntal Ghosh <kuntalghosh(dot)2007(at)gmail(dot)com> |
---|---|
To: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com> |
Cc: | Andres Freund <andres(at)anarazel(dot)de>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Robert Haas <robertmhaas(at)gmail(dot)com>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>, Craig Ringer <craig(at)2ndquadrant(dot)com>, Kevin Grittner <kgrittn(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>, Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru>, PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: exposing wait events for non-backends (was: Tracking wait event for latches) |
Date: | 2017-03-17 09:19:18 |
Message-ID: | CAGz5QC+t=rvx6PmSUGeLsp_nBmcRJPVd203D0pOQRQKpg2fX4w@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
0001-Infra-to-expose-all-backend-processes-in-pg_stat_get.patch | binary/octet-stream | 16.4 KB |
0002-Expose-stats-for-all-backends.patch | binary/octet-stream | 7.1 KB |
0003-Add-backend_type-column-in-pg_stat_get_activity.patch | binary/octet-stream | 8.4 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Heikki Linnakangas | 2017-03-17 09:19:22 | Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled |
Previous Message | Heikki Linnakangas | 2017-03-17 09:18:54 | pgsql: Fix and simplify check for whether we're running as Windows serv |