On Tue, Mar 14, 2017 at 1:50 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> Here is a first pass on this patch.
Thanks Michael for the review.
>
> void
> -pgstat_bestart(void)
> +pgstat_procstart(void)
> I would not have thought that this patch justifies potentially
> breaking extensions.
Since I'm using this method to start all kind of processes including
client backends, auxiliary procs etc., I thought of changing the name
as above. But, this surely doesn't justify breaking extensions. So,
I'll change it back to pgstat_bestart.
> @@ -365,6 +368,8 @@ CheckpointerMain(void)
> */
> AbsorbFsyncRequests();
>
> + pgstat_report_activity(STATE_RUNNING, NULL);
> +
> if (got_SIGHUP)
> It seems to me that what we want to know here are only the wait
> events, so I think that you had better drop this portion of the patch.
> It is hard to decide if an auxiliary process is idle or running, and
> this routine is a concept that applies to database backends running
> queries.
I see your point. I'll remove this part.
> -static LocalPgBackendStatus *localBackendStatusTable = NULL;
> +
> +/* Status for backends and auxiliary processes */
> +static LocalPgBackendStatus *localProcStatusTable = NULL;
> I don't quite understand why you need to have two layers here,
> wouldn't it be more simple to just extend localBackendStatusTable with
> enough slots for the system processes? That would be also less
> bug-prone. You need to be careful to have a correct mapping to
> include:
> - auxiliary processes, up to 4 slots used.
> - bgworker processes, decided by GUC.
> - autovacuum workers, decided by GUC.
I do have extended localBackendStatusTable with slots for non-backend
processes. But, I've renamed it as localProcStatusTable since it
includes all processes. I'll keep the variable name as
localBackendStatusTable in the updated patch to avoid any confusion.
I've extended BackendStatusArray to store auxiliary processes.
Backends use slots indexed in the range from 1 to MaxBackends
(inclusive), so we use MaxBackends + AuxProcType + 1 as the index of
the slot for an auxiliary process.
Additionally, to store the index of currently active client backends
from localBackendStatusTable, I've added an array named
localBackendStatusIndex. This is useful for generating a set of
currently active client backend ID numbers (from 1 to the number of
active client backends). These IDs are used for some pgstat_*
functions relevant to client processes, e.g.,
pg_stat_get_backend_activity, pg_stat_get_backend_client_port etc.
Any suggestions?
--
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com
