Re: Cluster name in ps output

Hi

When running more than one cluster I often find myself looking at
the output of 'iotop' or other tools wondering which
cluster's "wal receiver process" or "checkpointer process" etc
I'm seeing. Obviously it's easy enough to find out (for example
by looking at a tree view in htop/ps that shows the -D parameter
of the parent postmaster), but I thought it could be useful to be
able to give a name to the cluster and include it in the ps
output. Here is a strawman patch that does that.

If cluster_name is not set, it defaults to the empty string and
the ps output is unchanged. If it's set to 'foox' the ps output
includes that string in square brackets:

postgres: [foox] checkpointer process
postgres: [foox] writer process
postgres: [foox] wal writer process
postgres: [foox] autovacuum launcher process
postgres: [foox] stats collector process
postgres: [foox] munro foodb [local] idle

I would be grateful for any feedback. Thanks,

Thomas

Hi,

On 2014-05-05 10:00:34 +0000, Thomas Munro wrote:
> When running more than one cluster I often find myself looking at
> the output of 'iotop' or other tools wondering which
> cluster's "wal receiver process" or "checkpointer process" etc
> I'm seeing.

I wonder about that pretty regularly. To the point that I've a hacky
version of this locally. So +1 for me for the idea in general.

> If cluster_name is not set, it defaults to the empty string and
> the ps output is unchanged. If it's set to 'foox' the ps output
> includes that string in square brackets:
>
> postgres: [foox] checkpointer process
> postgres: [foox] writer process
> postgres: [foox] wal writer process
> postgres: [foox] autovacuum launcher process
> postgres: [foox] stats collector process
> postgres: [foox] munro foodb [local] idle

"postgres: [foox] ..." should rather be "postgres[foox]: ..." imo ;)

I guess the question is where this should be available as well. At the
very least I'd want to reference it in log_line_prefix as well?

> diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
> index 15020c4..7f7fd52 100644
> --- a/src/backend/utils/misc/guc.c
> +++ b/src/backend/utils/misc/guc.c
> @@ -449,6 +449,7 @@ int temp_file_limit = -1;
>
> int num_temp_buffers = 1024;
>
> +char *cluster_name;
> char *data_directory;
> char *ConfigFileName;
> char *HbaFileName;
> @@ -3091,6 +3092,17 @@ static struct config_string ConfigureNamesString[] =
> },
>
> {
> + {"cluster_name", PGC_POSTMASTER, CUSTOM_OPTIONS,
> + gettext_noop("Sets the name of the cluster that appears in 'ps' output."),
> + NULL,
> + GUC_IS_NAME
> + },
> + &cluster_name,
> + "",
> + NULL, NULL, NULL
> + },
> +
> + {
> {"data_directory", PGC_POSTMASTER, FILE_LOCATIONS,
> gettext_noop("Sets the server's data directory."),
> NULL,
> diff --git a/src/backend/utils/misc/ps_status.c b/src/backend/utils/misc/ps_status.c
> index 6294ca3..ead7ea4 100644
> --- a/src/backend/utils/misc/ps_status.c
> +++ b/src/backend/utils/misc/ps_status.c
> @@ -29,6 +29,7 @@
> #include "libpq/libpq.h"
> #include "miscadmin.h"
> #include "utils/ps_status.h"
> +#include "utils/guc.h"
>
> extern char **environ;
> bool update_process_title = true;
> @@ -264,15 +265,24 @@ init_ps_display(const char *username, const char *dbname,
> * apparently setproctitle() already adds a `progname:' prefix to the ps
> * line
> */
> - snprintf(ps_buffer, ps_buffer_size,
> - "%s %s %s ",
> - username, dbname, host_info);
> +#define PROGRAM_NAME_PREFIX ""
> #else
> - snprintf(ps_buffer, ps_buffer_size,
> - "postgres: %s %s %s ",
> - username, dbname, host_info);
> +#define PROGRAM_NAME_PREFIX "postgres: "
> #endif
>
> + if (*cluster_name == '\0')
> + {
> + snprintf(ps_buffer, ps_buffer_size,
> + PROGRAM_NAME_PREFIX "%s %s %s ",
> + username, dbname, host_info);
> + }
> + else
> + {
> + snprintf(ps_buffer, ps_buffer_size,
> + PROGRAM_NAME_PREFIX "[%s] %s %s %s ",
> + cluster_name, username, dbname, host_info);
> + }
> +
> ps_buffer_cur_len = ps_buffer_fixed_size = strlen(ps_buffer);

Aren't you potentially dereferencing a NULL pointer here?

Greetings,

Andres Freund

On 5 May 2014 10:10, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:

> Hi,
>
> On 2014-05-05 10:00:34 +0000, Thomas Munro wrote:
> > When running more than one cluster I often find myself looking at
> > the output of 'iotop' or other tools wondering which
> > cluster's "wal receiver process" or "checkpointer process" etc
> > I'm seeing.
>
> I wonder about that pretty regularly. To the point that I've a hacky
> version of this locally. So +1 for me for the idea in general.

Thanks! (Do you have a write up/diff somewhere showing the local
modifications you're running?)

> > If cluster_name is not set, it defaults to the empty string and
> > the ps output is unchanged. If it's set to 'foox' the ps output
> > includes that string in square brackets:
> >
> > postgres: [foox] checkpointer process
> > postgres: [foox] writer process
> > postgres: [foox] wal writer process
> > postgres: [foox] autovacuum launcher process
> > postgres: [foox] stats collector process
> > postgres: [foox] munro foodb [local] idle
>
> "postgres: [foox] ..." should rather be "postgres[foox]: ..." imo ;)
>
>
Hah -- I agree, but on systems using setproctitle, the program name and ":
" are provided already, so the end result would have to be different on
those systems and I figured it should be the same everywhere if possible.
(BTW I also tried to tidy up the way that is handled, so that instead of a
different snprintf statement being selected by the preprocessor, a macro
PROGRAM_NAME_PREFIX is defined to be empty on those systems).

> I guess the question is where this should be available as well. At the
> very least I'd want to reference it in log_line_prefix as well?
>

Good idea, I will look into that.

> > + if (*cluster_name == '\0')
> > + {
> > + snprintf(ps_buffer, ps_buffer_size,
> > + PROGRAM_NAME_PREFIX "%s %s %s ",
> > + username, dbname, host_info);
> > + }
> > + else
> > + {
> > + snprintf(ps_buffer, ps_buffer_size,
> > + PROGRAM_NAME_PREFIX "[%s] %s %s %s ",
> > + cluster_name, username, dbname,
> host_info);
> > + }
> > +
> > ps_buffer_cur_len = ps_buffer_fixed_size = strlen(ps_buffer);
>
> Aren't you potentially dereferencing a NULL pointer here?
>

Hmm -- I thought the GUC machinery would make sure cluster_name either
pointed to the default I provided, an empty string, or a string read from
the configuration file. Perhaps I need to go and read up on how GUCs work.

Thomas Munro

On 05/05/2014 06:00 PM, Thomas Munro wrote:
> Hi
>
> When running more than one cluster I often find myself looking at
> the output of 'iotop' or other tools wondering which
> cluster's "wal receiver process" or "checkpointer process" etc
> I'm seeing. Obviously it's easy enough to find out (for example
> by looking at a tree view in htop/ps that shows the -D parameter
> of the parent postmaster), but I thought it could be useful to be
> able to give a name to the cluster and include it in the ps
> output. Here is a strawman patch that does that.
>
> If cluster_name is not set, it defaults to the empty string and
> the ps output is unchanged. If it's set to 'foox' the ps output
> includes that string in square brackets:
>
> postgres: [foox] checkpointer process
> postgres: [foox] writer process
> postgres: [foox] wal writer process
> postgres: [foox] autovacuum launcher process
> postgres: [foox] stats collector process
> postgres: [foox] munro foodb [local] idle

I'd find something like that very useful.

I'd even be tempted to tack on the port number, though that might be
overkill.

Perhaps just use the port number instead of a new cluster name? Most
people won't use/set something like a cluster name, though I guess
distros that use pg_wrapper would probably auto-set it via pg_wrapper's
configuration.

Personally I'd find:

postgres[5433]: checkpointer process

at least as useful. The only time that's not unique is in a BSD jail or
lxc container, and in those cases IIRC ps can show you the
jail/container anyway.

Showing the port would help new-ish users a lot; many seem to be very
confused by which PostgreSQL instance(s) they're connecting to and which
are running. Especially on Mac OS X, where people often land up with
Apple's PostgreSQL, Homebrew, Postgres.app, and who knows what else
running at the same time.

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

On 2014-05-05 10:49:19 +0000, Thomas Munro wrote:
> On 5 May 2014 10:10, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>
> > Hi,
> >
> > On 2014-05-05 10:00:34 +0000, Thomas Munro wrote:
> > > When running more than one cluster I often find myself looking at
> > > the output of 'iotop' or other tools wondering which
> > > cluster's "wal receiver process" or "checkpointer process" etc
> > > I'm seeing.
> >
> > I wonder about that pretty regularly. To the point that I've a hacky
> > version of this locally. So +1 for me for the idea in general.
>
>
> Thanks! (Do you have a write up/diff somewhere showing the local
> modifications you're running?)

I've just hacked in the port into the ps display since that was
sufficient for my case.

> > > If cluster_name is not set, it defaults to the empty string and
> > > the ps output is unchanged. If it's set to 'foox' the ps output
> > > includes that string in square brackets:
> > >
> > > postgres: [foox] checkpointer process
> > > postgres: [foox] writer process
> > > postgres: [foox] wal writer process
> > > postgres: [foox] autovacuum launcher process
> > > postgres: [foox] stats collector process
> > > postgres: [foox] munro foodb [local] idle
> >
> > "postgres: [foox] ..." should rather be "postgres[foox]: ..." imo ;)
> >
> >
> Hah -- I agree, but on systems using setproctitle, the program name and ":
> " are provided already, so the end result would have to be different on
> those systems and I figured it should be the same everywhere if possible.

Fair point.

> > Aren't you potentially dereferencing a NULL pointer here?
> >
>
> Hmm -- I thought the GUC machinery would make sure cluster_name either
> pointed to the default I provided, an empty string, or a string read from
> the configuration file. Perhaps I need to go and read up on how GUCs work.

That's true - but I am not sure you can guarantee it's only called after
the GUC machinery has started up. Particularly on windows. I guess just
initializing the global variable to "" should do the trick.

Greetings,

Andres Freund

* Andres Freund (andres(at)2ndquadrant(dot)com) wrote:
> On 2014-05-05 10:00:34 +0000, Thomas Munro wrote:
> > When running more than one cluster I often find myself looking at
> > the output of 'iotop' or other tools wondering which
> > cluster's "wal receiver process" or "checkpointer process" etc
> > I'm seeing.
>
> I wonder about that pretty regularly. To the point that I've a hacky
> version of this locally. So +1 for me for the idea in general.

Ditto.

> > If cluster_name is not set, it defaults to the empty string and
> > the ps output is unchanged. If it's set to 'foox' the ps output
> > includes that string in square brackets:
> >
> > postgres: [foox] checkpointer process
> > postgres: [foox] writer process
> > postgres: [foox] wal writer process
> > postgres: [foox] autovacuum launcher process
> > postgres: [foox] stats collector process
> > postgres: [foox] munro foodb [local] idle
>
> "postgres: [foox] ..." should rather be "postgres[foox]: ..." imo ;)
>
> I guess the question is where this should be available as well. At the
> very least I'd want to reference it in log_line_prefix as well?

I'm not entirely sure that I see the point of having it in
log_line_prefix- each cluster logs to its own log file which includes
the cluster name (at least on Debian/Ubuntu and friends). The only use
case I can imagine here would be for syslog, but you could just *set*
the cluster name in the log_line_prefix, as it'd be (by definition..)
configurable per cluster.

I'd much rather see other things added as log_line_prefix options.. An
interesting thought that just occured to me would be to allow any GUC to
be added to log_line_prefix using some kind of extended % support (eg:
'%{my_guc_here}' or something...). Would also be useful for extensions
which add GUCs then? Not sure about specifics, but does seem like an
interesting idea.

Oh, and I know people will shoot me for bringing it up again, but I'd
still like to see the CSV format be configurable ala log_line_prefix,
and the same for any kind of logging (or auditing) to a table which we
might eventually support. Yes, we need to work out how to do file
changes when it's updated and stick a header on each new file with the
columns included.

Thanks,

Stephen

Craig,

* Craig Ringer (craig(at)2ndquadrant(dot)com) wrote:
> postgres[5433]: checkpointer process
>
> at least as useful. The only time that's not unique is in a BSD jail or
> lxc container, and in those cases IIRC ps can show you the
> jail/container anyway.

Uhh, that's not at all true. You can trivially have multiple IPs on a
box w/o jails or containers (aliased interfaces) and then run PG on the
default port- which I find to be *far* more convenient than having the
same IP and a bunch of different ports.

What you *can't* have is two clusters with the same name and same major
version, at least on the Debian/Ubuntu distributions, and as such, I
would argue to also include the major version rather than include the
port, which you could get from pg_lsclusters.

> Showing the port would help new-ish users a lot; many seem to be very
> confused by which PostgreSQL instance(s) they're connecting to and which
> are running. Especially on Mac OS X, where people often land up with
> Apple's PostgreSQL, Homebrew, Postgres.app, and who knows what else
> running at the same time.

I'm far from convinced that showing the port in the ps output will
really help these users.. Not to mention that you can get that from
'netstat -anp' anyway.

Thanks,

Stephen

On 2014-05-05 07:58:30 -0400, Stephen Frost wrote:
> > I guess the question is where this should be available as well. At the
> > very least I'd want to reference it in log_line_prefix as well?
>
> I'm not entirely sure that I see the point of having it in
> log_line_prefix- each cluster logs to its own log file which includes
> the cluster name (at least on Debian/Ubuntu and friends). The only use
> case I can imagine here would be for syslog, but you could just *set*
> the cluster name in the log_line_prefix, as it'd be (by definition..)
> configurable per cluster.

So I've to configure it in multiple locations? I don't see the point. I
usually try to configure as much in common/template config files that
are included. Everything that doesn't have to be overwritten is good.

Greetings,

Andres Freund

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

* Andres Freund (andres(at)2ndquadrant(dot)com) wrote:
> On 2014-05-05 07:58:30 -0400, Stephen Frost wrote:
> > > I guess the question is where this should be available as well. At the
> > > very least I'd want to reference it in log_line_prefix as well?
> >
> > I'm not entirely sure that I see the point of having it in
> > log_line_prefix- each cluster logs to its own log file which includes
> > the cluster name (at least on Debian/Ubuntu and friends). The only use
> > case I can imagine here would be for syslog, but you could just *set*
> > the cluster name in the log_line_prefix, as it'd be (by definition..)
> > configurable per cluster.
>
> So I've to configure it in multiple locations? I don't see the point. I
> usually try to configure as much in common/template config files that
> are included. Everything that doesn't have to be overwritten is good.

I see the point- we've already got quite a few %whatever's and adding
mostly useless ones may just add confusion and unnecessary complexity.
If you've already got your postgresql.conf templated through
puppet/chef/salt/whatever then having to add another '%{ CLUSTER_NAME }%'
or whatever shouldn't be a terribly difficult thing.

Thanks,

Stephen

On 5 May 2014 10:49, Thomas Munro <munro(at)ip9(dot)org> wrote:

> On 5 May 2014 10:10, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>
>> I guess the question is where this should be available as well. At the
>> very least I'd want to reference it in log_line_prefix as well?
>>
>
> Good idea, I will look into that.
>

See v2 patch attached which lets you use %C for cluster name in the log
prefix.

Maybe it would be overkill, but seeing the various responses about which
information belongs in the ps string, I guess we could also use a format
string with %blah fields for that. Then the Debian/Ubuntu package users
who tend to think of the major version + name as the complete cluster
identifier could use "[%V/%C] ..." to get "postgres: [9.3/main] ...", and
others could throw in a "%P" to see a port number.

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> On 2014-05-05 10:49:19 +0000, Thomas Munro wrote:
>> Hah -- I agree, but on systems using setproctitle, the program name and ":
>> " are provided already, so the end result would have to be different on
>> those systems and I figured it should be the same everywhere if possible.

> Fair point.

How about dropping the brackets, and the cluster-name concept, and
just doing

postgres: 5432 checkpointer process

>>> Aren't you potentially dereferencing a NULL pointer here?

>> Hmm -- I thought the GUC machinery would make sure cluster_name either
>> pointed to the default I provided, an empty string, or a string read from
>> the configuration file. Perhaps I need to go and read up on how GUCs work.

> That's true - but I am not sure you can guarantee it's only called after
> the GUC machinery has started up.

The elog code MUST be able to work before GUC initialization is done.
What do you think will happen if we fail to open postgresql.conf,
for example?

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> How about dropping the brackets, and the cluster-name concept, and
> just doing
>
> postgres: 5432 checkpointer process

-1 for my part, as I'd just end up with a bunch of those and no
distinction between the various processes. In other words, without a
cluster distinction, it's useless.

Including the value of listen_addresses along w/ the port would make it
useful. If we really don't want the cluster-name concept (which,
personally, I like quite a bit), how about including the listen_address
value if it isn't '*'? I could see that also helping users who
installed from a distro and got '127.0.0.1' and don't understand why
they can't connect...

Of course, these are users who can use 'ps' but not 'netstat'. Not sure
how big that set really is.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> How about dropping the brackets, and the cluster-name concept, and
>> just doing
>>
>> postgres: 5432 checkpointer process

> -1 for my part, as I'd just end up with a bunch of those and no
> distinction between the various processes. In other words, without a
> cluster distinction, it's useless.

Yeah, after I sent that I got to the bit about running multiple
postmasters with different IP-address bindings. I agree the port number
alone wouldn't be enough in that scenario.

> Including the value of listen_addresses along w/ the port would make it
> useful. If we really don't want the cluster-name concept (which,
> personally, I like quite a bit), how about including the listen_address
> value if it isn't '*'?

Nah, let's do cluster name. That way, somebody who's only got one
postmaster isn't suddenly going to see a lot of useless clutter,
ie the user gets to decide what to add to ps output. "SHOW cluster_name"
might be useful at the application level as well, I suspect.

I still think the brackets are unnecessary though.

Also, -1 for adding another log_line_prefix escape. If you're routing
multiple clusters logging to the same place (which is already a bit
unlikely IMO), you can put distinguishing strings in log_line_prefix
already. And it's not like we've got an infinite supply of letters
for those escapes.

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > Including the value of listen_addresses along w/ the port would make it
> > useful. If we really don't want the cluster-name concept (which,
> > personally, I like quite a bit), how about including the listen_address
> > value if it isn't '*'?
>
> Nah, let's do cluster name. That way, somebody who's only got one
> postmaster isn't suddenly going to see a lot of useless clutter,
> ie the user gets to decide what to add to ps output. "SHOW cluster_name"
> might be useful at the application level as well, I suspect.

Ah, yes, agreed, that could be quite useful.

> I still think the brackets are unnecessary though.

Either way is fine for me on this.

> Also, -1 for adding another log_line_prefix escape. If you're routing
> multiple clusters logging to the same place (which is already a bit
> unlikely IMO), you can put distinguishing strings in log_line_prefix
> already. And it's not like we've got an infinite supply of letters
> for those escapes.

Agreed.

Thanks,

Stephen

On 2014-05-05 09:52:33 -0400, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> >>> Aren't you potentially dereferencing a NULL pointer here?
>
> >> Hmm -- I thought the GUC machinery would make sure cluster_name either
> >> pointed to the default I provided, an empty string, or a string read from
> >> the configuration file. Perhaps I need to go and read up on how GUCs work.
>
> > That's true - but I am not sure you can guarantee it's only called after
> > the GUC machinery has started up.
>
> The elog code MUST be able to work before GUC initialization is done.
> What do you think will happen if we fail to open postgresql.conf,
> for example?

But elog. shouldn't call init_ps_display(), right? Anyway, I am all for
initializing it sensibly, after all it was I that suggested doing so above...

Greetings,

Andres Freund

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

On 2014-05-05 08:03:04 -0400, Stephen Frost wrote:
> Craig,
>
> * Craig Ringer (craig(at)2ndquadrant(dot)com) wrote:
> > postgres[5433]: checkpointer process
> >
> > at least as useful. The only time that's not unique is in a BSD jail or
> > lxc container, and in those cases IIRC ps can show you the
> > jail/container anyway.
>
> Uhh, that's not at all true. You can trivially have multiple IPs on a
> box w/o jails or containers (aliased interfaces) and then run PG on the
> default port- which I find to be *far* more convenient than having the
> same IP and a bunch of different ports.

Only that you then need different socket directories. Do you really do
that regularly?

Anyway, I am happy having the cluster_name thingy.

Greetings,

Andres Freund

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

On 2014-05-05 10:07:46 -0400, Tom Lane wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > Including the value of listen_addresses along w/ the port would make it
> > useful. If we really don't want the cluster-name concept (which,
> > personally, I like quite a bit), how about including the listen_address
> > value if it isn't '*'?
>
> Nah, let's do cluster name. That way, somebody who's only got one
> postmaster isn't suddenly going to see a lot of useless clutter,
> ie the user gets to decide what to add to ps output. "SHOW cluster_name"
> might be useful at the application level as well, I suspect.

Hm. What about unifiyng this with event_source? Not sure if it's a good
idea, but it's a pretty similar thing.

> Also, -1 for adding another log_line_prefix escape. If you're routing
> multiple clusters logging to the same place (which is already a bit
> unlikely IMO), you can put distinguishing strings in log_line_prefix
> already. And it's not like we've got an infinite supply of letters
> for those escapes.

Using syslog and including the same config file from multiple clusters
isn't that uncommon. But I can live without it.

Greetings,

Andres Freund

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

* Andres Freund (andres(at)2ndquadrant(dot)com) wrote:
> On 2014-05-05 08:03:04 -0400, Stephen Frost wrote:
> > Uhh, that's not at all true. You can trivially have multiple IPs on a
> > box w/o jails or containers (aliased interfaces) and then run PG on the
> > default port- which I find to be *far* more convenient than having the
> > same IP and a bunch of different ports.
>
> Only that you then need different socket directories. Do you really do
> that regularly?

Yup. I've wished for that to be the default in Debian quite a few
times, actually... Much easier to deal with firewall rules and users
who are coming from pgAdmin and friends if they only have to deal with
understanding what hosts they need to connect to, and not worry about
the port also.

> Anyway, I am happy having the cluster_name thingy.

Agreed.

Thanks,

Stpehen

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> On 2014-05-05 10:07:46 -0400, Tom Lane wrote:
>> Also, -1 for adding another log_line_prefix escape. If you're routing
>> multiple clusters logging to the same place (which is already a bit
>> unlikely IMO), you can put distinguishing strings in log_line_prefix
>> already. And it's not like we've got an infinite supply of letters
>> for those escapes.

> Using syslog and including the same config file from multiple clusters
> isn't that uncommon. But I can live without it.

So, if you are sharing a config file, how is it that you can set a
per-cluster cluster_name but not a per-cluster log_line_prefix?

regards, tom lane

On 2014-05-05 13:07:48 -0400, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > On 2014-05-05 10:07:46 -0400, Tom Lane wrote:
> >> Also, -1 for adding another log_line_prefix escape. If you're routing
> >> multiple clusters logging to the same place (which is already a bit
> >> unlikely IMO), you can put distinguishing strings in log_line_prefix
> >> already. And it's not like we've got an infinite supply of letters
> >> for those escapes.
>
> > Using syslog and including the same config file from multiple clusters
> > isn't that uncommon. But I can live without it.
>
> So, if you are sharing a config file, how is it that you can set a
> per-cluster cluster_name but not a per-cluster log_line_prefix?

Well, it's a included file. With log_line_prefix support just
cluster_name has to be set per cluster. Without you need string
interpolation in the config management to include cluster_name in
log_line_prefix.

Greetings,

Andres Freund

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

On 5 May 2014 17:22, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:

> On 2014-05-05 13:07:48 -0400, Tom Lane wrote:
> > Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > > On 2014-05-05 10:07:46 -0400, Tom Lane wrote:
> > >> Also, -1 for adding another log_line_prefix escape. If you're routing
> > >> multiple clusters logging to the same place (which is already a bit
> > >> unlikely IMO), you can put distinguishing strings in log_line_prefix
> > >> already. And it's not like we've got an infinite supply of letters
> > >> for those escapes.
> >
> > > Using syslog and including the same config file from multiple clusters
> > > isn't that uncommon. But I can live without it.
> >
> > So, if you are sharing a config file, how is it that you can set a
> > per-cluster cluster_name but not a per-cluster log_line_prefix?
>
> Well, it's a included file. With log_line_prefix support just
> cluster_name has to be set per cluster. Without you need string
> interpolation in the config management to include cluster_name in
> log_line_prefix.
>

Attached as cluster-name-in-ps-v3-a.patch is a new version with
the following changes:

* the brackets removed, as suggested by Tom Lane

* static initialization of cluster_name to avoid any possibility
of an uninitialized/null pointer being used before GUC
initialization, as suggested by Andres Freund

* cluster_name moved to config group CONN_AUTH_SETTINGS, on the
basis that it's similar to bonjour_name, but it isn't
really... open to suggestions for a better config_group!

* a small amount of documentation in config.sgml (with
cluster_name under Connection Settings, which probably
isn't right either)

* sample conf file updated to show cluster_name and %C in
log_line_prefix

A shorter version without the log_line_prefix support that Tom didn't
like is attached as cluster-name-in-ps-v3-b.patch. I will try to add these
to the open commitfest, and see if there is something I can usefully
review in return.

I verified that SHOW cluster_name works as expected and you can't
change it with SET.

Thanks,

Thomas Munro

Hi.

I reviewed the version of this patch without log_line_prefix support,
since that seemed to be generally acceptable in followup discussion.

The patch didn't apply any more because of some changes to guc.c, but it
was trivial to regenerate (fixed patch attached).

> diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
> index 70e5a51..84ae5f3 100644
> --- a/src/backend/utils/misc/postgresql.conf.sample
> +++ b/src/backend/utils/misc/postgresql.conf.sample
> @@ -74,6 +74,8 @@
> # (change requires restart)
> #bonjour_name = '' # defaults to the computer name
> # (change requires restart)
> +#cluster_name = '' # defaults to the computer name
> + # (change requires restart)

Cut-and-paste error (there's no default). Also fixed in the attached
patch.

The patch looks OK, and works as advertised (I tested on Linux). If we
want the feature (I like it), this patch is a good enough way to get it.

I'm marking it ready for committer.

-- Abhijit

>> * cluster_name moved to config group CONN_AUTH_SETTINGS, on the
>> basis that it's similar to bonjour_name, but it isn't
>> really... open to suggestions for a better config_group!

Categorizing this parameter to CONN_AUTH_SETTINGS looks strange to me
because it's not directly related to connection and authentication.
LOGGING_WHAT is better if we allow log_line_prefix to include the
cluster_name. Or STATS_COLLECTOR which update_process_title is
categorized to? STATS_COLLECTOR also looks a bit strange not only for
cluster_name but also update_process_title, though...

On Wed, Jun 25, 2014 at 1:29 PM, Abhijit Menon-Sen <ams(at)2ndquadrant(dot)com> wrote:
> The patch looks OK, and works as advertised (I tested on Linux). If we
> want the feature (I like it), this patch is a good enough way to get it.

I got the following compiler warning.

guc.c:3099: warning: initialization from incompatible pointer type

monitoring.sgml explains PS display. Isn't it better to update monitoring.sgml
so that it refers to cluster_name?

Regards,

--
Fujii Masao

At 2014-06-25 16:13:19 +0900, masao(dot)fujii(at)gmail(dot)com wrote:
>
> Categorizing this parameter to CONN_AUTH_SETTINGS looks strange to me

Oh yes. Sorry, I meant to respond to this point in my original review,
but forgot. Yes, CONN_AUTH_SETTINGS is just weird. But I couldn't find
an obviously better answer either.

LOGGING_WHAT would work with log_line_prefix support, but I don't think
there was as much support for that version of this patch. I personally
don't have a strong opinion about whether it's worth adding an escape.

> STATS_COLLECTOR also looks a bit strange not only for cluster_name but
> also update_process_title, though...

True. Is UNGROUPED the only answer?

> monitoring.sgml explains PS display. Isn't it better to update
> monitoring.sgml so that it refers to cluster_name?

Good point.

-- Abhijit

On Thu, Jun 26, 2014 at 4:05 PM, Abhijit Menon-Sen <ams(at)2ndquadrant(dot)com> wrote:
> At 2014-06-25 16:13:19 +0900, masao(dot)fujii(at)gmail(dot)com wrote:
>>
>> Categorizing this parameter to CONN_AUTH_SETTINGS looks strange to me
>
> Oh yes. Sorry, I meant to respond to this point in my original review,
> but forgot. Yes, CONN_AUTH_SETTINGS is just weird. But I couldn't find
> an obviously better answer either.
>
> LOGGING_WHAT would work with log_line_prefix support, but I don't think
> there was as much support for that version of this patch. I personally
> don't have a strong opinion about whether it's worth adding an escape.
>
>> STATS_COLLECTOR also looks a bit strange not only for cluster_name but
>> also update_process_title, though...
>
> True. Is UNGROUPED the only answer?

Or create new group like REPORTING_WHAT?

Regards,

--
Fujii Masao

On 25 June 2014 08:13, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Wed, Jun 25, 2014 at 1:29 PM, Abhijit Menon-Sen <ams(at)2ndquadrant(dot)com> wrote:
>> The patch looks OK, and works as advertised (I tested on Linux). If we
>> want the feature (I like it), this patch is a good enough way to get it.
>
> I got the following compiler warning.
>
> guc.c:3099: warning: initialization from incompatible pointer type

Thank you both for your reviews. Please find attached an updated v5
patch (based on Abhijit's rebased and improved version) which fixes
the compiler warning. (The previous unusual and possibly incorrect
static initialization with a string literal was intended to make sure
logging would work before the GUC machinery has finished setting up
default values, but that no longer applies.)

Regards,
Thomas Munro

Hi,

On 2014-06-26 23:03:24 +0100, Thomas Munro wrote:
> + {"cluster_name", PGC_POSTMASTER, CONN_AUTH_SETTINGS,
> + gettext_noop("Sets the name of the cluster that appears in 'ps' output."),
> + NULL,
> + GUC_IS_NAME
> + },
> + &cluster_name,
> + "",
> + NULL, NULL, NULL
> + },
> +

In my opinion this should rather be LOGGING or LOGGING_WHAT rather than
CONN_AUTH_SETTINGS. I don't really see what place it it in the latter
category?

Possibly you've copied it from bonjour? But that's in the category
because it's used to advertises connections which isn't the case for
cluster_name.

I also don't see a reason for it to be marked as GUC_IS_NAME? That's
about truncating it so it fits into a sql identifer. Not that it should
ever play a role...

> +#cluster_name = '' # visible in ps output if set
> + # (change requires restart)

Not sure if referring to ps is the best thing here. Maybe 'visible in
the processes name if set'? Same for the GUC's description string.

Otherwise it looks good to me.

Greetings,

Andres Freund

Hi,

On 2014-06-28 15:08:45 +0200, Andres Freund wrote:
> Otherwise it looks good to me.

So, I'd looked at it with an eye towards committing it and found some
more things. I've now
* added the restriction that the cluster name can only be ASCII. It's
shown from several clusters with differing encodings, and it's shown
in process titles, so that seems wise.
* moved everything to the LOGGING_WHAT category
* added minimal documention to monitoring.sgml
* expanded the config.sgml entry to mention the restriction on the name.
* Changed the GUCs short description

I'll leave the patch sit a while before actually committing it.

I also think, but haven't done so, we should add a double colon after
the cluster name, so it's not:

postgres: server1 stats collector process
but
postgres: server1: stats collector process

Comments?

Greetings,

Andres Freund

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

On 29 June 2014 10:55, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> So, I'd looked at it with an eye towards committing it and found some
> more things. I've now
> * added the restriction that the cluster name can only be ASCII. It's
> shown from several clusters with differing encodings, and it's shown
> in process titles, so that seems wise.
> * moved everything to the LOGGING_WHAT category
> * added minimal documention to monitoring.sgml
> * expanded the config.sgml entry to mention the restriction on the name.
> * Changed the GUCs short description

Thank you.

> I also think, but haven't done so, we should add a double colon after
> the cluster name, so it's not:
>
> postgres: server1 stats collector process
> but
> postgres: server1: stats collector process

+1

Best regards,
Thomas Munro

On 2014-06-29 11:11:14 +0100, Thomas Munro wrote:
> On 29 June 2014 10:55, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > So, I'd looked at it with an eye towards committing it and found some
> > more things. I've now
> > * added the restriction that the cluster name can only be ASCII. It's
> > shown from several clusters with differing encodings, and it's shown
> > in process titles, so that seems wise.
> > * moved everything to the LOGGING_WHAT category
> > * added minimal documention to monitoring.sgml
> > * expanded the config.sgml entry to mention the restriction on the name.
> > * Changed the GUCs short description
>
> Thank you.
>
> > I also think, but haven't done so, we should add a double colon after
> > the cluster name, so it's not:
> >
> > postgres: server1 stats collector process
> > but
> > postgres: server1: stats collector process
>
> +1

Committed with the above changes. Thanks for the contribution!

Greetings,

Andres Freund

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

On Sun, Jun 29, 2014 at 9:25 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2014-06-29 11:11:14 +0100, Thomas Munro wrote:
>> On 29 June 2014 10:55, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> > So, I'd looked at it with an eye towards committing it and found some
>> > more things. I've now
>> > * added the restriction that the cluster name can only be ASCII. It's
>> > shown from several clusters with differing encodings, and it's shown
>> > in process titles, so that seems wise.
>> > * moved everything to the LOGGING_WHAT category
>> > * added minimal documention to monitoring.sgml
>> > * expanded the config.sgml entry to mention the restriction on the name.
>> > * Changed the GUCs short description
>>
>> Thank you.
>>
>> > I also think, but haven't done so, we should add a double colon after
>> > the cluster name, so it's not:
>> >
>> > postgres: server1 stats collector process
>> > but
>> > postgres: server1: stats collector process
>>
>> +1
>
> Committed with the above changes. Thanks for the contribution!

+ build). Only printable ASCII characters may be used in the
+ <varname>application_name</varname> value. Other characters will be

application_name should be cluster_name?

Regards,

--
Fujii Masao

Abjit, all:

If we're adding log_line_prefix support for cluster_name (something I
think is a good idea), we need to also add it to CSVLOG. So, where do
we put it in CSVLog?

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On 2014-06-30 12:10:53 -0700, Josh Berkus wrote:
> Abjit, all:
>
> If we're adding log_line_prefix support for cluster_name (something I
> think is a good idea), we need to also add it to CSVLOG. So, where do
> we put it in CSVLog?

That was shot down (unfortunately imo), and I don't think anybody
actively working on it.

Greetings,

Andres Freund

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

On 06/29/2014 02:25 PM, Andres Freund wrote:
> On 2014-06-29 11:11:14 +0100, Thomas Munro wrote:
>> > On 29 June 2014 10:55, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>>> > > So, I'd looked at it with an eye towards committing it and found some
>>> > > more things. I've now
>>> > > * added the restriction that the cluster name can only be ASCII. It's
>>> > > shown from several clusters with differing encodings, and it's shown
>>> > > in process titles, so that seems wise.
>>> > > * moved everything to the LOGGING_WHAT category
>>> > > * added minimal documention to monitoring.sgml
>>> > > * expanded the config.sgml entry to mention the restriction on the name.
>>> > > * Changed the GUCs short description
>> >
>> > Thank you.
>> >
>>> > > I also think, but haven't done so, we should add a double colon after
>>> > > the cluster name, so it's not:
>>> > >
>>> > > postgres: server1 stats collector process
>>> > > but
>>> > > postgres: server1: stats collector process
>> >
>> > +1
>
> Committed with the above changes. Thanks for the contribution!

Is there a reason for not using this in synchronous_standby_names,
perhaps falling back to application_name if not set?
--
Vik

On Wed, Jul 2, 2014 at 3:45 AM, Vik Fearing <vik(dot)fearing(at)dalibo(dot)com> wrote:
> On 06/29/2014 02:25 PM, Andres Freund wrote:
>> On 2014-06-29 11:11:14 +0100, Thomas Munro wrote:
>>> > On 29 June 2014 10:55, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>>>> > > So, I'd looked at it with an eye towards committing it and found some
>>>> > > more things. I've now
>>>> > > * added the restriction that the cluster name can only be ASCII. It's
>>>> > > shown from several clusters with differing encodings, and it's shown
>>>> > > in process titles, so that seems wise.
>>>> > > * moved everything to the LOGGING_WHAT category
>>>> > > * added minimal documention to monitoring.sgml
>>>> > > * expanded the config.sgml entry to mention the restriction on the name.
>>>> > > * Changed the GUCs short description
>>> >
>>> > Thank you.
>>> >
>>>> > > I also think, but haven't done so, we should add a double colon after
>>>> > > the cluster name, so it's not:
>>>> > >
>>>> > > postgres: server1 stats collector process
>>>> > > but
>>>> > > postgres: server1: stats collector process
>>> >
>>> > +1
>>
>> Committed with the above changes. Thanks for the contribution!
>
> Is there a reason for not using this in synchronous_standby_names,
> perhaps falling back to application_name if not set?

You mean that if synchronous_standby_names is an empty it automatically
should be set to cluster_name? Or, you mean that if application_name is not
set in primary_conninfo the standby should automatically use its cluster_name
as application_name in primary_conninfo? I'm afraid that those may cause
the trouble such as that standby is unexpectedly treated as synchronous one
even though a user want to set up it as asynchronous one by emptying
synchronous_standby_names in the master.

Regards,

--
Fujii Masao

On 07/04/2014 08:50 AM, Fujii Masao wrote:
> On Wed, Jul 2, 2014 at 3:45 AM, Vik Fearing <vik(dot)fearing(at)dalibo(dot)com> wrote:
>> Is there a reason for not using this in synchronous_standby_names,
>> perhaps falling back to application_name if not set?
>
> You mean that if synchronous_standby_names is an empty it automatically
> should be set to cluster_name? Or, you mean that if application_name is not
> set in primary_conninfo the standby should automatically use its cluster_name
> as application_name in primary_conninfo? I'm afraid that those may cause
> the trouble such as that standby is unexpectedly treated as synchronous one
> even though a user want to set up it as asynchronous one by emptying
> synchronous_standby_names in the master.

No, I mean that synchronous_standby_names should look at cluster_name
first, and if it's not set then unfortunately look at application_name
for backward compatibility.

Using application_name for this always seems like a hack to me, and
cluster_name is a much better fit. We should have created cluster_name
back when we created synchronous_standby_names.
--
Vik

Vik Fearing <vik(dot)fearing(at)dalibo(dot)com> writes:
>> You mean that if synchronous_standby_names is an empty it automatically
>> should be set to cluster_name?

> No, I mean that synchronous_standby_names should look at cluster_name
> first, and if it's not set then unfortunately look at application_name
> for backward compatibility.

I think you're failing to explain yourself very well. What you really
mean is that we should use cluster_name not application_name to determine
the name that a standby server reports to the master.

There's something to that, perhaps, but I think that the mechanism we use
for doing the reporting involves the application_name parameter passed
through libpq. It might be a bit painful to change that, and I'm not
entirely sure it'd be less confusing.

regards, tom lane

On Fri, Jul 4, 2014 at 10:33 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Vik Fearing <vik(dot)fearing(at)dalibo(dot)com> writes:
>>> You mean that if synchronous_standby_names is an empty it automatically
>>> should be set to cluster_name?
>
>> No, I mean that synchronous_standby_names should look at cluster_name
>> first, and if it's not set then unfortunately look at application_name
>> for backward compatibility.
>
> I think you're failing to explain yourself very well. What you really
> mean is that we should use cluster_name not application_name to determine
> the name that a standby server reports to the master.
>
> There's something to that, perhaps, but I think that the mechanism we use
> for doing the reporting involves the application_name parameter passed
> through libpq. It might be a bit painful to change that, and I'm not
> entirely sure it'd be less confusing.

I actually prefer it the way it is now, I think. Arguably,
application_name is not quite the right thing for identifying a
synchronous standby, because it's intended to answer the question
"What *class* of connection is this?" rather than "Which *precise*
connection is this?". But I don't think there's anything especially
wrong with having a class that has only 1 member when synchronous
replication is in use; indeed, in some ways it seems quite natural.
That connection is after all unique.

OTOH, AIUI, cluster_name is all about what shows up in the ps output,
and is intended to distinguish multiple clusters running on the same
server. If you're not doing that, you likely want cluster_name to be
empty, but you might still want to use synchronous replication. If
you are doing it, you may well want to set it to a value that
distinguishes the server only from others running on the same box,
like maybe the version number or port -- whereas for matching against
synchronous_standby_names, we need a value that's meaningful across
all related servers, but not necessarily distinguishing from other
stuff on the same server.

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

Tom Lane wrote:

> Also, -1 for adding another log_line_prefix escape. If you're routing
> multiple clusters logging to the same place (which is already a bit
> unlikely IMO), you can put distinguishing strings in log_line_prefix
> already. And it's not like we've got an infinite supply of letters
> for those escapes.

FWIW if we find ourselves short on log_line_prefix letters, we can
always invent more verbose notation -- for instance we could have escape
for each GUC var such as %{config:port} for the port number, and so on.
Probably this will be mostly useless for most GUC params, but it might
come in handy for a few of them. And of course we could have more
prefixes apart from "config:".

Not that this affects the current patch in any way.

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