Re: pg_primary_conninfo

Attached patch implements a function called pg_primary_conninfo() that
returns, well, the primary_conninfo used on the standby when in
streaming replication mode (otherwise NULL).

Objections?

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Tue, Dec 28, 2010 at 8:31 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> Attached patch implements a function called pg_primary_conninfo() that
> returns, well, the primary_conninfo used on the standby when in
> streaming replication mode (otherwise NULL).

+1. Let's make sure to explicitly document what this function returns
when recovery was previous in progress, but we are now in normal
running.

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

On Tue, Dec 28, 2010 at 14:38, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Dec 28, 2010 at 8:31 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>> Attached patch implements a function called pg_primary_conninfo() that
>> returns, well, the primary_conninfo used on the standby when in
>> streaming replication mode (otherwise NULL).
>
> +1.  Let's make sure to explicitly document what this function returns
> when recovery was previous in progress, but we are now in normal
> running.

Oh, didn't think of that scenario.

Is that intended behaviour though? I tend to think that it is (since
you can check with pg_is_in_recovery) as long as it's documented, but
might it make more sense to have it return NULL in this case?

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> Attached patch implements a function called pg_primary_conninfo() that
> returns, well, the primary_conninfo used on the standby when in
> streaming replication mode (otherwise NULL).

> Objections?

What's the use case? And aren't there security reasons to NOT expose
that? It might contain a password for instance.

> + if (recptr.xlogid == 0 && recptr.xrecoff == 0 && conninfo[0] != '\0')
> + PG_RETURN_NULL();

This test seems a bit incoherent.

regards, tom lane

On Dec 28, 2010 3:58 PM, "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
> > Attached patch implements a function called pg_primary_conninfo() that
> > returns, well, the primary_conninfo used on the standby when in
> > streaming replication mode (otherwise NULL).
>
> > Objections?
>
> What's the use case? And aren't there security reasons to NOT expose
> that? It might contain a password for instance.

Good point - should be made superuser only.
>

> > + if (recptr.xlogid == 0 && recptr.xrecoff == 0 && conninfo[0] !=
'\0')
> > + PG_RETURN_NULL();
>
> This test seems a bit incoherent.

I used that to test that streaming repl is enabled at all. Is there a better
way?

/Magnus

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> On Dec 28, 2010 3:58 PM, "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> What's the use case? And aren't there security reasons to NOT expose
>> that? It might contain a password for instance.

> Good point - should be made superuser only.

I'm still wondering what's the actual use-case for exposing this inside
SQL. Those with a legitimate need-to-know can look at the slave
server's config files, no?

regards, tom lane

Le 28/12/2010 16:34, Tom Lane a écrit :
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> On Dec 28, 2010 3:58 PM, "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> What's the use case? And aren't there security reasons to NOT expose
>>> that? It might contain a password for instance.
>
>> Good point - should be made superuser only.
>
> I'm still wondering what's the actual use-case for exposing this inside
> SQL. Those with a legitimate need-to-know can look at the slave
> server's config files, no?
>

This is something I wanted to have in 9.0 when I coded in pgAdmin some
features related to the HotStandby. Knowing on which IP is the master
can help pgAdmin offer the user to register the master node.

It's also interesting to get lag between master and slave. As soon as
I'm connected to a slave, I can connect to the master and get the lag
between them. Something I can't do right now in pgAdmin.

--
Guillaume
http://www.postgresql.fr
http://dalibo.com

Guillaume Lelarge <guillaume(at)lelarge(dot)info> writes:
> Le 28/12/2010 16:34, Tom Lane a crit :
>> I'm still wondering what's the actual use-case for exposing this inside
>> SQL. Those with a legitimate need-to-know can look at the slave
>> server's config files, no?

> This is something I wanted to have in 9.0 when I coded in pgAdmin some
> features related to the HotStandby. Knowing on which IP is the master
> can help pgAdmin offer the user to register the master node.

> It's also interesting to get lag between master and slave. As soon as
> I'm connected to a slave, I can connect to the master and get the lag
> between them. Something I can't do right now in pgAdmin.

The proposed primary_conninfo seems like a pretty awful solution to
those problems, though.

1. It'll have to be restricted to superusers, therefore ordinary
users on the slave can't actually make use of it.

2. It's not what you want, since you don't want to connect as the
replication user. Therefore, you'd have to start by parsing out
the parts you do need. Expecting every client to include conninfo
parsing logic doesn't seem cool to me.

I can see the point of, say, a primary_host_address() function returning
inet, which would be way better on both those dimensions than the
current proposal. But I'm not sure what else would be needed.

regards, tom lane

Le 28/12/2010 17:36, Tom Lane a écrit :
> Guillaume Lelarge <guillaume(at)lelarge(dot)info> writes:
>> Le 28/12/2010 16:34, Tom Lane a écrit :
>>> I'm still wondering what's the actual use-case for exposing this inside
>>> SQL. Those with a legitimate need-to-know can look at the slave
>>> server's config files, no?
>
>> This is something I wanted to have in 9.0 when I coded in pgAdmin some
>> features related to the HotStandby. Knowing on which IP is the master
>> can help pgAdmin offer the user to register the master node.
>
>> It's also interesting to get lag between master and slave. As soon as
>> I'm connected to a slave, I can connect to the master and get the lag
>> between them. Something I can't do right now in pgAdmin.
>
> The proposed primary_conninfo seems like a pretty awful solution to
> those problems, though.
>

I would say "not the best one, but better than what I have now" :)

> 1. It'll have to be restricted to superusers, therefore ordinary
> users on the slave can't actually make use of it.
>

pgAdmin's users usually connect as superusers.

> 2. It's not what you want, since you don't want to connect as the
> replication user. Therefore, you'd have to start by parsing out
> the parts you do need. Expecting every client to include conninfo
> parsing logic doesn't seem cool to me.
>
> I can see the point of, say, a primary_host_address() function returning
> inet, which would be way better on both those dimensions than the
> current proposal. But I'm not sure what else would be needed.
>

Yeah, it would be better that way. I'm actually interested in Magnus's
patch because, during 9.0 development phase, I had in mind to parse the
primary_conninfo till I found I could not get this value with SHOW or
current_setting().

But, actually, what I really need is host and port. This way, I could
connect to the master node, with the same user and password that was
used on the slave node.

--
Guillaume
http://www.postgresql.fr
http://dalibo.com

On Tue, Dec 28, 2010 at 11:36 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

>
> I can see the point of, say, a primary_host_address() function returning
> inet, which would be way better on both those dimensions than the
> current proposal. But I'm not sure what else would be needed.
>
>
+1, since it bypasses security risks associated with exposing
username/password.

Ability to see port number will be a useful addition.

Another case to consider is what if slave is connected to a local server
over unix-domain sockets? Returning NULL might make it ambiguous with the
case where the instance has been promoted out of standby.

Regards,
--
gurjeet.singh
@ EnterpriseDB - The Enterprise Postgres Company
http://www.EnterpriseDB.com

singh(dot)gurjeet(at){ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet

Mail sent from my BlackLaptop device

Le 28/12/2010 17:50, Gurjeet Singh a écrit :
> On Tue, Dec 28, 2010 at 11:36 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
>>
>> I can see the point of, say, a primary_host_address() function returning
>> inet, which would be way better on both those dimensions than the
>> current proposal. But I'm not sure what else would be needed.
>>
>>
> +1, since it bypasses security risks associated with exposing
> username/password.
>
> Ability to see port number will be a useful addition.
>
> Another case to consider is what if slave is connected to a local server
> over unix-domain sockets? Returning NULL might make it ambiguous with the
> case where the instance has been promoted out of standby.
>

The host should be the socket file path.

--
Guillaume
http://www.postgresql.fr
http://dalibo.com

On Dec 28, 2010, at 10:34 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I'm still wondering what's the actual use-case for exposing this inside
> SQL. Those with a legitimate need-to-know can look at the slave
> server's config files, no?

SQL access is frequently more convenient, though. Although maybe now that we've made recovery.conf use the GUC lexer we oughta continue in that vein and expose those parameters as PGC_INTERNAL GUCs rather than inventing a new function for it...

...Robert

Le 28/12/2010 18:12, Robert Haas a écrit :
> On Dec 28, 2010, at 10:34 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I'm still wondering what's the actual use-case for exposing this inside
>> SQL. Those with a legitimate need-to-know can look at the slave
>> server's config files, no?
>
> SQL access is frequently more convenient, though. Although maybe now that we've made recovery.conf use the GUC lexer we oughta continue in that vein and expose those parameters as PGC_INTERNAL GUCs rather than inventing a new function for it...
>

That was the first thing I wanted. Knowing the trigger file for example
would be quite useful for pgAdmin and pgPool for example.

--
Guillaume
http://www.postgresql.fr
http://dalibo.com

On Tue, Dec 28, 2010 at 12:12 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Dec 28, 2010, at 10:34 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > I'm still wondering what's the actual use-case for exposing this inside
> > SQL. Those with a legitimate need-to-know can look at the slave
> > server's config files, no?
>
> SQL access is frequently more convenient, though. Although maybe now that
> we've made recovery.conf use the GUC lexer we oughta continue in that vein
> and expose those parameters as PGC_INTERNAL GUCs rather than inventing a new
> function for it...
>
>
+1 for SQL access, but exposing it via pg_settings opens up the security
problem as there might be sensitive info in those GUCs.

Regards,
--
gurjeet.singh
@ EnterpriseDB - The Enterprise Postgres Company
http://www.EnterpriseDB.com

singh(dot)gurjeet(at){ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet

Mail sent from my BlackLaptop device

Gurjeet Singh <singh(dot)gurjeet(at)gmail(dot)com> writes:
> On Tue, Dec 28, 2010 at 12:12 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> SQL access is frequently more convenient, though. Although maybe now that
>> we've made recovery.conf use the GUC lexer we oughta continue in that vein
>> and expose those parameters as PGC_INTERNAL GUCs rather than inventing a new
>> function for it...

> +1 for SQL access, but exposing it via pg_settings opens up the security
> problem as there might be sensitive info in those GUCs.

IIRC we do have a GUC property that hides the value from non-superusers,
so we could easily have a GUC that is equivalent to the proposed
pg_primary_conninfo function. Of course this does nothing for my
objections to the function. Also, I'm not sure how we'd deal with the
state-dependency aspect of it (ie, value changes once you exit recovery
mode).

regards, tom lane

Le 28/12/2010 19:30, Tom Lane a écrit :
> Gurjeet Singh <singh(dot)gurjeet(at)gmail(dot)com> writes:
>> On Tue, Dec 28, 2010 at 12:12 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> SQL access is frequently more convenient, though. Although maybe now that
>>> we've made recovery.conf use the GUC lexer we oughta continue in that vein
>>> and expose those parameters as PGC_INTERNAL GUCs rather than inventing a new
>>> function for it...
>
>> +1 for SQL access, but exposing it via pg_settings opens up the security
>> problem as there might be sensitive info in those GUCs.
>
> IIRC we do have a GUC property that hides the value from non-superusers,
> so we could easily have a GUC that is equivalent to the proposed
> pg_primary_conninfo function. Of course this does nothing for my
> objections to the function. Also, I'm not sure how we'd deal with the
> state-dependency aspect of it (ie, value changes once you exit recovery
> mode).
>

We already have superuser GUC.

b1=> show data_directory;
ERROR: must be superuser to examine "data_directory"

We only need to do the same for primary_conninfo and trigger_file (as I
remember it, there are the only ones needing this).

--
Guillaume
http://www.postgresql.fr
http://dalibo.com

On Tue, Dec 28, 2010 at 1:30 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Gurjeet Singh <singh(dot)gurjeet(at)gmail(dot)com> writes:
> > On Tue, Dec 28, 2010 at 12:12 PM, Robert Haas <robertmhaas(at)gmail(dot)com>
> wrote:
> >> SQL access is frequently more convenient, though. Although maybe now
> that
> >> we've made recovery.conf use the GUC lexer we oughta continue in that
> vein
> >> and expose those parameters as PGC_INTERNAL GUCs rather than inventing a
> new
> >> function for it...
>
> > +1 for SQL access, but exposing it via pg_settings opens up the security
> > problem as there might be sensitive info in those GUCs.
>
> IIRC we do have a GUC property that hides the value from non-superusers,
> so we could easily have a GUC that is equivalent to the proposed
> pg_primary_conninfo function. Of course this does nothing for my
> objections to the function. Also, I'm not sure how we'd deal with the
> state-dependency aspect of it (ie, value changes once you exit recovery
> mode).
>

I would vote for making host:port part visible to non-superusers. This info
is definitely usable in combination with pg_current_xlog_location() and
pg_last_xlog_receive_location() to allow non-superusers to monitor streaming
replication.

Given that primary_conninfo is already parsed by libpq, how difficult would
it be to extract and store/display those host:port components.

Regards,
--
gurjeet.singh
@ EnterpriseDB - The Enterprise Postgres Company
http://www.EnterpriseDB.com

singh(dot)gurjeet(at){ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet

Mail sent from my BlackLaptop device

On Tue, Dec 28, 2010 at 17:43, Guillaume Lelarge <guillaume(at)lelarge(dot)info> wrote:
> Le 28/12/2010 17:36, Tom Lane a écrit :
>> Guillaume Lelarge <guillaume(at)lelarge(dot)info> writes:
>>> Le 28/12/2010 16:34, Tom Lane a écrit :
>> 1. It'll have to be restricted to superusers, therefore ordinary
>> users on the slave can't actually make use of it.
>>
>
> pgAdmin's users usually connect as superusers.

It would be a function for DBAs, of course. I don't see why "normal
users" would be intersted in it, really.

>> 2. It's not what you want, since you don't want to connect as the
>> replication user.  Therefore, you'd have to start by parsing out
>> the parts you do need.  Expecting every client to include conninfo
>> parsing logic doesn't seem cool to me.
>>
>> I can see the point of, say, a primary_host_address() function returning
>> inet, which would be way better on both those dimensions than the
>> current proposal.  But I'm not sure what else would be needed.
>>
>
> Yeah, it would be better that way. I'm actually interested in Magnus's
> patch because, during 9.0 development phase, I had in mind to parse the
> primary_conninfo till I found I could not get this value with SHOW or
> current_setting().
>
> But, actually, what I really need is host and port. This way, I could
> connect to the master node, with the same user and password that was
> used on the slave node.

I agree it might well be more useful to have it split up for us. We'd
need the host name (though it would have to be text and not inet,
since we'd need the unix socket path for a local connection) and port.
And username. But certainly not password, and probably none of the
other parameters.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Tue, Dec 28, 2010 at 18:12, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Dec 28, 2010, at 10:34 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I'm still wondering what's the actual use-case for exposing this inside
>> SQL.  Those with a legitimate need-to-know can look at the slave
>> server's config files, no?
>
> SQL access is frequently more convenient, though.

Yes. Reading it in the files does not scale with $LOTS of servers, be
them slaves or masters or both. You can't assume that people have
direct filesystem access to the server (or at least it's data
directory) - particularly when the organisation is large enough that
you have different teams running the db's and the OS's, not to mention
when you have some on-call group who verifies the things in the middle
of the night...

Unless you mean reading them with pg_read_file() and then parsing it
manually, but that just requires everybody to re-invent the wheel we
already have in the parser.

> Although maybe now that we've made recovery.conf use the GUC lexer we oughta continue in that vein and expose those parameters as PGC_INTERNAL GUCs rather than inventing a new function for it...

That's definitely another option that I wouldn't object to if people
prefer that way.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On 29.12.2010 10:36, Magnus Hagander wrote:
> On Tue, Dec 28, 2010 at 18:12, Robert Haas<robertmhaas(at)gmail(dot)com> wrote:
>> Although maybe now that we've made recovery.conf use the GUC lexer we oughta continue in that vein and expose those parameters as PGC_INTERNAL GUCs rather than inventing a new function for it...
>
> That's definitely another option that I wouldn't object to if people
> prefer that way.

I recall from previous discussions that we have a consensus that we
should unite recovery.conf and postgresql.conf, so that they're all GUCs
and you can put all the settings in postgresql.conf. Let's do that.

http://archives.postgresql.org/pgsql-hackers/2010-10/msg00033.php

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> On Tue, Dec 28, 2010 at 18:12, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> Although maybe now that we've made recovery.conf use the GUC lexer we
>>oughta continue in that vein and expose those parameters as
>>PGC_INTERNAL GUCs rather than inventing a new function for it...
>
> That's definitely another option that I wouldn't object to if people
> prefer that way.

+1.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On Wed, Dec 29, 2010 at 5:51 PM, Heikki Linnakangas
<heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> On 29.12.2010 10:36, Magnus Hagander wrote:
>>
>> On Tue, Dec 28, 2010 at 18:12, Robert Haas<robertmhaas(at)gmail(dot)com>  wrote:
>>>
>>>  Although maybe now that we've made recovery.conf use the GUC lexer we
>>> oughta continue in that vein and expose those parameters as PGC_INTERNAL
>>> GUCs rather than inventing a new function for it...
>>
>> That's definitely another option that I wouldn't object to if people
>> prefer that way.
>
> I recall from previous discussions that we have a consensus that we should
> unite recovery.conf and postgresql.conf, so that they're all GUCs and you
> can put all the settings in postgresql.conf. Let's do that.
>
> http://archives.postgresql.org/pgsql-hackers/2010-10/msg00033.php

Simon has argued that we should allow those parameters to be set in
both recovery.conf and postgresql.conf for backward compatibility.
http://archives.postgresql.org/pgsql-hackers/2010-10/msg00017.php

So I'm thinking to make ProcessConfigFile() parse not only postgresql.conf
but also recovery.conf rather than move all the recovery parameters to
postgresql.conf.

Comments?

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Wed, Jan 12, 2011 at 11:52 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> So I'm thinking to make ProcessConfigFile() parse not only postgresql.conf
> but also recovery.conf rather than move all the recovery parameters to
> postgresql.conf.
>
> Comments?

+1.

Actually moving the settings can be done later in about 5 seconds if
we all agree it's a good idea, but let's not get bogged down in that
now.

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