Thread: Re: [HACKERS] exposing wait events for non-backends (was: Trackingwait event for latches)

On Tue, Oct 4, 2016 at 11:59 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Mon, Oct 3, 2016 at 8:43 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> The rest looks good to me. Thanks for the feedback and the time!
>
> Thanks for the fixes.  I committed this with an additional compile
> fix, but the buildfarm turned up a few more problems that my 'make
> check-world' didn't find.  Hopefully those are fixed now, but we'll
> see.

So, one of the problems in this patch as committed is that for any
process that doesn't show up in pg_stat_activity, there's no way to
see the wait event information.  That sucks.  I think there are
basically two ways to fix this:

1. Show all processes that have a PGPROC in pg_stat_activity,
including auxiliary processes and whatnot, and use some new field in
pg_stat_activity to indicate the process type.

2. Add a second view, say pg_stat_system_activity, to show the
processes that don't appear in pg_stat_activity.  A bunch of columns
could likely be omitted, but there would be some duplication, too.

Preferences?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas wrote:

> So, one of the problems in this patch as committed is that for any
> process that doesn't show up in pg_stat_activity, there's no way to
> see the wait event information.  That sucks.  I think there are
> basically two ways to fix this:
> 
> 1. Show all processes that have a PGPROC in pg_stat_activity,
> including auxiliary processes and whatnot, and use some new field in
> pg_stat_activity to indicate the process type.
> 
> 2. Add a second view, say pg_stat_system_activity, to show the
> processes that don't appear in pg_stat_activity.  A bunch of columns
> could likely be omitted, but there would be some duplication, too.
> 
> Preferences?

I vote 1.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2016-12-12 13:26:32 -0300, Alvaro Herrera wrote:
> Robert Haas wrote:
> 
> > So, one of the problems in this patch as committed is that for any
> > process that doesn't show up in pg_stat_activity, there's no way to
> > see the wait event information.  That sucks.  I think there are
> > basically two ways to fix this:
> > 
> > 1. Show all processes that have a PGPROC in pg_stat_activity,
> > including auxiliary processes and whatnot, and use some new field in
> > pg_stat_activity to indicate the process type.
> > 
> > 2. Add a second view, say pg_stat_system_activity, to show the
> > processes that don't appear in pg_stat_activity.  A bunch of columns
> > could likely be omitted, but there would be some duplication, too.
> > 
> > Preferences?
> 
> I vote 1.

+1

On Mon, Dec 12, 2016 at 11:19 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> So, one of the problems in this patch as committed is that for any
> process that doesn't show up in pg_stat_activity, there's no way to
> see the wait event information.  That sucks.  I think there are
> basically two ways to fix this:
>
> 1. Show all processes that have a PGPROC in pg_stat_activity,
> including auxiliary processes and whatnot, and use some new field in
> pg_stat_activity to indicate the process type.
>
> 2. Add a second view, say pg_stat_system_activity, to show the
> processes that don't appear in pg_stat_activity.  A bunch of columns
> could likely be omitted, but there would be some duplication, too.
>
> Preferences?

And now I'm noticing that Michael Paquier previously started a thread
on this problem which I failed to note before starting this one:

http://postgr.es/m/CAB7nPqSYN05rGsYCTahxTz+2hBikh7=m+hr2JTXaZv_Ei=qJAg@mail.gmail.com

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Dec 12, 2016 at 10:33 AM, Andres Freund <andres@anarazel.de> wrote:
> On 2016-12-12 13:26:32 -0300, Alvaro Herrera wrote:
>> Robert Haas wrote:

>>> 1. Show all processes that have a PGPROC in pg_stat_activity,

>>> 2. Add a second view, say pg_stat_system_activity,

>> I vote 1.
>
> +1

+1

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Dec 13, 2016 at 1:45 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> And now I'm noticing that Michael Paquier previously started a thread
> on this problem which I failed to note before starting this one:
>
> http://postgr.es/m/CAB7nPqSYN05rGsYCTahxTz+2hBikh7=m+hr2JTXaZv_Ei=qJAg@mail.gmail.com

Yes. I already had a look at what could be done to expose the
auxiliary processes in a system view with a set of proposals, though I
am not sure what would be better. It would be better to move the
discussion on this thread in my opinion, we are tracking down a
solution for a new problem.
-- 
Michael

On 13 December 2016 at 01:45, Kevin Grittner <kgrittn@gmail.com> wrote:
> On Mon, Dec 12, 2016 at 10:33 AM, Andres Freund <andres@anarazel.de> wrote:
>> On 2016-12-12 13:26:32 -0300, Alvaro Herrera wrote:
>>> Robert Haas wrote:
>
>>>> 1. Show all processes that have a PGPROC in pg_stat_activity,
>
>>>> 2. Add a second view, say pg_stat_system_activity,
>
>>> I vote 1.
>>
>> +1
>
> +1

I've long wanted the ability to see auxillary process state in
pg_stat_activity, so +1.

Right now pg_stat_replication is a join over pg_stat_get_activity()
and pg_stat_get_wal_senders() filtered for walsenders, and
pg_stat_activity is a view over pg_stat_get_activity() filtered for
processes with a user id. I'd really like to see walsenders in
pg_stat_activity now that logical decoding makes them more than dumb
I/O channels, as well as other auxillary processes.

We should probably expose a proc_type or something, with types:

* client_backend
* bgworker
* walsender
* autovacuum
* checkpointer
* bgwriter

for simpler filtering.

I don't think existing user code is likely to get upset by more
processes appearing in pg_stat_activity, and it'll be very handy.

-- Craig Ringer                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Dec 13, 2016 at 10:05 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
> We should probably expose a proc_type or something, with types:
>
> * client_backend
> * bgworker
> * walsender
> * autovacuum
> * checkpointer
> * bgwriter

A text field is adapted then, more than a single character.

> for simpler filtering.
>
> I don't think existing user code is likely to get upset by more
> processes appearing in pg_stat_activity, and it'll be very handy.

Indeed, for WAL senders now abusing of the query field is definitely
not consistent. Even if having this information is useful, adding such
a column would make sense. Still, one thing that is important to keep
with pg_stat_activity is the ability to count the number of
connections that are part of max_connections for monitoring purposes.
The docs definitely would need an example of such a query counting
only client_backend and WAL senders and tell users that this can be
used to count how many active connections there are.
-- 
Michael

On 13 December 2016 at 09:13, Michael Paquier <michael.paquier@gmail.com> wrote:
> On Tue, Dec 13, 2016 at 10:05 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
>> We should probably expose a proc_type or something, with types:
>>
>> * client_backend
>> * bgworker
>> * walsender
>> * autovacuum
>> * checkpointer
>> * bgwriter
>
> A text field is adapted then, more than a single character.
>
>> for simpler filtering.
>>
>> I don't think existing user code is likely to get upset by more
>> processes appearing in pg_stat_activity, and it'll be very handy.
>
> Indeed, for WAL senders now abusing of the query field is definitely
> not consistent. Even if having this information is useful, adding such
> a column would make sense. Still, one thing that is important to keep
> with pg_stat_activity is the ability to count the number of
> connections that are part of max_connections for monitoring purposes.
> The docs definitely would need an example of such a query counting
> only client_backend and WAL senders and tell users that this can be
> used to count how many active connections there are.

Good point.

No need for a new field, since a non-null client_port should be
sufficient.  But definitely documented.

-- Craig Ringer                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

On Mon, Dec 12, 2016 at 8:13 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Tue, Dec 13, 2016 at 10:05 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
>> We should probably expose a proc_type or something, with types:
>>
>> * client_backend
>> * bgworker
>> * walsender
>> * autovacuum
>> * checkpointer
>> * bgwriter
>
> A text field is adapted then, more than a single character.

Sure.

>> for simpler filtering.
>>
>> I don't think existing user code is likely to get upset by more
>> processes appearing in pg_stat_activity, and it'll be very handy.
>
> Indeed, for WAL senders now abusing of the query field is definitely
> not consistent. Even if having this information is useful, adding such
> a column would make sense. Still, one thing that is important to keep
> with pg_stat_activity is the ability to count the number of
> connections that are part of max_connections for monitoring purposes.
> The docs definitely would need an example of such a query counting
> only client_backend and WAL senders and tell users that this can be
> used to count how many active connections there are.

Let's confine ourselves to fixing one problem at a time.  I think we
can get where we want to be in this case by adding one new column and
some new rows to pg_stat_activity.  Michael, is that something you're
going to do?  If not, one of my esteemed colleagues here at
EnterpriseDB will have a try.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Dec 13, 2016 at 11:40:54AM -0500, Robert Haas wrote:
> Let's confine ourselves to fixing one problem at a time.  I think we
> can get where we want to be in this case by adding one new column and
> some new rows to pg_stat_activity.

Agreed. Let's also remove the abuse of WAL senders with the query field
at the same time.

> Michael, is that something you're
> going to do?  If not, one of my esteemed colleagues here at
> EnterpriseDB will have a try.

If I had received feedback on the other thread, I would have coded a
proposal of patch already. But as long as SCRAM is not done I will
restrain from taking an extra project. I am fine to do reviews as I already
looked at ways to solve the problem though. So if anybody has room to do
it please be my guest.

Regarding the way to solve things, I think that having in ProcGlobal an
array of PGPROC entries for each auxiliary process is the way to go, the
position of each entry in the array defining what the process type is.
That would waste some shared memory, but we are not talking about that
much here. That would as well remove the need of having checkpointerLatch,
walWriteLatch and the startup fields in ProcGlobal.
--
Michael

Hello everyone,

As discussed in this thread, I've attached a set of patches to show
auxiliary processes, autovacuum launcher and bgworker along with other
backends in pg_stat_activity. For now, 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. However, BackendStatusArray should be renamed to
something like 'ProcStatusArray' along with many others in pgstat.c
and pgstatfuncs.c(Ex: LocalPgBackendStatus etc). But, that needs a
major code refactoring. I can do the changes if we agree with that.

I've also kept a local array, named localBackendStatusIndex, which
stores the index of currently active backends from BackendStatusArray.
It assigns ids to currently active backend from 1 to the number of
active backends.(It is required in some pgstat_* functions, for
example: pg_stat_get_backend_idset). Hence, we are not affecting the
outputs of other sql functions apart from pg_stat_activity and
pg_stat_get_activity.

I've also added an extra column, named proc_type (suggested by Craig
and Robert), to indicate the type of process in pg_stat_activity view.
proc_type includes:

* client backend
* autovacuum launcher
* wal sender
* bgworker
* writer
* checkpointer
* wal writer
* wal receiver

Here is the present output with the relevant columns. (Didn't show
backend_start since it takes too long space)

postgres=# select pid, usesysid, application_name, wait_event_type,
wait_event, state, proc_type from pg_stat_activity;
  pid   | usesysid |       application_name       | wait_event_type |
   wait_event      | state  |      proc_type

--------+----------+------------------------------+-----------------+---------------------+--------+---------------------
 109945 |          |                              | Activity        |
AutoVacuumMain      | idle   | autovacuum launcher
 109947 |          | logical replication launcher | Activity        |
LogicalLauncherMain | idle   | bgworker
 109962 |       10 | walreceiver                  | Activity        |
WalSenderMain       | idle   | wal sender
 109976 |       10 | psql                         |                 |
                   | active | client backend
 109943 |          |                              | Activity        |
BgWriterMain        | idle   | writer
 109942 |          |                              | Activity        |
CheckpointerMain    | idle   | checkpointer
 109944 |          |                              | Activity        |
WalWriterMain       | idle   | wal writer
(7 rows)

Whereas, the output of other pgstat_* functions remains unchanged. For example,

postgres=# SELECT pg_stat_get_backend_pid(s.backendid) AS procpid,
       pg_stat_get_backend_activity(s.backendid) AS current_query
    FROM (SELECT pg_stat_get_backend_idset() AS backendid) AS s;
 procpid |                           current_query
---------+-------------------------------------------------------------------
  120713 | SELECT pg_stat_get_backend_pid(s.backendid) AS procpid,          +
         |        pg_stat_get_backend_activity(s.backendid) AS current_query+
         |     FROM (SELECT pg_stat_get_backend_idset() AS backendid) AS s;


Thoughts?

-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi Kuntal,

Patches apply and compile fine.  Works as advertised.

Some minor comments on the patches themselves.

In 0001:

- * pgstat_bestart() -
+ * pgstat_procstart() -
+ *
+ *  Initialize this process's entry in the PgBackendStatus array.
+ *  Called from InitPostgres and AuxiliaryProcessMain.

Not being called from AuxiliaryProcessMain().  Maybe leftover comment from
a previous version.  Actually I see that in patch 0002, Main() functions
of various auxiliary processes call pgstat_procstart, not
AuxiliaryProcessMain.

+     * user-defined functions which expects ids of backends starting from
1 to

s/expects/expect/g

+/*
+ * AuxiliaryPidGetProc -- get PGPROC for an auxiliary process
+ * given its PID
+ *
+ * Returns NULL if not found.
+ */
+PGPROC *
+AuxiliaryPidGetProc(int pid)
+{
+    PGPROC     *result;

Initialize to NULL so that the comment above is true. :)


In 0002:

@@ -248,6 +248,9 @@ BackgroundWriterMain(void)     */    prev_hibernate = false;

+    /* report walwriter process in the PgBackendStatus array */
+    pgstat_procstart();
+

s/walwriter/writer/g

Patch 0004 should update monitoring.sgml.

Thanks,
Amit

Hello Amit,

On Tue, Mar 7, 2017 at 4:24 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> Hi Kuntal,
>
> Patches apply and compile fine.  Works as advertised.
>
> Some minor comments on the patches themselves.
>
Thanks for the review.

> In 0001:
>
> - * pgstat_bestart() -
> + * pgstat_procstart() -
> + *
> + *  Initialize this process's entry in the PgBackendStatus array.
> + *  Called from InitPostgres and AuxiliaryProcessMain.
>
> Not being called from AuxiliaryProcessMain().  Maybe leftover comment from
> a previous version.  Actually I see that in patch 0002, Main() functions
> of various auxiliary processes call pgstat_procstart, not
> AuxiliaryProcessMain.
>
Fixed.

> +     * user-defined functions which expects ids of backends starting from
> 1 to
>
> s/expects/expect/g
>
Fixed.

> +/*
> + * AuxiliaryPidGetProc -- get PGPROC for an auxiliary process
> + * given its PID
> + *
> + * Returns NULL if not found.
> + */
> +PGPROC *
> +AuxiliaryPidGetProc(int pid)
> +{
> +    PGPROC     *result;
>
> Initialize to NULL so that the comment above is true. :)
>
Fixed.

> In 0002:
>
> @@ -248,6 +248,9 @@ BackgroundWriterMain(void)
>       */
>      prev_hibernate = false;
>
> +    /* report walwriter process in the PgBackendStatus array */
> +    pgstat_procstart();
> +
>
> s/walwriter/writer/g
Fixed.

> Patch 0004 should update monitoring.sgml.
Added.

I've attached the updated patches. PFA.


-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Perhaps I'm confused by the title of this thread/CF entry, but
background workers already do show up in pg_stat_activity.  (You can
verify that by testing the background sessions patch.)  So which
additional things are we aiming to see with this?

In practice, I think it's common to do a quick select * from
pg_stat_activity to determine whether a database instance is in use.
(You always see your own session, but that's easy to eyeball.)  If we
add all the various background processes by default, that will make
things harder, especially if there is no straightforward way to filter
them out.

Perhaps a pg_stat_user_* and pg_stat_system_* split like we have for
some of the statistics tables would be useful?

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Mar 9, 2017 at 2:30 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> Perhaps I'm confused by the title of this thread/CF entry, but
> background workers already do show up in pg_stat_activity.  (You can
> verify that by testing the background sessions patch.)  So which
> additional things are we aiming to see with this?

All the processes that don't normally show up in pg_stat_activity,
such as auxiliary processes.

> In practice, I think it's common to do a quick select * from
> pg_stat_activity to determine whether a database instance is in use.
> (You always see your own session, but that's easy to eyeball.)  If we
> add all the various background processes by default, that will make
> things harder, especially if there is no straightforward way to filter
> them out.
>
> Perhaps a pg_stat_user_* and pg_stat_system_* split like we have for
> some of the statistics tables would be useful?

I thought of the same kind of thing, and it was discussed upthread.
There seemed to be more votes for keeping it all in one view, but that
could change if more people vote.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi,

On 2017-03-09 14:30:21 -0500, Peter Eisentraut wrote:
> In practice, I think it's common to do a quick select * from
> pg_stat_activity to determine whether a database instance is in use.

> (You always see your own session, but that's easy to eyeball.)  If we
> add all the various background processes by default, that will make
> things harder, especially if there is no straightforward way to filter
> them out.

A good chunk of those still apply to database attached background
workers (say dropping a database, using it as a template) - so I'm not
really convinced that's an issue.

- Andres

Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Mar 9, 2017 at 2:30 PM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> In practice, I think it's common to do a quick select * from
>> pg_stat_activity to determine whether a database instance is in use.

> I thought of the same kind of thing, and it was discussed upthread.
> There seemed to be more votes for keeping it all in one view, but that
> could change if more people vote.

I've not been paying much attention to this thread, but it seems like
something that would help Peter's use-case and have other uses as well
is a new column that distinguishes different process types --- user
session, background worker, autovacuum worker, etc.
        regards, tom lane

On 2017-03-09 16:37:29 -0500, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Thu, Mar 9, 2017 at 2:30 PM, Peter Eisentraut
> > <peter.eisentraut@2ndquadrant.com> wrote:
> >> In practice, I think it's common to do a quick select * from
> >> pg_stat_activity to determine whether a database instance is in use.
> 
> > I thought of the same kind of thing, and it was discussed upthread.
> > There seemed to be more votes for keeping it all in one view, but that
> > could change if more people vote.
> 
> I've not been paying much attention to this thread, but it seems like
> something that would help Peter's use-case and have other uses as well
> is a new column that distinguishes different process types --- user
> session, background worker, autovacuum worker, etc.

The patches upthread add precisely such a column.

Andres

On Fri, Mar 10, 2017 at 3:11 AM, Andres Freund <andres@anarazel.de> wrote:
> On 2017-03-09 16:37:29 -0500, Tom Lane wrote:
>> Robert Haas <robertmhaas@gmail.com> writes:
>> > On Thu, Mar 9, 2017 at 2:30 PM, Peter Eisentraut
>> > <peter.eisentraut@2ndquadrant.com> wrote:
>> >> In practice, I think it's common to do a quick select * from
>> >> pg_stat_activity to determine whether a database instance is in use.
>>
>> > I thought of the same kind of thing, and it was discussed upthread.
>> > There seemed to be more votes for keeping it all in one view, but that
>> > could change if more people vote.
>>
>> I've not been paying much attention to this thread, but it seems like
>> something that would help Peter's use-case and have other uses as well
>> is a new column that distinguishes different process types --- user
>> session, background worker, autovacuum worker, etc.
>
> The patches upthread add precisely such a column.
>
The patch exposes auxiliary processes, autovacuum launcher and
bgworker along with other backends in pg_stat_activity. It also adds
an extra column, named proc_type (suggested by Craig
and Robert), to indicate the type of process in pg_stat_activity view.
proc_type includes:

* client backend
* autovacuum launcher
* wal sender
* bgworker
* writer
* checkpointer
* wal writer
* wal receiver

Here is the present output with the relevant columns.
postgres=# SELECT wait_event_type, wait_event, state, proc_type FROM
pg_stat_activity;wait_event_type |     wait_event      | state  |      proc_type
-----------------+---------------------+--------+---------------------Activity        | AutoVacuumMain      | idle   |
autovacuumlauncherActivity        | LogicalLauncherMain | idle   | bgworkerActivity        | WalSenderMain       | idle
 | wal sender                |                     | active | client backendActivity        | BgWriterMain        |
idle  | writerActivity        | CheckpointerMain    | idle   | checkpointerActivity        | WalWriterMain       | idle
 | wal writer
 
(7 rows)


-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com

On Fri, Mar 10, 2017 at 2:36 PM, Kuntal Ghosh
<kuntalghosh.2007@gmail.com> wrote:
> On Fri, Mar 10, 2017 at 3:11 AM, Andres Freund <andres@anarazel.de> wrote:
>> On 2017-03-09 16:37:29 -0500, Tom Lane wrote:
>>> Robert Haas <robertmhaas@gmail.com> writes:
>>> > On Thu, Mar 9, 2017 at 2:30 PM, Peter Eisentraut
>>> > <peter.eisentraut@2ndquadrant.com> wrote:
>>> >> In practice, I think it's common to do a quick select * from
>>> >> pg_stat_activity to determine whether a database instance is in use.
>>>
>>> > I thought of the same kind of thing, and it was discussed upthread.
>>> > There seemed to be more votes for keeping it all in one view, but that
>>> > could change if more people vote.
>>>
>>> I've not been paying much attention to this thread, but it seems like
>>> something that would help Peter's use-case and have other uses as well
>>> is a new column that distinguishes different process types --- user
>>> session, background worker, autovacuum worker, etc.
>>
>> The patches upthread add precisely such a column.
>>
> The patch exposes auxiliary processes, autovacuum launcher and
> bgworker along with other backends in pg_stat_activity. It also adds
> an extra column, named proc_type (suggested by Craig
> and Robert), to indicate the type of process in pg_stat_activity view.
> proc_type includes:
>
> * client backend
> * autovacuum launcher
> * wal sender
> * bgworker
> * writer
> * checkpointer
> * wal writer
> * wal receiver

"writer" would be better if defined as "background writer" instead?
You are forgetting in this list autovacuum workers and the startup
process, the latter is important for nodes in recovery.

> Here is the present output with the relevant columns.
> postgres=# SELECT wait_event_type, wait_event, state, proc_type FROM
> pg_stat_activity;
>  wait_event_type |     wait_event      | state  |      proc_type
> -----------------+---------------------+--------+---------------------
>  Activity        | AutoVacuumMain      | idle   | autovacuum launcher
>  Activity        | LogicalLauncherMain | idle   | bgworker
>  Activity        | WalSenderMain       | idle   | wal sender
>                  |                     | active | client backend
>  Activity        | BgWriterMain        | idle   | writer
>  Activity        | CheckpointerMain    | idle   | checkpointer
>  Activity        | WalWriterMain       | idle   | wal writer
> (7 rows)

Here is a first pass on this patch.

+     <entry>Type of the current server process. Possible types are:
+      autovacuum launcher, bgworker, checkpointer, client backend,
+      wal receiver, wal sender, wal writer and writer.
+     </entry>
There should be markups around those terms. Shouldn't "wal writer" and
"wal sender" and "wal receiver" not use any space? On HEAD a WAL
sender is defined as "walsender".

+ * Each auxliliary process also maintains a PgBackendStatus struct in shared
+ * memory.
s/auxliliary/auxiliary/.
void
-pgstat_bestart(void)
+pgstat_procstart(void)
I would not have thought that this patch justifies potentially
breaking extensions.

For WAL senders, the "query" field is still filled with "walsender".
This should be removed.

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

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

+PgBackendStatus *
+pgstat_fetch_stat_procentry(int procid)
+{
+   pgstat_read_current_status();
This function is used nowhere.
-- 
Michael

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

On Tue, Mar 14, 2017 at 7:22 AM, Kuntal Ghosh
<kuntalghosh.2007@gmail.com> wrote:
> 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.

I think the subject of this the thread, for which I'm probably to
blame, is bad terminology.  The processes we're talking about exposing
in pg_stat_activity here are really backends, too, I think.  They're
just ... special backends.  So I would tend to avoid any backend ->
process type of renaming.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Mar 15, 2017 at 4:52 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Mar 14, 2017 at 7:22 AM, Kuntal Ghosh
> <kuntalghosh.2007@gmail.com> wrote:
>> 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.
>
> I think the subject of this the thread, for which I'm probably to
> blame, is bad terminology.  The processes we're talking about exposing
> in pg_stat_activity here are really backends, too, I think.  They're
> just ... special backends.  So I would tend to avoid any backend ->
> process type of renaming.

FWIW, my impression on the matter matches what is written in this paragraph.
-- 
Michael

On Tue, Mar 14, 2017 at 1:50 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> "writer" would be better if defined as "background writer" instead?
> You are forgetting in this list autovacuum workers and the startup
> process, the latter is important for nodes in recovery.
>
Modified "writer" as "background writer". Included autovacuum workers
and startup process.

> +     <entry>Type of the current server process. Possible types are:
> +      autovacuum launcher, bgworker, checkpointer, client backend,
> +      wal receiver, wal sender, wal writer and writer.
> +     </entry>
> There should be markups around those terms. Shouldn't "wal writer" and
> "wal sender" and "wal receiver" not use any space? On HEAD a WAL
> sender is defined as "walsender".
Enclosed each type in <literal/>. Removed space from "wal sender" and
"wal receiver".

> For WAL senders, the "query" field is still filled with "walsender".
> This should be removed.
Fixed.

> @@ -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.
Removed this part as suggested.

I've attached the updated patches.
In 0001-Infra-to-expose-all-backend-processes-in-pg_stat_get.patch,
I've extended BackendStatusArray to to store auxiliary processes.
Backends
use slots indexed in the range from 1 to MaxBackends (inclusive), so
we can use MaxBackends + AuxBackendType + 1 as the index of the slot
for an auxiliary process. Also, I've added a backend_type to describe
the type of the backend process. The type includes:
* autovacuum launcher
* autovacuum worker
* background writer
* bgworker
* client backend
* checkpointer
* startup
* walreceiver
* walsender
* walwriter
In 0002-Expose-stats-for-all-backends.patch, I've added the required
code for reporting activity of different auxiliary processes,
autovacuum launcher and bgworker processes.
In 0003-Add-backend_type-column-in-pg_stat_get_activity.patch, I've
added a column named backend_type in pg_stat_get_activity to show the
type of the process to user.

There are some pg_stat_* functions where showing all the backends
doesn't make much sense. For example,
postgres=# SELECT pg_stat_get_backend_pid(s.backendid) AS pid,
       pg_stat_get_backend_activity(s.backendid) AS query
    FROM (SELECT pg_stat_get_backend_idset() AS backendid) AS s;
  pid  |                              query
-------+------------------------------------------------------------------
 17300 | SELECT pg_stat_get_backend_pid(s.backendid) AS pid,             +
       |        pg_stat_get_backend_activity(s.backendid) AS query       +
       |     FROM (SELECT pg_stat_get_backend_idset() AS backendid) AS s;
 16925 | <command string not enabled>
 16927 | <command string not enabled>
 16926 | <command string not enabled>
 16929 | <command string not enabled>
IMHO, this scenario can be easily avoided by filtering backends using
backend_type. I'm not sure whether we should add any logic in the code
for handling such cases. Thoughts?


-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Mar 15, 2017 at 9:14 PM, Kuntal Ghosh
<kuntalghosh.2007@gmail.com> wrote:
> I've attached the updated patches.

Thanks for the new versions. This begins to look really clear.

> In 0001-Infra-to-expose-all-backend-processes-in-pg_stat_get.patch,
> I've extended BackendStatusArray to to store auxiliary processes.
> Backends
> use slots indexed in the range from 1 to MaxBackends (inclusive), so
> we can use MaxBackends + AuxBackendType + 1 as the index of the slot
> for an auxiliary process. Also, I've added a backend_type to describe
> the type of the backend process. The type includes:
> * autovacuum launcher
> * autovacuum worker
> * background writer
> * bgworker
> * client backend
> * checkpointer
> * startup
> * walreceiver
> * walsender
> * walwriter
> In 0002-Expose-stats-for-all-backends.patch, I've added the required
> code for reporting activity of different auxiliary processes,
> autovacuum launcher and bgworker processes.
> In 0003-Add-backend_type-column-in-pg_stat_get_activity.patch, I've
> added a column named backend_type in pg_stat_get_activity to show the
> type of the process to user.
>
> There are some pg_stat_* functions where showing all the backends
> doesn't make much sense. For example,
> postgres=# SELECT pg_stat_get_backend_pid(s.backendid) AS pid,
>        pg_stat_get_backend_activity(s.backendid) AS query
>     FROM (SELECT pg_stat_get_backend_idset() AS backendid) AS s;
>   pid  |                              query
> -------+------------------------------------------------------------------
>  17300 | SELECT pg_stat_get_backend_pid(s.backendid) AS pid,             +
>        |        pg_stat_get_backend_activity(s.backendid) AS query       +
>        |     FROM (SELECT pg_stat_get_backend_idset() AS backendid) AS s;
>  16925 | <command string not enabled>
>  16927 | <command string not enabled>
>  16926 | <command string not enabled>
>  16929 | <command string not enabled>
> IMHO, this scenario can be easily avoided by filtering backends using
> backend_type. I'm not sure whether we should add any logic in the code
> for handling such cases. Thoughts?

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.

+/* Total number of backends including auxiliary */
+#define NumBackendStatSlots (MaxBackends + NUM_AUXPROCTYPES)
+
This variable remains localized in pgstat.c, so let's define it there.

+      <literal>bgworker</>, <literal>background writer</>,
That's really bike-shedding, but we could say here "background worker"
and be consistent with the rest.

+/* 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.

- * ----------
+ *
+ * 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.

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

+   /* 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().

+       /*
+        * Before returning, report autovacuum launcher process in the
+        * PgBackendStatus array.
+        */
+       pgstat_bestart();       return;
Wouldn't that be better in AutoVacLauncherMain()?

+       /*
+        * Before returning, report the background worker process in the
+        * PgBackendStatus array.
+        */
+       if (!bootstrap)
+           pgstat_bestart();
Ditto with BackgroundWriterMain().

@@ -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?
-- 
Michael

On Thu, Mar 16, 2017 at 1:18 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Wed, Mar 15, 2017 at 9:14 PM, Kuntal Ghosh
> <kuntalghosh.2007@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

On Tue, Mar 21, 2017 at 10:52 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
Thank you for the review.

> 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.
We reserve a slot for each possible BackendId, plus one for each
possible auxiliary process type. For a non-auxiliary process,
BackendId is used to refer the backend status in PgBackendStatus
array. So, a bgworker without any BackendId can't initialize its'
entry in PgBackendStatus array. In simple terms, it will not be shown
in pg_stat_activity. I've added some comments regarding this in
pgstat_bestart().
And, any bgworker having valid BackendId will have either a valid
userid or BOOTSTRAP_SUPERUSERID.

> 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.
>
In this case, pgstat_bestart() shouldn't be called from this module as
the spawned bgworker will have invalid BackendId. By the way, thanks
for sharing the repo link. Found a lot of interesting things to
explore and learn. :)

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

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

> Except for the two issues pointed out in this email, I am pretty cool
> with this patch. Nice work.
Thank you. :)

Please find the updated patches.


-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Mar 21, 2017 at 10:37 PM, Kuntal Ghosh
<kuntalghosh.2007@gmail.com> wrote:
> On Tue, Mar 21, 2017 at 10:52 AM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
> We reserve a slot for each possible BackendId, plus one for each
> possible auxiliary process type. For a non-auxiliary process,
> BackendId is used to refer the backend status in PgBackendStatus
> array. So, a bgworker without any BackendId can't initialize its'
> entry in PgBackendStatus array. In simple terms, it will not be shown
> in pg_stat_activity. I've added some comments regarding this in
> pgstat_bestart().

- * Called from InitPostgres.
- * MyDatabaseId, session userid, and application_name must be set
- * (hence, this cannot be combined with pgstat_initialize).
+ *
+ * Apart from auxiliary processes, MyBackendId, MyDatabaseId,
+ * session userid, and application_name must be set for a
+ * backend (hence, this cannot be combined with pgstat_initialize).
That looks right.

> And, any bgworker having valid BackendId will have either a valid
> userid or BOOTSTRAP_SUPERUSERID.

So looking at the area of the code in more details, my memories have
failed me a bit. InitPostgres() is setting up MyBackendId via
SharedInvalBackendInit(), something that will never be called for
backend processes not connected to a database. The bgworker for
logical replication does not do that.

>> 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.
>>
> In this case, pgstat_bestart() shouldn't be called from this module as
> the spawned bgworker will have invalid BackendId. By the way, thanks
> for sharing the repo link. Found a lot of interesting things to
> explore and learn. :)

RIP to this process. Not sure if that's worth the documentation. I
imagine that people usually implement bgworkers by deleting lines in
worker_spi and keeping its structure.

>> Except for the two issues pointed out in this email, I am pretty cool
>> with this patch. Nice work.
> Thank you. :)
>
> Please find the updated patches.

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.
       if (!bootstrap)           CommitTransactionCommand();
+       return;
Some useless noise here.
-- 
Michael

On Wed, Mar 22, 2017 at 1:31 AM, Michael Paquier
<michael.paquier@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.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Mar 22, 2017 at 12:20 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, Mar 22, 2017 at 1:31 AM, Michael Paquier
> <michael.paquier@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?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Mar 23, 2017 at 1:24 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> 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 working on a couple of bgworkers some time ago, I recalled that
they only showed up in pg_stat_activity only if calling
pgstat_report_activity() in them. Just looking again, visibly I was
mistaken, they do indeed show up when if WaitLatch() or
pgstat_report_activity() are not used. Please let me discard that
remark.
-- 
Michael

On Wed, Mar 22, 2017 at 9:54 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, Mar 22, 2017 at 12:20 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Wed, Mar 22, 2017 at 1:31 AM, Michael Paquier
>> <michael.paquier@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Mar 23, 2017 at 8:19 PM, Kuntal Ghosh
<kuntalghosh.2007@gmail.com> wrote:
> Hence, to be consistent with others, bgworker processes can be
> initialized from BackgroundWorkerInitializeConnectionBy[Oid].

Yeah, I am fine with that. Thanks for the updated versions.
-- 
Michael

On Thu, Mar 23, 2017 at 7:29 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Thu, Mar 23, 2017 at 8:19 PM, Kuntal Ghosh
> <kuntalghosh.2007@gmail.com> wrote:
>> Hence, to be consistent with others, bgworker processes can be
>> initialized from BackgroundWorkerInitializeConnectionBy[Oid].
>
> Yeah, I am fine with that. Thanks for the updated versions.

I think this is still not good.  The places where pgstat_bestart() has
been added are not even correct.  For example, the call added to
BackgroundWriterMain() occurs after the section that does
error-recovery, so it would get repeated every time the background
writer recovers from an error.  There are similar problems elsewhere.
Furthermore, although in theory there's an idea here that we're making
it no longer the responsibility of InitPostgres() to call
pgstat_bestart(), the patch as proposed only removes one of the two
calls, so we really don't even have a consistent practice.  I think
it's better to go with the idea of having InitPostgres() be
responsible for calling this for regular backends, and
AuxiliaryProcessMain() for auxiliary backends.  That involves
substantially fewer calls to pgstat_bestart() and they are spread
across only two functions, which IMHO makes fewer bugs of omission a
lot less likely.

Updated patch with that change attached.  In addition to that
modification, I made some other alterations:

- I changed the elog() for the can't-happen case in pgstat_bestart()
from PANIC to FATAL.  The contents of shared memory aren't corrupted,
so I think PANIC is overkill.

- I tweaked the comment in WalSndLoop() just before the
pgstat_report_activity() call to accurately reflect what the effect of
that call now is.

- I changed the column ordering in pg_stat_get_activity() to put
backend_type with the other columns that appear in pg_stat_activity,
rather than (as the patch did) grouping it with the ones that appear
in pg_stat_ssl.

- I modified the code to tolerate a NULL return from
AuxiliaryPidGetProc().  I am pretty sure that without that there's a
race condition that could lead to a crash if somebody tried to call
this function just as an auxiliary process was terminating.

- I updated the documentation slightly.

- I rebased over some conflicting commits.

If there aren't objections, I plan to commit this version.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Mar 24, 2017 at 9:23 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Mar 23, 2017 at 7:29 AM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Thu, Mar 23, 2017 at 8:19 PM, Kuntal Ghosh
>> <kuntalghosh.2007@gmail.com> wrote:
>>> Hence, to be consistent with others, bgworker processes can be
>>> initialized from BackgroundWorkerInitializeConnectionBy[Oid].
>>
>> Yeah, I am fine with that. Thanks for the updated versions.
>
> I think this is still not good.  The places where pgstat_bestart() has
> been added are not even correct.  For example, the call added to
> BackgroundWriterMain() occurs after the section that does
> error-recovery, so it would get repeated every time the background
> writer recovers from an error.  There are similar problems elsewhere.
> Furthermore, although in theory there's an idea here that we're making
> it no longer the responsibility of InitPostgres() to call
> pgstat_bestart(), the patch as proposed only removes one of the two
> calls, so we really don't even have a consistent practice.  I think
> it's better to go with the idea of having InitPostgres() be
> responsible for calling this for regular backends, and
> AuxiliaryProcessMain() for auxiliary backends.  That involves
> substantially fewer calls to pgstat_bestart() and they are spread
> across only two functions, which IMHO makes fewer bugs of omission a
> lot less likely.
Agreed. Calling it from  InitPostgres() and AuxiliaryProcessMain()
seems correct because of the following two reasons as you've mentioned
up in the thread:
1. security-filtering should be left to some higher-level facility
that can make policy decisions rather than being hard-coded in the
individual modules.
2. makes fewer bugs of omission a lot less likely.

> Updated patch with that change attached.  In addition to that
> modification, I made some other alterations:
>
> - I changed the elog() for the can't-happen case in pgstat_bestart()
> from PANIC to FATAL.  The contents of shared memory aren't corrupted,
> so I think PANIC is overkill.
Agreed and duly noted for future.

> - I tweaked the comment in WalSndLoop() just before the
> pgstat_report_activity() call to accurately reflect what the effect of
> that call now is.
>
> - I changed the column ordering in pg_stat_get_activity() to put
> backend_type with the other columns that appear in pg_stat_activity,
> rather than (as the patch did) grouping it with the ones that appear
> in pg_stat_ssl.
Thank you.

> - I modified the code to tolerate a NULL return from
> AuxiliaryPidGetProc().  I am pretty sure that without that there's a
> race condition that could lead to a crash if somebody tried to call
> this function just as an auxiliary process was terminating.
Wow. Haven't thought of that. If it's called after
AuxiliaryProcKill(), a crash is evident.

> - I updated the documentation slightly.
Looks good to me.

> - I rebased over some conflicting commits.
Sorry for the inconveniences. It seems that the conflicting changes
occurred within few hours after I've posted the patch.

> If there aren't objections, I plan to commit this version.
Thank you for looking into the patch and doing the necessary changes.
All the changes look good to me and I've tested the feature and it has
passed all the regression tests.



-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com

On Sat, Mar 25, 2017 at 5:26 PM, Kuntal Ghosh
<kuntalghosh.2007@gmail.com> wrote:
> On Fri, Mar 24, 2017 at 9:23 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> I think this is still not good.  The places where pgstat_bestart() has
>> been added are not even correct.  For example, the call added to
>> BackgroundWriterMain() occurs after the section that does
>> error-recovery, so it would get repeated every time the background
>> writer recovers from an error.  There are similar problems elsewhere.
>> Furthermore, although in theory there's an idea here that we're making
>> it no longer the responsibility of InitPostgres() to call
>> pgstat_bestart(), the patch as proposed only removes one of the two
>> calls, so we really don't even have a consistent practice.  I think
>> it's better to go with the idea of having InitPostgres() be
>> responsible for calling this for regular backends, and
>> AuxiliaryProcessMain() for auxiliary backends.  That involves
>> substantially fewer calls to pgstat_bestart() and they are spread
>> across only two functions, which IMHO makes fewer bugs of omission a
>> lot less likely.
>
> Agreed. Calling it from  InitPostgres() and AuxiliaryProcessMain()
> seems correct because of the following two reasons as you've mentioned
> up in the thread:
> 1. security-filtering should be left to some higher-level facility
> that can make policy decisions rather than being hard-coded in the
> individual modules.
> 2. makes fewer bugs of omission a lot less likely.

Okay, fine for me.

>> - I modified the code to tolerate a NULL return from
>> AuxiliaryPidGetProc().  I am pretty sure that without that there's a
>> race condition that could lead to a crash if somebody tried to call
>> this function just as an auxiliary process was terminating.
>
> Wow. Haven't thought of that. If it's called after
> AuxiliaryProcKill(), a crash is evident.

This one is a good catch.
-- 
Michael

Thank you Robert for committing the patch.

commit fc70a4b0df38bda6a13941f1581f25fbb643c7f3

I've changed the status to Committed.

On Mon, Mar 27, 2017 at 6:09 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Sat, Mar 25, 2017 at 5:26 PM, Kuntal Ghosh
> <kuntalghosh.2007@gmail.com> wrote:
>> On Fri, Mar 24, 2017 at 9:23 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>> I think this is still not good.  The places where pgstat_bestart() has
>>> been added are not even correct.  For example, the call added to
>>> BackgroundWriterMain() occurs after the section that does
>>> error-recovery, so it would get repeated every time the background
>>> writer recovers from an error.  There are similar problems elsewhere.
>>> Furthermore, although in theory there's an idea here that we're making
>>> it no longer the responsibility of InitPostgres() to call
>>> pgstat_bestart(), the patch as proposed only removes one of the two
>>> calls, so we really don't even have a consistent practice.  I think
>>> it's better to go with the idea of having InitPostgres() be
>>> responsible for calling this for regular backends, and
>>> AuxiliaryProcessMain() for auxiliary backends.  That involves
>>> substantially fewer calls to pgstat_bestart() and they are spread
>>> across only two functions, which IMHO makes fewer bugs of omission a
>>> lot less likely.
>>
>> Agreed. Calling it from  InitPostgres() and AuxiliaryProcessMain()
>> seems correct because of the following two reasons as you've mentioned
>> up in the thread:
>> 1. security-filtering should be left to some higher-level facility
>> that can make policy decisions rather than being hard-coded in the
>> individual modules.
>> 2. makes fewer bugs of omission a lot less likely.
>
> Okay, fine for me.
>
>>> - I modified the code to tolerate a NULL return from
>>> AuxiliaryPidGetProc().  I am pretty sure that without that there's a
>>> race condition that could lead to a crash if somebody tried to call
>>> this function just as an auxiliary process was terminating.
>>
>> Wow. Haven't thought of that. If it's called after
>> AuxiliaryProcKill(), a crash is evident.
>
> This one is a good catch.
> --
> Michael



-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com