On Thu, Mar 16, 2017 at 1:18 PM, Michael Paquier <michael.paqu...@gmail.com> wrote: > On Wed, Mar 15, 2017 at 9:14 PM, Kuntal Ghosh > <kuntalghosh.2...@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
0001-Infra-to-expose-all-backend-processes-in-pg_stat_get.patch
Description: binary/octet-stream
0002-Expose-stats-for-all-backends.patch
Description: binary/octet-stream
0003-Add-backend_type-column-in-pg_stat_get_activity.patch
Description: binary/octet-stream
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers