Horton

Thread

  1. pg_primary_conninfo

    Magnus Hagander <magnus@hagander.net> — 2010-12-28T13:31:56Z

    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/

  2. Re: pg_primary_conninfo

    Robert Haas <robertmhaas@gmail.com> — 2010-12-28T13:38:13Z

    On Tue, Dec 28, 2010 at 8:31 AM, Magnus Hagander <magnus@hagander.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

  3. Re: pg_primary_conninfo

    Magnus Hagander <magnus@hagander.net> — 2010-12-28T13:39:37Z

    On Tue, Dec 28, 2010 at 14:38, Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Dec 28, 2010 at 8:31 AM, Magnus Hagander <magnus@hagander.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/

  4. Re: pg_primary_conninfo

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-12-28T14:58:28Z

    Magnus Hagander <magnus@hagander.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

  5. Re: pg_primary_conninfo

    Magnus Hagander <magnus@hagander.net> — 2010-12-28T15:04:56Z

    On Dec 28, 2010 3:58 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
>
> Magnus Hagander <magnus@hagander.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

  6. Re: pg_primary_conninfo

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-12-28T15:34:53Z

    Magnus Hagander <magnus@hagander.net> writes:
> On Dec 28, 2010 3:58 PM, "Tom Lane" <tgl@sss.pgh.pa.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

  7. Re: pg_primary_conninfo

    Guillaume Lelarge <guillaume@lelarge.info> — 2010-12-28T16:30:54Z

    Le 28/12/2010 16:34, Tom Lane a écrit :
> Magnus Hagander <magnus@hagander.net> writes:
>> On Dec 28, 2010 3:58 PM, "Tom Lane" <tgl@sss.pgh.pa.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

  8. Re: pg_primary_conninfo

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-12-28T16:36:14Z

    Guillaume Lelarge <guillaume@lelarge.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

  9. Re: pg_primary_conninfo

    Guillaume Lelarge <guillaume@lelarge.info> — 2010-12-28T16:43:12Z

    Le 28/12/2010 17:36, Tom Lane a écrit :
> Guillaume Lelarge <guillaume@lelarge.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

  10. Re: pg_primary_conninfo

    Gurjeet Singh <singh.gurjeet@gmail.com> — 2010-12-28T16:50:24Z

    On Tue, Dec 28, 2010 at 11:36 AM, Tom Lane <tgl@sss.pgh.pa.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.gurjeet@{ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet

Mail sent from my BlackLaptop device

  11. Re: pg_primary_conninfo

    Guillaume Lelarge <guillaume@lelarge.info> — 2010-12-28T17:02:01Z

    Le 28/12/2010 17:50, Gurjeet Singh a écrit :
> On Tue, Dec 28, 2010 at 11:36 AM, Tom Lane <tgl@sss.pgh.pa.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

  12. Re: pg_primary_conninfo

    Robert Haas <robertmhaas@gmail.com> — 2010-12-28T17:12:00Z

    On Dec 28, 2010, at 10:34 AM, Tom Lane <tgl@sss.pgh.pa.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

  13. Re: pg_primary_conninfo

    Guillaume Lelarge <guillaume@lelarge.info> — 2010-12-28T18:19:58Z

    Le 28/12/2010 18:12, Robert Haas a écrit :
> On Dec 28, 2010, at 10:34 AM, Tom Lane <tgl@sss.pgh.pa.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

  14. Re: pg_primary_conninfo

    Gurjeet Singh <singh.gurjeet@gmail.com> — 2010-12-28T18:22:48Z

    On Tue, Dec 28, 2010 at 12:12 PM, Robert Haas <robertmhaas@gmail.com> wrote:

> On Dec 28, 2010, at 10:34 AM, Tom Lane <tgl@sss.pgh.pa.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.gurjeet@{ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet

Mail sent from my BlackLaptop device

  15. Re: pg_primary_conninfo

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-12-28T18:30:44Z

    Gurjeet Singh <singh.gurjeet@gmail.com> writes:
> On Tue, Dec 28, 2010 at 12:12 PM, Robert Haas <robertmhaas@gmail.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

  16. Re: pg_primary_conninfo

    Guillaume Lelarge <guillaume@lelarge.info> — 2010-12-28T18:52:12Z

    Le 28/12/2010 19:30, Tom Lane a écrit :
> Gurjeet Singh <singh.gurjeet@gmail.com> writes:
>> On Tue, Dec 28, 2010 at 12:12 PM, Robert Haas <robertmhaas@gmail.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

  17. Re: pg_primary_conninfo

    Gurjeet Singh <singh.gurjeet@gmail.com> — 2010-12-28T19:02:36Z

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

> Gurjeet Singh <singh.gurjeet@gmail.com> writes:
> > On Tue, Dec 28, 2010 at 12:12 PM, Robert Haas <robertmhaas@gmail.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.gurjeet@{ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet

Mail sent from my BlackLaptop device

  18. Re: pg_primary_conninfo

    Magnus Hagander <magnus@hagander.net> — 2010-12-29T08:33:24Z

    On Tue, Dec 28, 2010 at 17:43, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> Le 28/12/2010 17:36, Tom Lane a écrit :
>> Guillaume Lelarge <guillaume@lelarge.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/

  19. Re: pg_primary_conninfo

    Magnus Hagander <magnus@hagander.net> — 2010-12-29T08:36:03Z

    On Tue, Dec 28, 2010 at 18:12, Robert Haas <robertmhaas@gmail.com> wrote:
> On Dec 28, 2010, at 10:34 AM, Tom Lane <tgl@sss.pgh.pa.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/

  20. Re: pg_primary_conninfo

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-12-29T08:51:42Z

    On 29.12.2010 10:36, Magnus Hagander wrote:
> On Tue, Dec 28, 2010 at 18:12, Robert Haas<robertmhaas@gmail.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

  21. Re: pg_primary_conninfo

    Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2010-12-29T21:12:20Z

    Magnus Hagander <magnus@hagander.net> writes:
> On Tue, Dec 28, 2010 at 18:12, Robert Haas <robertmhaas@gmail.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

  22. Re: pg_primary_conninfo

    Fujii Masao <masao.fujii@gmail.com> — 2011-01-13T04:52:20Z

    On Wed, Dec 29, 2010 at 5:51 PM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
> On 29.12.2010 10:36, Magnus Hagander wrote:
>>
>> On Tue, Dec 28, 2010 at 18:12, Robert Haas<robertmhaas@gmail.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

  23. Re: pg_primary_conninfo

    Robert Haas <robertmhaas@gmail.com> — 2011-01-13T11:06:08Z

    On Wed, Jan 12, 2011 at 11:52 PM, Fujii Masao <masao.fujii@gmail.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