Re: bgw_type (was Re: [HACKERS] Why does logical replication launcher set application_name?)

[Peter Eisentraut]

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.  bgw_name on the
> +   other hand can contain additional information about the specific process.
> +   (Typically, the string for bgw_name will 
> contain
> +   the string for bgw_type 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

Re: bgw_type (was Re: [HACKERS] Why does logical replication launcher set application_name?)

[Peter Eisentraut]

On 8/31/17 23:22, Michael Paquier wrote:
> On Fri, Sep 1, 2017 at 4:49 AM, 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.
>>
>> 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 launcher set application_name?)

[Daniel Gustafsson]

> On 31 Aug 2017, at 21:49, 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.
> 
> 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.  bgw_name on the
+   other hand can contain additional information about the specific process.
+   (Typically, the string for bgw_name will contain
+   the string for bgw_type 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 launcher set application_name?)

[Petr Jelinek]

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?)

[Peter Eisentraut]

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 launcher set application_name?)

[Michael Paquier]

On Fri, Sep 1, 2017 at 4:49 AM, 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.
>
> 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

bgw_type (was Re: [HACKERS] Why does logical replication launcher set application_name?)

[Peter Eisentraut]

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
From 73142adf6e56e44d97bd9f855072cba17ef5ea4c Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Thu, 31 Aug 2017 12:24:47 -0400
Subject: [PATCH v2] Add background worker type

Add bgw_type field to background worker structure.  It is intended to be
set to the same value for all workers of the same type, so they can be
grouped in pg_stat_activity, for example.

The backend_type column in pg_stat_activity now shows bgw_type for a
background worker.  The ps listing and some log messages no longer call
out that a process is a "background worker" but just show the bgw_type.
That way, being a background worker is effectively an implementation
detail now that is not shown to the user.
---
 contrib/pg_prewarm/autoprewarm.c   |  6 ++--
 doc/src/sgml/bgworker.sgml | 12 +--
 src/backend/access/transam/parallel.c  |  1 +
 src/backend/postmaster/bgworker.c  | 53 +++---
 src/backend/postmaster/postmaster.c| 10 ++
 src/backend/replication/logical/launcher.c |  3 ++
 src/backend/utils/adt/pgstatfuncs.c| 16 +++--
 src/include/postmaster/bgworker.h  |  2 ++
 src/test/modules/test_shm_mq/setup.c   |  2 +-
 src/test/modules/worker_spi/worker_spi.c   |  8 +++--
 10 files changed, 91 insertions(+), 22 deletions(-)

diff --git a/contrib/pg_prewarm/autoprewarm.c b/contrib/pg_prewarm/autoprewarm.c
index cc0350e6d6..006c3153db 100644
--- a/contrib/pg_prewarm/autoprewarm.c
+++ b/contrib/pg_prewarm/autoprewarm.c
@@ -800,7 +800,8 @@ apw_start_master_worker(void)
worker.bgw_start_time = BgWorkerStart_ConsistentState;
strcpy(worker.bgw_library_name, "pg_prewarm");
strcpy(worker.bgw_function_name, "autoprewarm_main");
-   strcpy(worker.bgw_name, "autoprewarm");
+   strcpy(worker.bgw_name, "autoprewarm master");
+   strcpy(worker.bgw_type, "autoprewarm master");
 
if (process_shared_preload_libraries_in_progress)
{
@@ -840,7 +841,8 @@ apw_start_database_worker(void)
worker.bgw_start_time = BgWorkerStart_ConsistentState;
strcpy(worker.bgw_library_name, "pg_prewarm");
strcpy(worker.bgw_function_name, "autoprewarm_database_main");
-   strcpy(worker.bgw_name, "autoprewarm");
+   strcpy(worker.bgw_name, "autoprewarm worker");
+   strcpy(worker.bgw_type, "autoprewarm worker");
 
/* must set notify PID to wait for shutdown */
worker.bgw_notify_pid = MyProcPid;
diff --git a/doc/src/sgml/bgworker.sgml b/doc/src/sgml/bgworker.sgml
index b422323081..822632bf02 100644
--- a/doc/src/sgml/bgworker.sgml
+++ b/doc/src/sgml/bgworker.sgml
@@ -51,6 +51,7 @@ Background Worker Processes
 typedef struct BackgroundWorker
 {
 charbgw_name[BGW_MAXLEN];
+charbgw_type[BGW_MAXLEN];
 int bgw_flags;
 BgWorkerStartTime bgw_start_time;
 int bgw_restart_time;   /* in seconds, or BGW_NEVER_RESTART */
@@ -64,8 +65,15 @@ Background Worker Processes
   
 
   
-   bgw_name is a string to be used in log messages, process
-   listings and similar contexts.
+   bgw_name and bgw_type are
+   strings to be used in log messages, process listings and similar contexts.
+   bgw_type should be the same for all background
+   workers of the same type, so that it is possible to group such workers in a
+   process listing, for example.  bgw_name on the
+   other hand can contain additional information about the specific process.
+   (Typically, the string for bgw_name will contain
+   the string for bgw_type somehow, but that is not
+   strictly required.)
   
 
   
diff --git a/src/backend/access/transam/parallel.c 
b/src/backend/access/transam/parallel.c
index 17b10383e4..9828259e6d 100644
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -438,6 +438,7 @@ LaunchParallelWorkers(ParallelContext *pcxt)
memset(, 0, sizeof(worker));
snprintf(worker.bgw_name, BGW_MAXLEN, "parallel worker for PID %d",
 MyProcPid);
+   snprintf(worker.bgw_type, BGW_MAXLEN, "parallel worker");
worker.bgw_flags =
BGWORKER_SHMEM_ACCESS | BGWORKER_BACKEND_DATABASE_CONNECTION
|