Why does logical replication launcher set application_name?

On Wed, Apr 12, 2017 at 3:40 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I notice looking at pg_stat_activity that the logical replication launcher
sets its application_name to "logical replication launcher". This seems
inconsistent (no other standard background process sets application_name),
redundant with other columns that already tell you what it is, and an
unreasonable consumption of horizontal space in the tabular output.
Can we drop that? If we do have to have something like that, what about
putting it in the "query" field where it's much less likely to be
substantially wider than any other entry in the column?

It seems to me that the logic behind that is to be able to identify
easily the logical replication launcher in pg_stat_activity, so using
the query field instead sounds fine for me as we need another way than
backend_type to guess what is this bgworker.
--
Michael

--
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, Apr 11, 2017 at 8:11 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Wed, Apr 12, 2017 at 3:40 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I notice looking at pg_stat_activity that the logical replication launcher
sets its application_name to "logical replication launcher". This seems
inconsistent (no other standard background process sets application_name),
redundant with other columns that already tell you what it is, and an
unreasonable consumption of horizontal space in the tabular output.
Can we drop that? If we do have to have something like that, what about
putting it in the "query" field where it's much less likely to be
substantially wider than any other entry in the column?

It seems to me that the logic behind that is to be able to identify
easily the logical replication launcher in pg_stat_activity, so using
the query field instead sounds fine for me as we need another way than
backend_type to guess what is this bgworker.

Wait, why do we need two ways?

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

--
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, Apr 12, 2017 at 6:02 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, Apr 11, 2017 at 8:11 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Wed, Apr 12, 2017 at 3:40 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I notice looking at pg_stat_activity that the logical replication launcher
sets its application_name to "logical replication launcher". This seems
inconsistent (no other standard background process sets application_name),
redundant with other columns that already tell you what it is, and an
unreasonable consumption of horizontal space in the tabular output.
Can we drop that? If we do have to have something like that, what about
putting it in the "query" field where it's much less likely to be
substantially wider than any other entry in the column?

It seems to me that the logic behind that is to be able to identify
easily the logical replication launcher in pg_stat_activity, so using
the query field instead sounds fine for me as we need another way than
backend_type to guess what is this bgworker.

Wait, why do we need two ways?

For backend_type=background worker, application_name shows the name of
the background worker (BackgroundWorker->bgw_name). I think we need
some way to distinguish among different background workers. But,
application_name may not be the correct field to show the information.

--
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 12 April 2017 at 13:34, Kuntal Ghosh <kuntalghosh.2007@gmail.com> wrote:

For backend_type=background worker, application_name shows the name of
the background worker (BackgroundWorker->bgw_name). I think we need
some way to distinguish among different background workers. But,
application_name may not be the correct field to show the information.

It's better than (ab)using 'query' IMO.

I'd rather an abbreviated entry to address Tom's concerns about
format. 'lrlaunch' or whatever.

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

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

On 12/04/17 02:32, Robert Haas wrote:

On Tue, Apr 11, 2017 at 8:11 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Wed, Apr 12, 2017 at 3:40 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I notice looking at pg_stat_activity that the logical replication launcher
sets its application_name to "logical replication launcher". This seems
inconsistent (no other standard background process sets application_name),
redundant with other columns that already tell you what it is, and an
unreasonable consumption of horizontal space in the tabular output.
Can we drop that? If we do have to have something like that, what about
putting it in the "query" field where it's much less likely to be
substantially wider than any other entry in the column?

It seems to me that the logic behind that is to be able to identify
easily the logical replication launcher in pg_stat_activity, so using
the query field instead sounds fine for me as we need another way than
backend_type to guess what is this bgworker.

Wait, why do we need two ways?

What do you mean by two ways? There is no way if we don't set anything.

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

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

Craig Ringer <craig@2ndquadrant.com> writes:

On 12 April 2017 at 13:34, Kuntal Ghosh <kuntalghosh.2007@gmail.com> wrote:

For backend_type=background worker, application_name shows the name of
the background worker (BackgroundWorker->bgw_name). I think we need
some way to distinguish among different background workers. But,
application_name may not be the correct field to show the information.

It's better than (ab)using 'query' IMO.

I'd rather an abbreviated entry to address Tom's concerns about
format. 'lrlaunch' or whatever.

Basically the problem I've got with the LR launcher is that it looks
utterly unlike any other background process in pg_stat_activity.
Leaving out all-null columns to make my point:

regression=# select pid,usesysid,usename,application_name,backend_start,wait_event_type,wait_event,backend_type from pg_stat_activity where application_name != 'psql';
pid | usesysid | usename | application_name | backend_start | wait_event_type | wait_event | backend_type
-------+----------+----------+------------------------------+-------------------------------+-----------------+---------------------+---------------------
25416 | | | | 2017-04-16 12:32:46.987076-04 | Activity | AutoVacuumMain | autovacuum launcher
25418 | 10 | postgres | logical replication launcher | 2017-04-16 12:32:46.988859-04 | Activity | LogicalLauncherMain | background worker
25414 | | | | 2017-04-16 12:32:46.986745-04 | Activity | BgWriterHibernate | background writer
25413 | | | | 2017-04-16 12:32:46.986885-04 | Activity | CheckpointerMain | checkpointer
25415 | | | | 2017-04-16 12:32:46.9871-04 | Activity | WalWriterMain | walwriter
(5 rows)

Why has it got non-null entries for usesysid and usename, never mind
application_name? Why does it not follow the well-established convention
that backend_type is what identifies background processes?

I'm sure the answer to those questions is "it's an implementation artifact
from using the generic bgworker infrastructure", but that does not make it
look any less like sloppy, half-finished work.

If it is a limitation of the bgworker infrastructure that we can't make
the LR processes look more like the other kinds of built-in processes,
then I think we need to fix that limitation. And I further assert that
we need to do it for v10, because once we ship v10 people will adjust
their tools for this bogus output, and we'll face complaints about
backwards compatibility if we fix it later.

regards, tom lane

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

On 16/04/17 18:47, Tom Lane wrote:

Craig Ringer <craig@2ndquadrant.com> writes:

On 12 April 2017 at 13:34, Kuntal Ghosh <kuntalghosh.2007@gmail.com> wrote:

For backend_type=background worker, application_name shows the name of
the background worker (BackgroundWorker->bgw_name). I think we need
some way to distinguish among different background workers. But,
application_name may not be the correct field to show the information.

It's better than (ab)using 'query' IMO.

I'd rather an abbreviated entry to address Tom's concerns about
format. 'lrlaunch' or whatever.

Basically the problem I've got with the LR launcher is that it looks
utterly unlike any other background process in pg_stat_activity.
Leaving out all-null columns to make my point:

regression=# select pid,usesysid,usename,application_name,backend_start,wait_event_type,wait_event,backend_type from pg_stat_activity where application_name != 'psql';
pid | usesysid | usename | application_name | backend_start | wait_event_type | wait_event | backend_type
-------+----------+----------+------------------------------+-------------------------------+-----------------+---------------------+---------------------
25416 | | | | 2017-04-16 12:32:46.987076-04 | Activity | AutoVacuumMain | autovacuum launcher
25418 | 10 | postgres | logical replication launcher | 2017-04-16 12:32:46.988859-04 | Activity | LogicalLauncherMain | background worker
25414 | | | | 2017-04-16 12:32:46.986745-04 | Activity | BgWriterHibernate | background writer
25413 | | | | 2017-04-16 12:32:46.986885-04 | Activity | CheckpointerMain | checkpointer
25415 | | | | 2017-04-16 12:32:46.9871-04 | Activity | WalWriterMain | walwriter
(5 rows)

Why has it got non-null entries for usesysid and usename, never mind
application_name? Why does it not follow the well-established convention
that backend_type is what identifies background processes?

I'm sure the answer to those questions is "it's an implementation artifact
from using the generic bgworker infrastructure", but that does not make it
look any less like sloppy, half-finished work.

If it is a limitation of the bgworker infrastructure that we can't make
the LR processes look more like the other kinds of built-in processes,
then I think we need to fix that limitation. And I further assert that
we need to do it for v10, because once we ship v10 people will adjust
their tools for this bogus output, and we'll face complaints about
backwards compatibility if we fix it later.

It's indeed how bgworker infrastructure is reporting it. That being
said, since LR processes are in-core, we can add exception for them in
pgstat_bestart() so that they are reported more like other builtin
processes. We could also try to add api for bgworker processes to change
how they are reported so that any future workers (and all the external
workers) can be reported properly as well, but that seems better fit for
v11 at this point since it would be good if we had some discussion for
how that should look like.

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

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

On 16/04/17 22:27, Petr Jelinek wrote:

On 16/04/17 18:47, Tom Lane wrote:

Craig Ringer <craig@2ndquadrant.com> writes:

On 12 April 2017 at 13:34, Kuntal Ghosh <kuntalghosh.2007@gmail.com> wrote:

For backend_type=background worker, application_name shows the name of
the background worker (BackgroundWorker->bgw_name). I think we need
some way to distinguish among different background workers. But,
application_name may not be the correct field to show the information.

It's better than (ab)using 'query' IMO.

I'd rather an abbreviated entry to address Tom's concerns about
format. 'lrlaunch' or whatever.

Basically the problem I've got with the LR launcher is that it looks
utterly unlike any other background process in pg_stat_activity.
Leaving out all-null columns to make my point:

regression=# select pid,usesysid,usename,application_name,backend_start,wait_event_type,wait_event,backend_type from pg_stat_activity where application_name != 'psql';
pid | usesysid | usename | application_name | backend_start | wait_event_type | wait_event | backend_type
-------+----------+----------+------------------------------+-------------------------------+-----------------+---------------------+---------------------
25416 | | | | 2017-04-16 12:32:46.987076-04 | Activity | AutoVacuumMain | autovacuum launcher
25418 | 10 | postgres | logical replication launcher | 2017-04-16 12:32:46.988859-04 | Activity | LogicalLauncherMain | background worker
25414 | | | | 2017-04-16 12:32:46.986745-04 | Activity | BgWriterHibernate | background writer
25413 | | | | 2017-04-16 12:32:46.986885-04 | Activity | CheckpointerMain | checkpointer
25415 | | | | 2017-04-16 12:32:46.9871-04 | Activity | WalWriterMain | walwriter
(5 rows)

Why has it got non-null entries for usesysid and usename, never mind
application_name? Why does it not follow the well-established convention
that backend_type is what identifies background processes?

I'm sure the answer to those questions is "it's an implementation artifact
from using the generic bgworker infrastructure", but that does not make it
look any less like sloppy, half-finished work.

If it is a limitation of the bgworker infrastructure that we can't make
the LR processes look more like the other kinds of built-in processes,
then I think we need to fix that limitation. And I further assert that
we need to do it for v10, because once we ship v10 people will adjust
their tools for this bogus output, and we'll face complaints about
backwards compatibility if we fix it later.

It's indeed how bgworker infrastructure is reporting it. That being
said, since LR processes are in-core, we can add exception for them in
pgstat_bestart() so that they are reported more like other builtin
processes. We could also try to add api for bgworker processes to change
how they are reported so that any future workers (and all the external
workers) can be reported properly as well, but that seems better fit for
v11 at this point since it would be good if we had some discussion for
how that should look like.

Hmm so I took a look at code today with intention to implement this, but
it does not seem as straightforward as I'd hoped due to pgstat_bestart()
being called quite early in startup.

We can definitely easily detect that the bgworker is internal one by
library_name equals 'postgres' so we can easily remove the usesysid and
usename based on that. But that does not solve the issue of identifying
the processes in pg_stat_activity as logical replication laucher/worker.
I wonder if it would be okay to set backend_type to bgw_name for
internal workers and just leave the external ones as it is (or solve
them in v11 with some proper API) as we can control the length of name
there (it will still be longer than the values for other things but
maybe not too much).

Does that sound reasonable enough for v10?

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

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

On 4/18/17 12:13, Petr Jelinek wrote:

We can definitely easily detect that the bgworker is internal one by
library_name equals 'postgres' so we can easily remove the usesysid and
usename based on that.

I don't see why we need to do that. It is showing the correct
information, isn't it?

But that does not solve the issue of identifying
the processes in pg_stat_activity as logical replication laucher/worker.
I wonder if it would be okay to set backend_type to bgw_name for
internal workers and just leave the external ones as it is (or solve
them in v11 with some proper API) as we can control the length of name
there (it will still be longer than the values for other things but
maybe not too much).

I think showing bgw_name as backend_type always sounds reasonable. No
need to treat external implementations differently.

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

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

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

I think showing bgw_name as backend_type always sounds reasonable. No
need to treat external implementations differently.

That's definitely an approach we could use. It would encourage people
to use short bgw_names, which is a constraint that wasn't especially
apparent before, but I don't think that's a bad thing.

regards, tom lane

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

On 18/04/17 18:24, Peter Eisentraut wrote:

On 4/18/17 12:13, Petr Jelinek wrote:

We can definitely easily detect that the bgworker is internal one by
library_name equals 'postgres' so we can easily remove the usesysid and
usename based on that.

I don't see why we need to do that. It is showing the correct
information, isn't it?

It does, but it's also one of the things Tom complained about and I
think he is right in that at least values for launcher should be
filtered out there as there is not much meaning in what is shown for
launcher. The ugly part is that we can't tell it's launcher in any other
way than comparing bgw_library_name and bgw_function_name to specific
values.

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

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

Petr Jelinek <petr.jelinek@2ndquadrant.com> writes:

On 18/04/17 18:24, Peter Eisentraut wrote:

I don't see why we need to do that. It is showing the correct
information, isn't it?

It does, but it's also one of the things Tom complained about and I
think he is right in that at least values for launcher should be
filtered out there as there is not much meaning in what is shown for
launcher. The ugly part is that we can't tell it's launcher in any other
way than comparing bgw_library_name and bgw_function_name to specific
values.

I think you're thinking about it wrong. To my mind the issue is that
there should be some generic way to determine that a bgworker process
is or is not laboring on behalf of an identifiable user. It's great
that we can tell which user it is when there is one, but clearly some
bgworkers will be providing general services that aren't associated with
a single user. So it should be possible to set the userID to zero or
some such when the bgworker is one that isn't associated with a
particular user. Maybe the owning user needs to become an additional
parameter passed in struct BackgroundWorker.

regards, tom lane

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

On 18/04/17 19:18, Tom Lane wrote:

Petr Jelinek <petr.jelinek@2ndquadrant.com> writes:

On 18/04/17 18:24, Peter Eisentraut wrote:

I don't see why we need to do that. It is showing the correct
information, isn't it?

It does, but it's also one of the things Tom complained about and I
think he is right in that at least values for launcher should be
filtered out there as there is not much meaning in what is shown for
launcher. The ugly part is that we can't tell it's launcher in any other
way than comparing bgw_library_name and bgw_function_name to specific
values.

I think you're thinking about it wrong. To my mind the issue is that
there should be some generic way to determine that a bgworker process
is or is not laboring on behalf of an identifiable user. It's great
that we can tell which user it is when there is one, but clearly some
bgworkers will be providing general services that aren't associated with
a single user. So it should be possible to set the userID to zero or
some such when the bgworker is one that isn't associated with a
particular user. Maybe the owning user needs to become an additional
parameter passed in struct BackgroundWorker.

We can already do that. In fact after I wrote the above I thought we
could add some kind of boolean in the style of am_bootstrap_superuser as
BOOTSTRAP_SUPERUSER is what those bgworkers get assigned. I don't like
the name much though (am_bootstrap_superuser) as this should not be
associated with bootstrap IMHO.

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

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

On 4/18/17 12:37, Tom Lane wrote:

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

I think showing bgw_name as backend_type always sounds reasonable. No
need to treat external implementations differently.

That's definitely an approach we could use. It would encourage people
to use short bgw_names, which is a constraint that wasn't especially
apparent before, but I don't think that's a bad thing.

Actually, bgw_name is probably not food for
pg_stat_activity.backend_type, since it's often not the same for all
background workers of the same kind. For example, it might be "parallel
worker for PID %d". Ideally, a background worker would have a bgw_type
field and perhaps a bgw_name_extra field. However, a background worker
might also want to update some part of that dynamically, to change the
process title. Many details depend on the particular background workers.

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

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

On 4/18/17 13:18, Tom Lane wrote:

I think you're thinking about it wrong. To my mind the issue is that
there should be some generic way to determine that a bgworker process
is or is not laboring on behalf of an identifiable user. It's great
that we can tell which user it is when there is one, but clearly some
bgworkers will be providing general services that aren't associated with
a single user. So it should be possible to set the userID to zero or
some such when the bgworker is one that isn't associated with a
particular user. Maybe the owning user needs to become an additional
parameter passed in struct BackgroundWorker.

I think this is probably a problem particular to the logical replication
launcher. Other background workers either do work as a particular user,
as you say, or don't touch the database at all. So a localized hack or
a simple hide-the-user flag might suffice for now.

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

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

On 20/04/17 21:33, Peter Eisentraut wrote:

On 4/18/17 13:18, Tom Lane wrote:

I think you're thinking about it wrong. To my mind the issue is that
there should be some generic way to determine that a bgworker process
is or is not laboring on behalf of an identifiable user. It's great
that we can tell which user it is when there is one, but clearly some
bgworkers will be providing general services that aren't associated with
a single user. So it should be possible to set the userID to zero or
some such when the bgworker is one that isn't associated with a
particular user. Maybe the owning user needs to become an additional
parameter passed in struct BackgroundWorker.

I think this is probably a problem particular to the logical replication
launcher. Other background workers either do work as a particular user,
as you say, or don't touch the database at all. So a localized hack or
a simple hide-the-user flag might suffice for now.

But that still leaves the application_name issue. My proposal in general
would be to add boolean that indicates that the worker is not using
specific user (this can be easily set in
InitializeSessionUserIdStandalone()) and will work for multiple things.

About application_name, perhaps we should just add bgw_type or bgw_group
and show it as worker_type in activity and that's it?

I think this should be open item btw so I'll add it.

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

--
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, May 23, 2017 at 07:50:34PM +0200, Petr Jelinek wrote:

On 20/04/17 21:33, Peter Eisentraut wrote:

On 4/18/17 13:18, Tom Lane wrote:

I think you're thinking about it wrong. To my mind the issue is that
there should be some generic way to determine that a bgworker process
is or is not laboring on behalf of an identifiable user. It's great
that we can tell which user it is when there is one, but clearly some
bgworkers will be providing general services that aren't associated with
a single user. So it should be possible to set the userID to zero or
some such when the bgworker is one that isn't associated with a
particular user. Maybe the owning user needs to become an additional
parameter passed in struct BackgroundWorker.

I think this is probably a problem particular to the logical replication
launcher. Other background workers either do work as a particular user,
as you say, or don't touch the database at all. So a localized hack or
a simple hide-the-user flag might suffice for now.

But that still leaves the application_name issue. My proposal in general
would be to add boolean that indicates that the worker is not using
specific user (this can be easily set in
InitializeSessionUserIdStandalone()) and will work for multiple things.

About application_name, perhaps we should just add bgw_type or bgw_group
and show it as worker_type in activity and that's it?

I think this should be open item btw so I'll add it.

[Action required within three days. This is a generic notification.]

The above-described topic is currently a PostgreSQL 10 open item. Peter,
since you committed the patch believed to have created it, you own this open
item. If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know. Otherwise, please observe the policy on
open item ownership[1]/messages/by-id/20170404140717.GA2675809@tornado.leadboat.com and send a status update within three calendar days of
this message. Include a date for your subsequent status update. Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10. Consequently, I will appreciate your efforts
toward speedy resolution. Thanks.

[1]: /messages/by-id/20170404140717.GA2675809@tornado.leadboat.com

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

Here is a proposed solution that splits bgw_name into bgw_type and
bgw_name_extra. bgw_type shows up in pg_stat_activity.backend_type.
Uses of application_name are removed, because they are no longer
necessary to identity the process type.

This code appears to be buggy because I sometimes get NULL results of
the backend_type lookup, implying that it couldn't find the background
worker slot. This needs another look.

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

On Wed, May 31, 2017 at 12:10 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

Here is a proposed solution that splits bgw_name into bgw_type and
bgw_name_extra. bgw_type shows up in pg_stat_activity.backend_type.
Uses of application_name are removed, because they are no longer
necessary to identity the process type.

Hmm, is there any reasons why bgw_name_extra string doesn't appear in
pg_stat_activity? I'd say current patch makes the user difficult to
distinguish between apply worker and table sync worker.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

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

On 6/2/17 02:31, Masahiko Sawada wrote:

On Wed, May 31, 2017 at 12:10 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

Here is a proposed solution that splits bgw_name into bgw_type and
bgw_name_extra. bgw_type shows up in pg_stat_activity.backend_type.
Uses of application_name are removed, because they are no longer
necessary to identity the process type.

Hmm, is there any reasons why bgw_name_extra string doesn't appear in
pg_stat_activity?

That's the whole point: We want to be able to group similar process
types. The _extra part is particular to a single process, so it might
contain a specific OID or PID it is working on. The bgw_type is common
for all workers of that kind.

I'd say current patch makes the user difficult to
distinguish between apply worker and table sync worker.

We could arguably make apply workers and sync workers have different
bgw_type values. But if you are interested in that level of detail, you
should perhaps look at pg_stat_subscription. pg_stat_activity only
contains the "common" data, and the process-specific data is in other views.

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

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

On 5/30/17 23:10, Peter Eisentraut wrote:

Here is a proposed solution that splits bgw_name into bgw_type and
bgw_name_extra. bgw_type shows up in pg_stat_activity.backend_type.
Uses of application_name are removed, because they are no longer
necessary to identity the process type.

This code appears to be buggy because I sometimes get NULL results of
the backend_type lookup, implying that it couldn't find the background
worker slot. This needs another look.

I would like some more input on this proposal, especially from those
have have engineered the extended pg_stat_activity content.

If we don't come to a quick conclusion on this, I'd be content to leave
PG10 as is and put this patch into the next commit fest.

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

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

On 02/06/17 21:05, Peter Eisentraut wrote:

On 6/2/17 02:31, Masahiko Sawada wrote:

I'd say current patch makes the user difficult to
distinguish between apply worker and table sync worker.

We could arguably make apply workers and sync workers have different
bgw_type values. But if you are interested in that level of detail, you
should perhaps look at pg_stat_subscription. pg_stat_activity only
contains the "common" data, and the process-specific data is in other views.

Agreed with this.

However, I am not sure about the bgw_name_extra. I think I would have
preferred keeping full bgw_name field which would be used where full
name is needed and bgw_type where only the worker type is used. The
concatenation just doesn't sit well with me, especially if it requires
the bgw_name_extra to start with space.

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

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

On 6/2/17 16:44, Petr Jelinek wrote:

However, I am not sure about the bgw_name_extra. I think I would have
preferred keeping full bgw_name field which would be used where full
name is needed and bgw_type where only the worker type is used. The
concatenation just doesn't sit well with me, especially if it requires
the bgw_name_extra to start with space.

I see your point. There are also some i18n considerations to think through.

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

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

On 6/2/17 15:08, Peter Eisentraut wrote:

On 5/30/17 23:10, Peter Eisentraut wrote:

Here is a proposed solution that splits bgw_name into bgw_type and
bgw_name_extra. bgw_type shows up in pg_stat_activity.backend_type.
Uses of application_name are removed, because they are no longer
necessary to identity the process type.

This code appears to be buggy because I sometimes get NULL results of
the backend_type lookup, implying that it couldn't find the background
worker slot. This needs another look.

I would like some more input on this proposal, especially from those
have have engineered the extended pg_stat_activity content.

If we don't come to a quick conclusion on this, I'd be content to leave
PG10 as is and put this patch into the next commit fest.

If there are no new insights into this by Monday, I will commit patches
that remove the setting of application_name, which was originally
complained about, and postpone the rest of this patch.

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

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

On Sat, Jun 3, 2017 at 8:53 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

On 6/2/17 15:08, Peter Eisentraut wrote:

On 5/30/17 23:10, Peter Eisentraut wrote:

Here is a proposed solution that splits bgw_name into bgw_type and
bgw_name_extra. bgw_type shows up in pg_stat_activity.backend_type.
Uses of application_name are removed, because they are no longer
necessary to identity the process type.

This code appears to be buggy because I sometimes get NULL results of
the backend_type lookup, implying that it couldn't find the background
worker slot. This needs another look.

AFAICU, when we register a background worker using
RegisterDynamicBackgroundWorker, it's not included in
BackgroundWorkerList(which is postmaster's list of registered
background workers, in private memory). Instead, we use only
BackgroundWorkerSlots. Perhaps, this is the reason that backend_type
is NULL for parallel workers.

--
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 Sat, Jun 3, 2017 at 2:14 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:

On 02/06/17 21:05, Peter Eisentraut wrote:

On 6/2/17 02:31, Masahiko Sawada wrote:

I'd say current patch makes the user difficult to
distinguish between apply worker and table sync worker.

We could arguably make apply workers and sync workers have different
bgw_type values. But if you are interested in that level of detail, you
should perhaps look at pg_stat_subscription. pg_stat_activity only
contains the "common" data, and the process-specific data is in other views.

Agreed with this.

However, I am not sure about the bgw_name_extra. I think I would have
preferred keeping full bgw_name field which would be used where full
name is needed and bgw_type where only the worker type is used. The
concatenation just doesn't sit well with me, especially if it requires
the bgw_name_extra to start with space.

+1.

--
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 Sat, Jun 3, 2017 at 4:33 PM, Kuntal Ghosh <kuntalghosh.2007@gmail.com> wrote:

On Sat, Jun 3, 2017 at 2:14 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:

However, I am not sure about the bgw_name_extra. I think I would have
preferred keeping full bgw_name field which would be used where full
name is needed and bgw_type where only the worker type is used.

Yes, I don't thnk as well that this has any types of gain. With only
bgw_name, it is still possible to append the same prefix to all the
bgworkers of the same type, and do a search on pg_stat_activity using
'~' for example to fetch all the workers with the same string.

The concatenation just doesn't sit well with me, especially if it requires
the bgw_name_extra to start with space.

+1.

That's not friendly.
--
Michael

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

On 03/06/17 05:18, Peter Eisentraut wrote:

On 6/2/17 16:44, Petr Jelinek wrote:

However, I am not sure about the bgw_name_extra. I think I would have
preferred keeping full bgw_name field which would be used where full
name is needed and bgw_type where only the worker type is used. The
concatenation just doesn't sit well with me, especially if it requires
the bgw_name_extra to start with space.

I see your point. There are also some i18n considerations to think through.

So thinking a bit more, I wonder if we could simply do following:
- remove the application_name from logical workers
- add bgw_type and use it for worker type (if empty, use 'bgworker' like
now), would be probably nice if parallel workers added something to
indicate they are parallel workers there as well
- remove the 'bgworker:' prefix for ps display and just use the bgw_name

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

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

On 6/2/17 23:23, Peter Eisentraut wrote:

On 6/2/17 15:08, Peter Eisentraut wrote:

On 5/30/17 23:10, Peter Eisentraut wrote:

Here is a proposed solution that splits bgw_name into bgw_type and
bgw_name_extra. bgw_type shows up in pg_stat_activity.backend_type.
Uses of application_name are removed, because they are no longer
necessary to identity the process type.

This code appears to be buggy because I sometimes get NULL results of
the backend_type lookup, implying that it couldn't find the background
worker slot. This needs another look.

I would like some more input on this proposal, especially from those
have have engineered the extended pg_stat_activity content.

If we don't come to a quick conclusion on this, I'd be content to leave
PG10 as is and put this patch into the next commit fest.

If there are no new insights into this by Monday, I will commit patches
that remove the setting of application_name, which was originally
complained about, and postpone the rest of this patch.

Done, and added the rest of the patch to the next commit fest:
https://commitfest.postgresql.org/14/1165/

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

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

On Sat, Jun 3, 2017 at 3:33 AM, Kuntal Ghosh <kuntalghosh.2007@gmail.com> wrote:

Agreed with this.

However, I am not sure about the bgw_name_extra. I think I would have
preferred keeping full bgw_name field which would be used where full
name is needed and bgw_type where only the worker type is used. The
concatenation just doesn't sit well with me, especially if it requires
the bgw_name_extra to start with space.

+1.

+1 from me, too.

The problem with the status quo (after Peter's commit) is that there's
now nothing at all to identify the logical replication launcher, apart
from the wait_event field, which is likely to be LogicalLauncherMain
fairly often if you've got the launcher. I don't personally see why
we can't simply adopt Tom's original proposal of setting the query
string to something like "<logical replication launcher>" which, while
maybe not as elegant as providing some way to override the
backend_type field, would be almost no work and substantially better
for v10 than what we've got now.

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

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

On 6/6/17 15:58, Robert Haas wrote:

The problem with the status quo (after Peter's commit) is that there's
now nothing at all to identify the logical replication launcher, apart
from the wait_event field, which is likely to be LogicalLauncherMain
fairly often if you've got the launcher. I don't personally see why
we can't simply adopt Tom's original proposal of setting the query
string to something like "<logical replication launcher>" which, while
maybe not as elegant as providing some way to override the
backend_type field, would be almost no work and substantially better
for v10 than what we've got now.

The decision was made to add background workers to pg_stat_activity, but
no facility was provided to tell the background workers apart. Is it
now the job of every background worker to invent a hack to populate some
other pg_stat_activity field with some ad hoc information? What about
other logical replication worker types, parallel workers, external
background workers, auto-prewarm?

I think the bgw_type addition that I proposed nearby would solve this
quite well, but it needs a bit of work. And arguably, it's too late for
PG10, but one could also argue that this is a design fault in the
pg_stat_activity extension that is valid to fix in PG10.

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

--
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, Jun 7, 2017 at 4:58 AM, Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> wrote:

On 6/6/17 15:58, Robert Haas wrote:

The problem with the status quo (after Peter's commit) is that there's
now nothing at all to identify the logical replication launcher, apart
from the wait_event field, which is likely to be LogicalLauncherMain
fairly often if you've got the launcher. I don't personally see why
we can't simply adopt Tom's original proposal of setting the query
string to something like "<logical replication launcher>" which, while
maybe not as elegant as providing some way to override the
backend_type field, would be almost no work and substantially better
for v10 than what we've got now.

The decision was made to add background workers to pg_stat_activity, but
no facility was provided to tell the background workers apart. Is it
now the job of every background worker to invent a hack to populate some
other pg_stat_activity field with some ad hoc information? What about
other logical replication worker types, parallel workers, external
background workers, auto-prewarm?

I think the bgw_type addition that I proposed nearby would solve this
quite well, but it needs a bit of work. And arguably, it's too late for
PG10, but one could also argue that this is a design fault in the
pg_stat_activity extension that is valid to fix in PG10.

+1. I definitely think it would be a bad idea to put in what basically
looks like a workaround into 10, since the new feature was added there. I'd
rather have the fix for pg_stat_activity.

We used to keep our query state as a text field and that was a bad idea for
many reasons. So we moved it to a separate field. Let's not repeat that
mistake here.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

On Tue, Jun 6, 2017 at 10:58 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

The decision was made to add background workers to pg_stat_activity, but
no facility was provided to tell the background workers apart. Is it
now the job of every background worker to invent a hack to populate some
other pg_stat_activity field with some ad hoc information? What about
other logical replication worker types, parallel workers, external
background workers, auto-prewarm?

To be fair, some background workers were already shown in
pg_stat_activity; others were not. Parallel workers, for example,
always showed up in pg_stat_activity, but before v10 they didn't show
the query string, so you had to match them up with the process that
started them using the other fields. I think it's only workers not
connected to a database that weren't previously displayed. You might
be right that not enough that was given to how those could identify
themselves, but you would have had the problem anyway with logical
replication workers that connect to a database.

I think the bgw_type addition that I proposed nearby would solve this
quite well, but it needs a bit of work. And arguably, it's too late for
PG10, but one could also argue that this is a design fault in the
pg_stat_activity extension that is valid to fix in PG10.

I don't mind inventing a way for a background worker to display its
own text in place of the generic "background worker", but like others,
I didn't like splitting up the name field into two parts. I think you
could do the former without doing the latter.

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

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

On 5/30/17 23:10, Peter Eisentraut wrote:

Here is a proposed solution that splits bgw_name into bgw_type and
bgw_name_extra. bgw_type shows up in pg_stat_activity.backend_type.
Uses of application_name are removed, because they are no longer
necessary to identity the process type.

Updated patch incorporating the feedback. I have kept bgw_name as it
was and just added bgw_type completely independently.

One open question is how to treat a missing (empty) bgw_type. I
currently fill in bgw_name as a fallback. We could also treat it as an
error or a warning as a transition measure.

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

On Fri, Sep 1, 2017 at 4:49 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

On 5/30/17 23:10, Peter Eisentraut wrote:

Here is a proposed solution that splits bgw_name into bgw_type and
bgw_name_extra. bgw_type shows up in pg_stat_activity.backend_type.
Uses of application_name are removed, because they are no longer
necessary to identity the process type.

Updated patch incorporating the feedback. I have kept bgw_name as it
was and just added bgw_type completely independently.

-             errmsg("terminating background worker \"%s\" due to
administrator command",
-                    MyBgworkerEntry->bgw_name)));
+             errmsg("terminating %s due to administrator command",
+                    MyBgworkerEntry->bgw_type)));
"terminating background worker %s of type %s due to administrator
command", bgw_name, bgw_type?

One open question is how to treat a missing (empty) bgw_type. I
currently fill in bgw_name as a fallback. We could also treat it as an
error or a warning as a transition measure.

Hm. Why not reporting an empty type string as NULL at SQL level and
just let it empty them? I tend to like more interfaces that report
exactly what is exactly registered at memory-level, because that's
easier to explain to users and in the documentation, as well as easier
to interpret and easier for module developers.
--
Michael

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

On 8/31/17 23:22, Michael Paquier wrote:

One open question is how to treat a missing (empty) bgw_type. I
currently fill in bgw_name as a fallback. We could also treat it as an
error or a warning as a transition measure.

Hm. Why not reporting an empty type string as NULL at SQL level and
just let it empty them? I tend to like more interfaces that report
exactly what is exactly registered at memory-level, because that's
easier to explain to users and in the documentation, as well as easier
to interpret and easier for module developers.

But then background workers that are not updated for, say, PG11 will not
show anything useful in pg_stat_activity. We should have some amount of
backward compatibility here.

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

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

On 25/09/17 16:45, Peter Eisentraut wrote:

On 8/31/17 23:22, Michael Paquier wrote:

One open question is how to treat a missing (empty) bgw_type. I
currently fill in bgw_name as a fallback. We could also treat it as an
error or a warning as a transition measure.

Hm. Why not reporting an empty type string as NULL at SQL level and
just let it empty them? I tend to like more interfaces that report
exactly what is exactly registered at memory-level, because that's
easier to explain to users and in the documentation, as well as easier
to interpret and easier for module developers.

But then background workers that are not updated for, say, PG11 will not
show anything useful in pg_stat_activity. We should have some amount of
backward compatibility here.

Maybe the empty bgw_type could mean just "bgworker"?

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

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

On 31 Aug 2017, at 21:49, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:

On 5/30/17 23:10, Peter Eisentraut wrote:

Here is a proposed solution that splits bgw_name into bgw_type and
bgw_name_extra. bgw_type shows up in pg_stat_activity.backend_type.
Uses of application_name are removed, because they are no longer
necessary to identity the process type.

Updated patch incorporating the feedback. I have kept bgw_name as it
was and just added bgw_type completely independently.

The patch applies with minor fuzz, compiles without introduced warnings and
work the way it says on the tin. The utility of the proposed functionality is
a clear win so +1 on getting that in.

One open question is how to treat a missing (empty) bgw_type. I
currently fill in bgw_name as a fallback. We could also treat it as an
error or a warning as a transition measure.

Warnings tend to stick around forever in my experience. An error would work as
a transition measure, but it seems a hammer too far.

Falling back on bgw_name solves there being data with the risk that some
workers will have a wonky type displayed, and those are the ones that of course
will never get updated. Either going with NULL as Michael suggested elsewhere
in this thread (my preference as well) or a default string along the lines of
“Unknown type” would probably carry more incentive to update.

A few random comments on the patch:

Regarding the following hunk:

+   process listing, for example.  <structfield>bgw_name</structfield> on the
+   other hand can contain additional information about the specific process.
+   (Typically, the string for <structfield>bgw_name</structfield> will contain
+   the string for <structfield>bgw_type</structfield> somehow, but that is not
+   strictly required.)

This reads a bit too (oddly) detailed to me, I would’ve phrased it as something
along the lines of:

"Typically, the string for bgw_name will contain the type somehow, but that
is not strictly required.”

I find omitting “background worker” in the following hunk is leaving out
valuable information for bgworkers with badly configured types, but it’s
definitely a matter of taste rather than a straight-up patch critique:

-	 errmsg("terminating background worker \"%s\" due to administrator command",
-			MyBgworkerEntry->bgw_name)));
+	 errmsg("terminating %s due to administrator command",
+			MyBgworkerEntry->bgw_type)));

Updating the status to “Ready for committer” with the discussion around missing
bgw_type left as a decision point for a committer.

cheers ./daniel

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

On 8/31/17 23:22, Michael Paquier wrote:

On Fri, Sep 1, 2017 at 4:49 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

On 5/30/17 23:10, Peter Eisentraut wrote:

Here is a proposed solution that splits bgw_name into bgw_type and
bgw_name_extra. bgw_type shows up in pg_stat_activity.backend_type.
Uses of application_name are removed, because they are no longer
necessary to identity the process type.

Updated patch incorporating the feedback. I have kept bgw_name as it
was and just added bgw_type completely independently.

-             errmsg("terminating background worker \"%s\" due to
administrator command",
-                    MyBgworkerEntry->bgw_name)));
+             errmsg("terminating %s due to administrator command",
+                    MyBgworkerEntry->bgw_type)));
"terminating background worker %s of type %s due to administrator
command", bgw_name, bgw_type?

OK.

One open question is how to treat a missing (empty) bgw_type. I
currently fill in bgw_name as a fallback. We could also treat it as an
error or a warning as a transition measure.

Hm. Why not reporting an empty type string as NULL at SQL level and
just let it empty them? I tend to like more interfaces that report
exactly what is exactly registered at memory-level, because that's
easier to explain to users and in the documentation, as well as easier
to interpret and easier for module developers.

The problem here is that we refer to bgw_type in a bunch of places now,
and adding a suitable fallback in all of these places would be a lot of
code and it would create a regression in behavior. In practice, I think
that would be a lot of trouble for no gain.

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

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

On 9/27/17 18:59, Daniel Gustafsson wrote:

The patch applies with minor fuzz, compiles without introduced warnings and
work the way it says on the tin. The utility of the proposed functionality is
a clear win so +1 on getting that in.

I have committed this patch incorporating the feedback in this thread.

Regarding the following hunk:

+   process listing, for example.  <structfield>bgw_name</structfield> on the
+   other hand can contain additional information about the specific process.
+   (Typically, the string for <structfield>bgw_name</structfield> will contain
+   the string for <structfield>bgw_type</structfield> somehow, but that is not
+   strictly required.)

This reads a bit too (oddly) detailed to me, I would’ve phrased it as something
along the lines of:

"Typically, the string for bgw_name will contain the type somehow, but that
is not strictly required.”

Yes, that's better.

I find omitting “background worker” in the following hunk is leaving out
valuable information for bgworkers with badly configured types, but it’s
definitely a matter of taste rather than a straight-up patch critique:

-	 errmsg("terminating background worker \"%s\" due to administrator command",
-			MyBgworkerEntry->bgw_name)));
+	 errmsg("terminating %s due to administrator command",
+			MyBgworkerEntry->bgw_type)));

Also changed.

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

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