Thread: [HACKERS] Why does logical replication launcher set application_name?
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?
regards, tom lane
Re: [HACKERS] Why does logical replication launcher set application_name?
From
Michael Paquier
Date:
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
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
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
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
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
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_activitywhere 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 launcher25418 | 10 | postgres | logical replication launcher | 2017-04-16 12:32:46.988859-04 |
Activity | LogicalLauncherMain | background worker25414 | | | |
2017-04-1612:32:46.986745-04 | Activity | BgWriterHibernate | background writer25413 | | |
| 2017-04-16 12:32:46.986885-04 | Activity | CheckpointerMain | checkpointer25415 |
| | | 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
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_activitywhere 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
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_activitywhere 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
Re: [HACKERS] Why does logical replication launcher setapplication_name?
From
Peter Eisentraut
Date:
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
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
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
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
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
Re: [HACKERS] Why does logical replication launcher setapplication_name?
From
Peter Eisentraut
Date:
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
Re: [HACKERS] Why does logical replication launcher setapplication_name?
From
Peter Eisentraut
Date:
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
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
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] 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] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
Re: [HACKERS] Why does logical replication launcher setapplication_name?
From
Peter Eisentraut
Date:
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
Re: [HACKERS] Why does logical replication launcher set application_name?
From
Masahiko Sawada
Date:
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
Re: [HACKERS] Why does logical replication launcher setapplication_name?
From
Peter Eisentraut
Date:
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
Re: [HACKERS] Why does logical replication launcher setapplication_name?
From
Peter Eisentraut
Date:
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
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
Re: [HACKERS] Why does logical replication launcher setapplication_name?
From
Peter Eisentraut
Date:
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
Re: [HACKERS] Why does logical replication launcher setapplication_name?
From
Peter Eisentraut
Date:
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
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
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
Re: [HACKERS] Why does logical replication launcher set application_name?
From
Michael Paquier
Date:
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
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
Re: [HACKERS] Why does logical replication launcher setapplication_name?
From
Peter Eisentraut
Date:
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
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
Re: [HACKERS] Why does logical replication launcher setapplication_name?
From
Peter Eisentraut
Date:
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
Re: [HACKERS] Why does logical replication launcher set application_name?
From
Magnus Hagander
Date:
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.
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
bgw_type (was Re: [HACKERS] Why does logical replication launcher setapplication_name?)
From
Peter Eisentraut
Date:
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
Re: bgw_type (was Re: [HACKERS] Why does logical replication launcherset application_name?)
From
Michael Paquier
Date:
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
Re: bgw_type (was Re: [HACKERS] Why does logical replication launcherset application_name?)
From
Peter Eisentraut
Date:
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
Re: bgw_type (was Re: [HACKERS] Why does logical replication launcherset application_name?)
From
Petr Jelinek
Date:
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
Re: bgw_type (was Re: [HACKERS] Why does logical replication launcher set application_name?)
From
Daniel Gustafsson
Date:
> 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
Re: bgw_type (was Re: [HACKERS] Why does logical replication launcherset application_name?)
From
Peter Eisentraut
Date:
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
Re: bgw_type (was Re: [HACKERS] Why does logical replication launcherset application_name?)
From
Peter Eisentraut
Date:
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