On Wed, Mar 22, 2017 at 9:54 PM, Robert Haas <robertmh...@gmail.com> wrote: > On Wed, Mar 22, 2017 at 12:20 PM, Robert Haas <robertmh...@gmail.com> wrote: >> On Wed, Mar 22, 2017 at 1:31 AM, Michael Paquier >> <michael.paqu...@gmail.com> wrote: >>> Okay, switched as ready for committer. One note for the committer >>> though: keeping the calls of pgstat_bestart() out of >>> BackgroundWorkerInitializeConnection() and >>> BackgroundWorkerInitializeConnectionByOid() keeps users the >>> possibility to not have backends connected to the database show up in >>> pg_stat_activity. This may matter for some users (cloud deployment for >>> example). I am as well in favor in keeping the work of those routines >>> minimal, without touching at pgstat. >> >> I think that's just inviting bugs of omission, in both core and >> extension code. I think it'd be much better to do this in a >> centralized place. > > I mean, your argument boils down to "somebody might want to > deliberately hide things from pg_stat_activity". But that's not > really a mode we support in general, and supporting it only for > certain cases doesn't seem like something that this patch should be > about. We could add an option to BackgroundWorkerInitializeConnection > and BackgroundWorkerInitializeConnectionByOid to suppress it, if it's > something that somebody wants, but actually I'd be more inclined to > think that everybody (who has a shared memory connection) should go > into the machinery and then security-filtering should be left to some > higher-level facility that can make policy decisions rather than being > hard-coded in the individual modules. > > But I'm slightly confused as to how this even arises. Background > workers already show up in pg_stat_activity output, or at least I sure > think they do. So why does this patch need to make any change to that > case at all? When we initialize a process through InitPostgres, the control returns from the method at different locations based on the process's type(as soon as its relevant information is initialized). Following is the order of returning a process from InitPostgres:
IsAutoVacuumLauncherProcess walsender not connected to a DB bgworker not connected to a DB Other processes using InitPostgres Before the patch, not all the processes are shown in pg_stat_activity. Hence, only at two locations in InitPostgres, we need to call pgstat_bestart() to initialize the entry for the respective process. But, since we're increasing the types of a process shown in pg_stat_activity, it seems to be reasonable to move the pgstat_bestart() call to a proc's main entry point. I've followed the same approach for auxiliary processes as well. AutovacuumLauncher - AutoVacLauncherMain() bgwriter BackgroundWriterMain() checkpointer CheckpointerMain() startup StartupProcessMain() walwriter WalWriterMain() walreceiver WalReceiverMain() walsender InitWalSender() Hence, to be consistent with others, bgworker processes can be initialized from BackgroundWorkerInitializeConnectionBy[Oid]. I've attached the updated patches which reflect the above change. PFA. -- 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
