Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
On Thu, Jun 30, 2016 at 10:24 AM, Alvaro Herrera wrote: > Also, actually, I see no reason for the conninfo to be shown differently > regardless of a connection being already established. If we show the > conninfo that the server is trying to use, it might be easier to > diagnose a problem. In short, I think this is all misconceived (mea > culpa) and that we should have two conninfo members in that struct as > initially proposed, one obfuscated and the other not. Seriously! The whole problem here is being created by trying to use the same field for two different purposes: 1. The string that should actually be used for connections. 2. The sanitized version that should be exposed to the user. If you try to use the same variable to store two different values, both bugs and confusion may result. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
On Thu, Jul 7, 2016 at 4:43 AM, Stephen Frost wrote: > All, > > * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: >> Michael Paquier wrote: >> > On Wed, Jul 6, 2016 at 7:34 PM, Fujii Masao wrote: >> > > I have one question; why do we call the column "conn_info" instead of >> > > "conninfo" which is basically used in other places? "conninfo" is better >> > > to me. >> > >> > No real reason for one or the other to be honest. If you want to >> > change it you could just apply the attached. >> >> I was of two minds myself, and found no reason to change conn_info, so I >> decided to keep what was submitted. If you want to change it, I'm not >> opposed. >> >> Don't forget to bump catversion. > > 'conninfo' certainly seems to be more commonly used and I believe is > what was agreed to up-thread. +1. So since no one objects to change the column name, I applied Michael's patch. Thanks! Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
All, * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > Michael Paquier wrote: > > On Wed, Jul 6, 2016 at 7:34 PM, Fujii Masao wrote: > > > I have one question; why do we call the column "conn_info" instead of > > > "conninfo" which is basically used in other places? "conninfo" is better > > > to me. > > > > No real reason for one or the other to be honest. If you want to > > change it you could just apply the attached. > > I was of two minds myself, and found no reason to change conn_info, so I > decided to keep what was submitted. If you want to change it, I'm not > opposed. > > Don't forget to bump catversion. 'conninfo' certainly seems to be more commonly used and I believe is what was agreed to up-thread. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
Michael Paquier wrote: > On Wed, Jul 6, 2016 at 7:34 PM, Fujii Masao wrote: > > I have one question; why do we call the column "conn_info" instead of > > "conninfo" which is basically used in other places? "conninfo" is better to > > me. > > No real reason for one or the other to be honest. If you want to > change it you could just apply the attached. I was of two minds myself, and found no reason to change conn_info, so I decided to keep what was submitted. If you want to change it, I'm not opposed. Don't forget to bump catversion. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
On Wed, Jul 6, 2016 at 7:34 PM, Fujii Masao wrote:
> I have one question; why do we call the column "conn_info" instead of
> "conninfo" which is basically used in other places? "conninfo" is better to
> me.
No real reason for one or the other to be honest. If you want to
change it you could just apply the attached.
--
Michael
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index a8b8bb0..b620feb 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -1303,7 +1303,7 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
Replication slot name used by this WAL receiver
- conn_info
+ conninfo
text
Connection string used by this WAL receiver,
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index f52de3a..4fc5d5a 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -682,7 +682,7 @@ CREATE VIEW pg_stat_wal_receiver AS
s.latest_end_lsn,
s.latest_end_time,
s.slot_name,
-s.conn_info
+s.conninfo
FROM pg_stat_get_wal_receiver() s
WHERE s.pid IS NOT NULL;
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index d92c05e..5d233e3 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -2746,7 +2746,7 @@ DATA(insert OID = 3318 ( pg_stat_get_progress_info PGNSP PGUID 12 1 100 0 0
DESCR("statistics: information about progress of backends running maintenance command");
DATA(insert OID = 3099 ( pg_stat_get_wal_senders PGNSP PGUID 12 1 10 0 0 f f f f f t s r 0 0 2249 "" "{23,25,3220,3220,3220,3220,23,25}" "{o,o,o,o,o,o,o,o}" "{pid,state,sent_location,write_location,flush_location,replay_location,sync_priority,sync_state}" _null_ _null_ pg_stat_get_wal_senders _null_ _null_ _null_ ));
DESCR("statistics: information about currently active replication");
-DATA(insert OID = 3317 ( pg_stat_get_wal_receiver PGNSP PGUID 12 1 0 0 0 f f f f f f s r 0 0 2249 "" "{23,25,3220,23,3220,23,1184,1184,3220,1184,25,25}" "{o,o,o,o,o,o,o,o,o,o,o,o}" "{pid,status,receive_start_lsn,receive_start_tli,received_lsn,received_tli,last_msg_send_time,last_msg_receipt_time,latest_end_lsn,latest_end_time,slot_name,conn_info}" _null_ _null_ pg_stat_get_wal_receiver _null_ _null_ _null_ ));
+DATA(insert OID = 3317 ( pg_stat_get_wal_receiver PGNSP PGUID 12 1 0 0 0 f f f f f f s r 0 0 2249 "" "{23,25,3220,23,3220,23,1184,1184,3220,1184,25,25}" "{o,o,o,o,o,o,o,o,o,o,o,o}" "{pid,status,receive_start_lsn,receive_start_tli,received_lsn,received_tli,last_msg_send_time,last_msg_receipt_time,latest_end_lsn,latest_end_time,slot_name,conninfo}" _null_ _null_ pg_stat_get_wal_receiver _null_ _null_ _null_ ));
DESCR("statistics: information about WAL receiver");
DATA(insert OID = 2026 ( pg_backend_pidPGNSP PGUID 12 1 0 0 0 f f f f t f s r 0 0 23 "" _null_ _null_ _null_ _null_ _null_ pg_backend_pid _null_ _null_ _null_ ));
DESCR("statistics: current backend PID");
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 125c31b..ad44ae2 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1893,8 +1893,8 @@ pg_stat_wal_receiver| SELECT s.pid,
s.latest_end_lsn,
s.latest_end_time,
s.slot_name,
-s.conn_info
- FROM pg_stat_get_wal_receiver() s(pid, status, receive_start_lsn, receive_start_tli, received_lsn, received_tli, last_msg_send_time, last_msg_receipt_time, latest_end_lsn, latest_end_time, slot_name, conn_info)
+s.conninfo
+ FROM pg_stat_get_wal_receiver() s(pid, status, receive_start_lsn, receive_start_tli, received_lsn, received_tli, last_msg_send_time, last_msg_receipt_time, latest_end_lsn, latest_end_time, slot_name, conninfo)
WHERE (s.pid IS NOT NULL);
pg_stat_xact_all_tables| SELECT c.oid AS relid,
n.nspname AS schemaname,
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
On Mon, Jul 4, 2016 at 12:40 PM, Michael Paquier wrote: > On Sat, Jul 2, 2016 at 2:56 AM, Alvaro Herrera > wrote: >> Michael Paquier wrote: >>> On Fri, Jul 1, 2016 at 8:50 AM, Michael Paquier >>> wrote: >> >>> >> Okay, that argument I buy. >>> >> >>> >> I suppose this function/view should report no row at all if there is no >>> >> wal receiver connected, rather than a view with nulls. >>> > >>> > The function returns PG_RETURN_NULL() so as we don't have to use a >>> > SRF, and the view checks for IS NOT NULL, so there would be no rows >>> > popping up. >>> >>> In short, I would just go with the attached and call it a day. >> >> Done, thanks. Thanks! I have one question; why do we call the column "conn_info" instead of "conninfo" which is basically used in other places? "conninfo" is better to me. Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
On Sat, Jul 2, 2016 at 2:56 AM, Alvaro Herrera wrote: > Michael Paquier wrote: >> On Fri, Jul 1, 2016 at 8:50 AM, Michael Paquier >> wrote: > >> >> Okay, that argument I buy. >> >> >> >> I suppose this function/view should report no row at all if there is no >> >> wal receiver connected, rather than a view with nulls. >> > >> > The function returns PG_RETURN_NULL() so as we don't have to use a >> > SRF, and the view checks for IS NOT NULL, so there would be no rows >> > popping up. >> >> In short, I would just go with the attached and call it a day. > > Done, thanks. Thanks. I have noticed that the item was still in CLOSE_WAIT, so I have moved it to the section of resolved items. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
Michael Paquier wrote: > On Fri, Jul 1, 2016 at 8:50 AM, Michael Paquier > wrote: > >> Okay, that argument I buy. > >> > >> I suppose this function/view should report no row at all if there is no > >> wal receiver connected, rather than a view with nulls. > > > > The function returns PG_RETURN_NULL() so as we don't have to use a > > SRF, and the view checks for IS NOT NULL, so there would be no rows > > popping up. > > In short, I would just go with the attached and call it a day. Done, thanks. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
On Fri, Jul 1, 2016 at 8:50 AM, Michael Paquier
wrote:
> On Fri, Jul 1, 2016 at 8:48 AM, Alvaro Herrera
> wrote:
>> Michael Paquier wrote:
>>> Yeah, I know. Now my opinion regarding this view is that we should
>>> show information about a currently-working WAL receiver, and that it
>>> has nothing to do with reporting information of its previous startup state.
>>> That's more consistent with the WAL sender.
>>
>> Okay, that argument I buy.
>>
>> I suppose this function/view should report no row at all if there is no
>> wal receiver connected, rather than a view with nulls.
>
> The function returns PG_RETURN_NULL() so as we don't have to use a
> SRF, and the view checks for IS NOT NULL, so there would be no rows
> popping up.
In short, I would just go with the attached and call it a day.
--
Michael
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index d552f04..9d858ce 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -770,6 +770,7 @@ WalRcvDie(int code, Datum arg)
Assert(walrcv->pid == MyProcPid);
walrcv->walRcvState = WALRCV_STOPPED;
walrcv->pid = 0;
+ walrcv->ready_to_display = false;
SpinLockRelease(&walrcv->mutex);
/* Terminate the connection gracefully. */
@@ -1344,24 +1345,9 @@ pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
char *conninfo;
/* No WAL receiver, just return a tuple with NULL values */
- if (walrcv->pid == 0)
+ if (walrcv->pid == 0 || !walrcv->ready_to_display)
PG_RETURN_NULL();
- /*
- * Users attempting to read this data mustn't be shown security sensitive
- * data, so sleep until everything has been properly obfuscated.
- */
-retry:
- SpinLockAcquire(&walrcv->mutex);
- if (!walrcv->ready_to_display)
- {
- SpinLockRelease(&walrcv->mutex);
- CHECK_FOR_INTERRUPTS();
- pg_usleep(1000);
- goto retry;
- }
- SpinLockRelease(&walrcv->mutex);
-
/* determine result type */
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
On Fri, Jul 1, 2016 at 8:48 AM, Alvaro Herrera wrote: > Michael Paquier wrote: >> Yeah, I know. Now my opinion regarding this view is that we should >> show information about a currently-working WAL receiver, and that it >> has nothing to do with reporting information of its previous startup state. >> That's more consistent with the WAL sender. > > Okay, that argument I buy. > > I suppose this function/view should report no row at all if there is no > wal receiver connected, rather than a view with nulls. The function returns PG_RETURN_NULL() so as we don't have to use a SRF, and the view checks for IS NOT NULL, so there would be no rows popping up. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
Michael Paquier wrote: > On Fri, Jul 1, 2016 at 8:35 AM, Alvaro Herrera > wrote: > > Michael Paquier wrote: > >> On Thu, Jun 30, 2016 at 11:24 PM, Alvaro Herrera > >> wrote: > >> > Also, actually, I see no reason for the conninfo to be shown differently > >> > regardless of a connection being already established. If we show the > >> > conninfo that the server is trying to use, it might be easier to > >> > diagnose a problem. In short, I think this is all misconceived (mea > >> > culpa) and that we should have two conninfo members in that struct as > >> > initially proposed, one obfuscated and the other not. > >> > >> If the conninfo is leaking an incorrect password, say it has only a > >> couple of characters of difference with the real one, we'd still leak > >> information. > > > > No, I don't mean to leak any password. It would still be obfuscated, > > but all other details would be there (anything with default values). > > OK. There is no need to use two fields by the way. The WAL receiver > makes no attempts to reconnect with the same string and leaves immediately > should a connection fail. Yes, but the question is what happens if somebody queries before walreceiver attempts to connect, no? That's the case where the current code loops. > >> The window where the information of a failed connection is rather > >> limited as well, the WAL receiver process shuts down immediately and > >> would reset its PID to 0, hiding the information anyway. > > > > Some of the details are set by the startup process, such as the start > > LSN etc, not the walreceiver. Only the PID is reset AFAICS. > > Yeah, I know. Now my opinion regarding this view is that we should > show information about a currently-working WAL receiver, and that it > has nothing to do with reporting information of its previous startup state. > That's more consistent with the WAL sender. Okay, that argument I buy. I suppose this function/view should report no row at all if there is no wal receiver connected, rather than a view with nulls. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
On Fri, Jul 1, 2016 at 8:35 AM, Alvaro Herrera wrote: > Michael Paquier wrote: >> On Thu, Jun 30, 2016 at 11:24 PM, Alvaro Herrera >> wrote: >> > Also, actually, I see no reason for the conninfo to be shown differently >> > regardless of a connection being already established. If we show the >> > conninfo that the server is trying to use, it might be easier to >> > diagnose a problem. In short, I think this is all misconceived (mea >> > culpa) and that we should have two conninfo members in that struct as >> > initially proposed, one obfuscated and the other not. >> >> If the conninfo is leaking an incorrect password, say it has only a >> couple of characters of difference with the real one, we'd still leak >> information. > > No, I don't mean to leak any password. It would still be obfuscated, > but all other details would be there (anything with default values). OK. There is no need to use two fields by the way. The WAL receiver makes no attempts to reconnect with the same string and leaves immediately should a connection fail. >> The window where the information of a failed connection is rather >> limited as well, the WAL receiver process shuts down immediately and >> would reset its PID to 0, hiding the information anyway. > > Some of the details are set by the startup process, such as the start > LSN etc, not the walreceiver. Only the PID is reset AFAICS. Yeah, I know. Now my opinion regarding this view is that we should show information about a currently-working WAL receiver, and that it has nothing to do with reporting information of its previous startup state. That's more consistent with the WAL sender. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
Michael Paquier wrote: > On Thu, Jun 30, 2016 at 11:24 PM, Alvaro Herrera > wrote: > > Also, actually, I see no reason for the conninfo to be shown differently > > regardless of a connection being already established. If we show the > > conninfo that the server is trying to use, it might be easier to > > diagnose a problem. In short, I think this is all misconceived (mea > > culpa) and that we should have two conninfo members in that struct as > > initially proposed, one obfuscated and the other not. > > If the conninfo is leaking an incorrect password, say it has only a > couple of characters of difference with the real one, we'd still leak > information. No, I don't mean to leak any password. It would still be obfuscated, but all other details would be there (anything with default values). > The window where the information of a failed connection is rather > limited as well, the WAL receiver process shuts down immediately and > would reset its PID to 0, hiding the information anyway. Some of the details are set by the startup process, such as the start LSN etc, not the walreceiver. Only the PID is reset AFAICS. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
On Thu, Jun 30, 2016 at 11:24 PM, Alvaro Herrera wrote: > Also, actually, I see no reason for the conninfo to be shown differently > regardless of a connection being already established. If we show the > conninfo that the server is trying to use, it might be easier to > diagnose a problem. In short, I think this is all misconceived (mea > culpa) and that we should have two conninfo members in that struct as > initially proposed, one obfuscated and the other not. If the conninfo is leaking an incorrect password, say it has only a couple of characters of difference with the real one, we'd still leak information. That's not good IMO based on the concerns raised on this thread. I'd just mark all the fields as NULL in this case and move on. This way the code keeps being simple, and having this information means that the WAL receiver is correctly working. The window where the information of a failed connection is rather limited as well, the WAL receiver process shuts down immediately and would reset its PID to 0, hiding the information anyway. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
Fujii Masao wrote: > On Thu, Jun 30, 2016 at 10:12 PM, Michael Paquier > wrote: > > On Thu, Jun 30, 2016 at 9:41 PM, Alvaro Herrera > > wrote: > >> Fujii Masao wrote: > >>> On Thu, Jun 30, 2016 at 9:30 PM, Michael Paquier > >>> wrote: > >>> > On Thu, Jun 30, 2016 at 8:59 PM, Fujii Masao > >>> > wrote: > >> > >>> >> ISTM that we will never be able to get out of this loop if walreceiver > >>> >> fails to connect to the master (e.g., password is wrong) after we enter > >>> >> this loop. > >>> > > >>> > Wouldn't it be cleaner to just return an error here instead of retrying? > >>> > >>> I prefer to return NULL. Now NULL is returned when walreceiver's pid is 0. > >>> We can just change this logic so that NULL is returned pid is 0 OR the > >>> flag is false. > >> > >> For the conninfo only, or for everything? > > > > All of them. If this connstr is not ready for display, the WAL > > receiver does not have a proper connection yet, so there is nothing > > worth showing anyway to the user. > > +1 slotname seems worth showing. And if this process just started after some other process was already receiving, then the LSN fields surely can have useful data too. Also, actually, I see no reason for the conninfo to be shown differently regardless of a connection being already established. If we show the conninfo that the server is trying to use, it might be easier to diagnose a problem. In short, I think this is all misconceived (mea culpa) and that we should have two conninfo members in that struct as initially proposed, one obfuscated and the other not. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
On Thu, Jun 30, 2016 at 10:12 PM, Michael Paquier wrote: > On Thu, Jun 30, 2016 at 9:41 PM, Alvaro Herrera > wrote: >> Fujii Masao wrote: >>> On Thu, Jun 30, 2016 at 9:30 PM, Michael Paquier >>> wrote: >>> > On Thu, Jun 30, 2016 at 8:59 PM, Fujii Masao >>> > wrote: >> >>> >> ISTM that we will never be able to get out of this loop if walreceiver >>> >> fails to connect to the master (e.g., password is wrong) after we enter >>> >> this loop. >>> > >>> > Wouldn't it be cleaner to just return an error here instead of retrying? >>> >>> I prefer to return NULL. Now NULL is returned when walreceiver's pid is 0. >>> We can just change this logic so that NULL is returned pid is 0 OR the >>> flag is false. >> >> For the conninfo only, or for everything? > > All of them. If this connstr is not ready for display, the WAL > receiver does not have a proper connection yet, so there is nothing > worth showing anyway to the user. +1 Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
On Thu, Jun 30, 2016 at 9:41 PM, Alvaro Herrera wrote: > Fujii Masao wrote: >> On Thu, Jun 30, 2016 at 9:30 PM, Michael Paquier >> wrote: >> > On Thu, Jun 30, 2016 at 8:59 PM, Fujii Masao wrote: > >> >> ISTM that we will never be able to get out of this loop if walreceiver >> >> fails to connect to the master (e.g., password is wrong) after we enter >> >> this loop. >> > >> > Wouldn't it be cleaner to just return an error here instead of retrying? >> >> I prefer to return NULL. Now NULL is returned when walreceiver's pid is 0. >> We can just change this logic so that NULL is returned pid is 0 OR the >> flag is false. > > For the conninfo only, or for everything? All of them. If this connstr is not ready for display, the WAL receiver does not have a proper connection yet, so there is nothing worth showing anyway to the user. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
Fujii Masao wrote: > On Thu, Jun 30, 2016 at 9:30 PM, Michael Paquier > wrote: > > On Thu, Jun 30, 2016 at 8:59 PM, Fujii Masao wrote: > >> ISTM that we will never be able to get out of this loop if walreceiver > >> fails to connect to the master (e.g., password is wrong) after we enter > >> this loop. > > > > Wouldn't it be cleaner to just return an error here instead of retrying? > > I prefer to return NULL. Now NULL is returned when walreceiver's pid is 0. > We can just change this logic so that NULL is returned pid is 0 OR the > flag is false. For the conninfo only, or for everything? -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
On Thu, Jun 30, 2016 at 9:35 PM, Fujii Masao wrote:
> On Thu, Jun 30, 2016 at 9:30 PM, Michael Paquier
> wrote:
>> On Thu, Jun 30, 2016 at 8:59 PM, Fujii Masao wrote:
>>> (2)
>>> +retry:
>>> +SpinLockAcquire(&walrcv->mutex);
>>> +if (!walrcv->ready_to_display)
>>> +{
>>> +SpinLockRelease(&walrcv->mutex);
>>> +CHECK_FOR_INTERRUPTS();
>>> +pg_usleep(1000);
>>> +goto retry;
>>> +}
>>> +SpinLockRelease(&walrcv->mutex);
>>>
>>> ISTM that we will never be able to get out of this loop if walreceiver
>>> fails to connect to the master (e.g., password is wrong) after we enter
>>> this loop.
>>
>> Wouldn't it be cleaner to just return an error here instead of retrying?
>
> I prefer to return NULL. Now NULL is returned when walreceiver's pid is 0.
> We can just change this logic so that NULL is returned pid is 0 OR the
> flag is false.
OK, yes. That's indeed better this way. Need a patch?
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
On Thu, Jun 30, 2016 at 9:30 PM, Michael Paquier
wrote:
> On Thu, Jun 30, 2016 at 8:59 PM, Fujii Masao wrote:
>> (2)
>> +retry:
>> +SpinLockAcquire(&walrcv->mutex);
>> +if (!walrcv->ready_to_display)
>> +{
>> +SpinLockRelease(&walrcv->mutex);
>> +CHECK_FOR_INTERRUPTS();
>> +pg_usleep(1000);
>> +goto retry;
>> +}
>> +SpinLockRelease(&walrcv->mutex);
>>
>> ISTM that we will never be able to get out of this loop if walreceiver
>> fails to connect to the master (e.g., password is wrong) after we enter
>> this loop.
>
> Wouldn't it be cleaner to just return an error here instead of retrying?
I prefer to return NULL. Now NULL is returned when walreceiver's pid is 0.
We can just change this logic so that NULL is returned pid is 0 OR the
flag is false.
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
Fujii Masao wrote:
> On Thu, Jun 30, 2016 at 6:01 AM, Alvaro Herrera
> wrote:
> > Alvaro Herrera wrote:
> >
> >> I propose to push this patch, closing the open item, and you can rework
> >> on top -- I suppose you would completely remove the original conninfo
> >> from shared memory and instead only copy the obfuscated version there
> >> (and probably also remove the ready_to_display flag). I think we'd need
> >> to see the patch before deciding whether we want it in 9.6 or not,
> >> keeping in mind that having the conninfo in shared memory is a
> >> pre-existing problem, unrelated to the pgstats view new in 9.6.
> >
> > Pushed this.
>
> Thanks for pushing the patch!
> But I found two problems in the patch you pushed.
>
> (1)
> ready_to_display flag must be reset to false when walreceiver dies.
> Otherwise, pg_stat_wal_receiver can report the password (i.e.,
> the problem that I reported upthread can happen) when walreceiver restarts
> because ready_to_display flag is true from the beginning in that case.
> But you forgot to reset the flag to false when walreceiver dies.
Oops, you're right, since it's in shmem it doesn't get reset in the new
process. Will fix.
> (2)
> +retry:
> +SpinLockAcquire(&walrcv->mutex);
> +if (!walrcv->ready_to_display)
> +{
> +SpinLockRelease(&walrcv->mutex);
> +CHECK_FOR_INTERRUPTS();
> +pg_usleep(1000);
> +goto retry;
> +}
> +SpinLockRelease(&walrcv->mutex);
>
> ISTM that we will never be able to get out of this loop if walreceiver
> fails to connect to the master (e.g., password is wrong) after we enter
> this loop.
Yeah, I thought that was OK.
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
On Thu, Jun 30, 2016 at 8:59 PM, Fujii Masao wrote:
> (2)
> +retry:
> +SpinLockAcquire(&walrcv->mutex);
> +if (!walrcv->ready_to_display)
> +{
> +SpinLockRelease(&walrcv->mutex);
> +CHECK_FOR_INTERRUPTS();
> +pg_usleep(1000);
> +goto retry;
> +}
> +SpinLockRelease(&walrcv->mutex);
>
> ISTM that we will never be able to get out of this loop if walreceiver
> fails to connect to the master (e.g., password is wrong) after we enter
> this loop.
Wouldn't it be cleaner to just return an error here instead of retrying?
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
On Thu, Jun 30, 2016 at 6:01 AM, Alvaro Herrera
wrote:
> Alvaro Herrera wrote:
>
>> I propose to push this patch, closing the open item, and you can rework
>> on top -- I suppose you would completely remove the original conninfo
>> from shared memory and instead only copy the obfuscated version there
>> (and probably also remove the ready_to_display flag). I think we'd need
>> to see the patch before deciding whether we want it in 9.6 or not,
>> keeping in mind that having the conninfo in shared memory is a
>> pre-existing problem, unrelated to the pgstats view new in 9.6.
>
> Pushed this.
Thanks for pushing the patch!
But I found two problems in the patch you pushed.
(1)
ready_to_display flag must be reset to false when walreceiver dies.
Otherwise, pg_stat_wal_receiver can report the password (i.e.,
the problem that I reported upthread can happen) when walreceiver restarts
because ready_to_display flag is true from the beginning in that case.
But you forgot to reset the flag to false when walreceiver dies.
(2)
+retry:
+SpinLockAcquire(&walrcv->mutex);
+if (!walrcv->ready_to_display)
+{
+SpinLockRelease(&walrcv->mutex);
+CHECK_FOR_INTERRUPTS();
+pg_usleep(1000);
+goto retry;
+}
+SpinLockRelease(&walrcv->mutex);
ISTM that we will never be able to get out of this loop if walreceiver
fails to connect to the master (e.g., password is wrong) after we enter
this loop.
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
Michael Paquier writes: > On Thu, Jun 30, 2016 at 6:47 AM, Tom Lane wrote: >> It strikes me that keeping a password embedded in the conninfo from being >> exposed might be quite a bit harder/riskier if it became a GUC. Something >> to keep in mind if we ever try to make that change ... > Exposing it in memory for a long time is an issue even if we have a > new GUC-flag to obfuscate the value in some cases.. Well, mumble ... I'm having a hard time understanding the threat model we're guarding against there. An attacker who can read process memory can probably read the config file too. I don't mind getting rid of the in-memory copy if it's painless to do so, but I doubt that it's worth any large amount of effort. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
On Thu, Jun 30, 2016 at 6:47 AM, Tom Lane wrote: > Magnus Hagander writes: >> There was also that (old) thread about making the recovery.conf parameters >> be general GUCs. I don't actually remember the consensus there, but diong >> that would certainly change how it's handled as well. > > It strikes me that keeping a password embedded in the conninfo from being > exposed might be quite a bit harder/riskier if it became a GUC. Something > to keep in mind if we ever try to make that change ... Exposing it in memory for a long time is an issue even if we have a new GUC-flag to obfuscate the value in some cases.. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
Magnus Hagander writes: > There was also that (old) thread about making the recovery.conf parameters > be general GUCs. I don't actually remember the consensus there, but diong > that would certainly change how it's handled as well. It strikes me that keeping a password embedded in the conninfo from being exposed might be quite a bit harder/riskier if it became a GUC. Something to keep in mind if we ever try to make that change ... regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
On Wed, Jun 29, 2016 at 11:18 PM, Michael Paquier wrote: > On Thu, Jun 30, 2016 at 6:01 AM, Alvaro Herrera > wrote: > > Alvaro Herrera wrote: > > > >> I propose to push this patch, closing the open item, and you can rework > >> on top -- I suppose you would completely remove the original conninfo > >> from shared memory and instead only copy the obfuscated version there > >> (and probably also remove the ready_to_display flag). I think we'd need > >> to see the patch before deciding whether we want it in 9.6 or not, > >> keeping in mind that having the conninfo in shared memory is a > >> pre-existing problem, unrelated to the pgstats view new in 9.6. > > > > Pushed this. Feel free to tinker further with it, if you feel the need > > to. > > > > Regarding backpatching the clearing of shared memory, I'm inclined not > > to. If there is a real security concern there (I'm unsure what attack > > are we protecting against), it may be better fixed by the approach > > suggested by Fujii whereby the sensitive info is not ever published in > > shared memory. > > Yes, this is not going to be pretty invasive anyway. The cleanest way > to handle things here would be to refactor a bit xlog.c > (xlogparams.c?) so as readRecoveryCommandFile is exposed in its own > file, and the recovery parameters are handled in a single structure, > which is the return result of the call. To reduce a bit the cruft in > xlog.c that would be nice anyway I guess. > There was also that (old) thread about making the recovery.conf parameters be general GUCs. I don't actually remember the consensus there, but diong that would certainly change how it's handled as well. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
On Thu, Jun 30, 2016 at 6:01 AM, Alvaro Herrera wrote: > Alvaro Herrera wrote: > >> I propose to push this patch, closing the open item, and you can rework >> on top -- I suppose you would completely remove the original conninfo >> from shared memory and instead only copy the obfuscated version there >> (and probably also remove the ready_to_display flag). I think we'd need >> to see the patch before deciding whether we want it in 9.6 or not, >> keeping in mind that having the conninfo in shared memory is a >> pre-existing problem, unrelated to the pgstats view new in 9.6. > > Pushed this. Feel free to tinker further with it, if you feel the need > to. > > Regarding backpatching the clearing of shared memory, I'm inclined not > to. If there is a real security concern there (I'm unsure what attack > are we protecting against), it may be better fixed by the approach > suggested by Fujii whereby the sensitive info is not ever published in > shared memory. Yes, this is not going to be pretty invasive anyway. The cleanest way to handle things here would be to refactor a bit xlog.c (xlogparams.c?) so as readRecoveryCommandFile is exposed in its own file, and the recovery parameters are handled in a single structure, which is the return result of the call. To reduce a bit the cruft in xlog.c that would be nice anyway I guess. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
Alvaro Herrera wrote: > I propose to push this patch, closing the open item, and you can rework > on top -- I suppose you would completely remove the original conninfo > from shared memory and instead only copy the obfuscated version there > (and probably also remove the ready_to_display flag). I think we'd need > to see the patch before deciding whether we want it in 9.6 or not, > keeping in mind that having the conninfo in shared memory is a > pre-existing problem, unrelated to the pgstats view new in 9.6. Pushed this. Feel free to tinker further with it, if you feel the need to. Regarding backpatching the clearing of shared memory, I'm inclined not to. If there is a real security concern there (I'm unsure what attack are we protecting against), it may be better fixed by the approach suggested by Fujii whereby the sensitive info is not ever published in shared memory. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
Fujii Masao wrote:
> On Thu, Jun 30, 2016 at 2:50 AM, Alvaro Herrera
> wrote:
> > Fujii Masao wrote:
> >> On Wed, Jun 29, 2016 at 12:23 PM, Alvaro Herrera
> >> wrote:
> >> > Michael Paquier wrote:
> >> >> On Wed, Jun 29, 2016 at 6:42 AM, Alvaro Herrera
> >> >> wrote:
> >> >
> >> >> > I have already edited the patch following some of these ideas. Will
> >> >> > post a new version later.
> >> >>
> >> >> Cool, thanks.
> >> >
> >> > Here it is. I found it was annoying to maintain the function return
> >> > tupdesc in two places (pg_proc.h and the function code itself), so I
> >> > changed that too.
> >>
> >> ISTM that pg_stat_wal_receiver can return the security-sensitive fields
> >> if it's viewed before walreceiver overwrites the conninfo in the shared
> >> memory
> >> with the obfuscated one.
> >
> > Hmm, ouch. Maybe we can set a flag once the conninfo has been
> > obfuscated, and put the function to sleep until the flag is set.
>
> Or what about making walreceiver instead of startup process read
> primary_conninfo from the file?
Yeah, that sounds smart. I'm not sure it's a good fit for 9.6; what I
propose can be implemented in 10 lines, attached (wherein I also adopted
Michael's suggestion to get rid of the extra whitespace)
I propose to push this patch, closing the open item, and you can rework
on top -- I suppose you would completely remove the original conninfo
from shared memory and instead only copy the obfuscated version there
(and probably also remove the ready_to_display flag). I think we'd need
to see the patch before deciding whether we want it in 9.6 or not,
keeping in mind that having the conninfo in shared memory is a
pre-existing problem, unrelated to the pgstats view new in 9.6.
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index bd7bb77..a8b8bb0 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -1302,6 +1302,14 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
text
Replication slot name used by this WAL receiver
+
+ conn_info
+ text
+
+ Connection string used by this WAL receiver,
+ with security-sensitive fields obfuscated.
+
+
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 272c02f..f52de3a 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -681,7 +681,8 @@ CREATE VIEW pg_stat_wal_receiver AS
s.last_msg_receipt_time,
s.latest_end_lsn,
s.latest_end_time,
-s.slot_name
+s.slot_name,
+s.conn_info
FROM pg_stat_get_wal_receiver() s
WHERE s.pid IS NOT NULL;
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index b61e39d..45dccb3 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -20,6 +20,7 @@
#include
#include "libpq-fe.h"
+#include "pqexpbuffer.h"
#include "access/xlog.h"
#include "miscadmin.h"
#include "replication/walreceiver.h"
@@ -47,6 +48,7 @@ static char *recvBuf = NULL;
/* Prototypes for interface functions */
static void libpqrcv_connect(char *conninfo);
+static char *libpqrcv_get_conninfo(void);
static void libpqrcv_identify_system(TimeLineID *primary_tli);
static void libpqrcv_readtimelinehistoryfile(TimeLineID tli, char **filename, char **content, int *len);
static bool libpqrcv_startstreaming(TimeLineID tli, XLogRecPtr startpoint,
@@ -74,6 +76,7 @@ _PG_init(void)
walrcv_disconnect != NULL)
elog(ERROR, "libpqwalreceiver already loaded");
walrcv_connect = libpqrcv_connect;
+ walrcv_get_conninfo = libpqrcv_get_conninfo;
walrcv_identify_system = libpqrcv_identify_system;
walrcv_readtimelinehistoryfile = libpqrcv_readtimelinehistoryfile;
walrcv_startstreaming = libpqrcv_startstreaming;
@@ -118,6 +121,55 @@ libpqrcv_connect(char *conninfo)
}
/*
+ * Return a user-displayable conninfo string. Any security-sensitive fields
+ * are obfuscated.
+ */
+static char *
+libpqrcv_get_conninfo(void)
+{
+ PQconninfoOption *conn_opts;
+ PQconninfoOption *conn_opt;
+ PQExpBufferData buf;
+ char *retval;
+
+ Assert(streamConn != NULL);
+
+ initPQExpBuffer(&buf);
+ conn_opts = PQconninfo(streamConn);
+
+ if (conn_opts == NULL)
+ ereport(ERROR,
+(errmsg("could not parse connection string: %s",
+ _("out of memory";
+
+ /* build a clean connection string from pieces */
+ for (conn_opt = conn_opts; conn_opt->keyword != NULL; conn_opt++)
+ {
+ bool obfuscate;
+
+ /* Skip debug and empty options */
+ if (strchr(conn_opt->dispchar, 'D') ||
+ conn_opt->val == NULL ||
+ conn_opt->val[0] == '\0')
+ continue;
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
On Thu, Jun 30, 2016 at 2:50 AM, Alvaro Herrera wrote: > Fujii Masao wrote: >> On Wed, Jun 29, 2016 at 12:23 PM, Alvaro Herrera >> wrote: >> > Michael Paquier wrote: >> >> On Wed, Jun 29, 2016 at 6:42 AM, Alvaro Herrera >> >> wrote: >> > >> >> > I have already edited the patch following some of these ideas. Will >> >> > post a new version later. >> >> >> >> Cool, thanks. >> > >> > Here it is. I found it was annoying to maintain the function return >> > tupdesc in two places (pg_proc.h and the function code itself), so I >> > changed that too. >> >> ISTM that pg_stat_wal_receiver can return the security-sensitive fields >> if it's viewed before walreceiver overwrites the conninfo in the shared >> memory >> with the obfuscated one. > > Hmm, ouch. Maybe we can set a flag once the conninfo has been > obfuscated, and put the function to sleep until the flag is set. Or what about making walreceiver instead of startup process read primary_conninfo from the file? Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
Fujii Masao wrote: > On Wed, Jun 29, 2016 at 12:23 PM, Alvaro Herrera > wrote: > > Michael Paquier wrote: > >> On Wed, Jun 29, 2016 at 6:42 AM, Alvaro Herrera > >> wrote: > > > >> > I have already edited the patch following some of these ideas. Will > >> > post a new version later. > >> > >> Cool, thanks. > > > > Here it is. I found it was annoying to maintain the function return > > tupdesc in two places (pg_proc.h and the function code itself), so I > > changed that too. > > ISTM that pg_stat_wal_receiver can return the security-sensitive fields > if it's viewed before walreceiver overwrites the conninfo in the shared memory > with the obfuscated one. Hmm, ouch. Maybe we can set a flag once the conninfo has been obfuscated, and put the function to sleep until the flag is set. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
On Wed, Jun 29, 2016 at 12:23 PM, Alvaro Herrera wrote: > Michael Paquier wrote: >> On Wed, Jun 29, 2016 at 6:42 AM, Alvaro Herrera >> wrote: > >> > I have already edited the patch following some of these ideas. Will >> > post a new version later. >> >> Cool, thanks. > > Here it is. I found it was annoying to maintain the function return > tupdesc in two places (pg_proc.h and the function code itself), so I > changed that too. ISTM that pg_stat_wal_receiver can return the security-sensitive fields if it's viewed before walreceiver overwrites the conninfo in the shared memory with the obfuscated one. Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
On Wed, Jun 29, 2016 at 12:23 PM, Alvaro Herrera wrote: > Michael Paquier wrote: >> On Wed, Jun 29, 2016 at 6:42 AM, Alvaro Herrera >> wrote: > >> > I have already edited the patch following some of these ideas. Will >> > post a new version later. >> >> Cool, thanks. > > Here it is. I found it was annoying to maintain the function return > tupdesc in two places (pg_proc.h and the function code itself), so I > changed that too. This looks globally fine to me. Good catch to handle NULL results of walrcv_get_conninfo. + appendPQExpBuffer(&buf, "%s=%s ", + conn_opt->keyword, + obfuscate ? "" : conn_opt->val) This would add an extra space at the end of the string unconditionally. What about checking if buf->len == 0, then fill buf with "%s=%s", and " %s=%s" otherwise? Do we want to do something for back-branches regarding the presence of the connection string in shared memory? The only invasive point is the addition of the interface routine to get back the obfuscated connection string from libpqwalreceiver. That's a private interface in the backend, but perhaps it would be a problem to change that in a minor release? -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
Michael Paquier wrote:
> On Wed, Jun 29, 2016 at 6:42 AM, Alvaro Herrera
> wrote:
> > I have already edited the patch following some of these ideas. Will
> > post a new version later.
>
> Cool, thanks.
Here it is. I found it was annoying to maintain the function return
tupdesc in two places (pg_proc.h and the function code itself), so I
changed that too.
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index bd7bb77..a8b8bb0 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -1302,6 +1302,14 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
text
Replication slot name used by this WAL receiver
+
+ conn_info
+ text
+
+ Connection string used by this WAL receiver,
+ with security-sensitive fields obfuscated.
+
+
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 272c02f..f52de3a 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -681,7 +681,8 @@ CREATE VIEW pg_stat_wal_receiver AS
s.last_msg_receipt_time,
s.latest_end_lsn,
s.latest_end_time,
-s.slot_name
+s.slot_name,
+s.conn_info
FROM pg_stat_get_wal_receiver() s
WHERE s.pid IS NOT NULL;
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index b61e39d..2a10c56 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -20,6 +20,7 @@
#include
#include "libpq-fe.h"
+#include "pqexpbuffer.h"
#include "access/xlog.h"
#include "miscadmin.h"
#include "replication/walreceiver.h"
@@ -47,6 +48,7 @@ static char *recvBuf = NULL;
/* Prototypes for interface functions */
static void libpqrcv_connect(char *conninfo);
+static char *libpqrcv_get_conninfo(void);
static void libpqrcv_identify_system(TimeLineID *primary_tli);
static void libpqrcv_readtimelinehistoryfile(TimeLineID tli, char **filename, char **content, int *len);
static bool libpqrcv_startstreaming(TimeLineID tli, XLogRecPtr startpoint,
@@ -74,6 +76,7 @@ _PG_init(void)
walrcv_disconnect != NULL)
elog(ERROR, "libpqwalreceiver already loaded");
walrcv_connect = libpqrcv_connect;
+ walrcv_get_conninfo = libpqrcv_get_conninfo;
walrcv_identify_system = libpqrcv_identify_system;
walrcv_readtimelinehistoryfile = libpqrcv_readtimelinehistoryfile;
walrcv_startstreaming = libpqrcv_startstreaming;
@@ -118,6 +121,54 @@ libpqrcv_connect(char *conninfo)
}
/*
+ * Return a user-displayable conninfo string. Any security-sensitive fields
+ * are obfuscated.
+ */
+static char *
+libpqrcv_get_conninfo(void)
+{
+ PQconninfoOption *conn_opts;
+ PQconninfoOption *conn_opt;
+ PQExpBufferData buf;
+ char *retval;
+
+ Assert(streamConn != NULL);
+
+ initPQExpBuffer(&buf);
+ conn_opts = PQconninfo(streamConn);
+
+ if (conn_opts == NULL)
+ ereport(ERROR,
+(errmsg("could not parse connection string: %s",
+ _("out of memory";
+
+ /* build a clean connection string from pieces */
+ for (conn_opt = conn_opts; conn_opt->keyword != NULL; conn_opt++)
+ {
+ bool obfuscate;
+
+ /* Skip debug and empty options */
+ if (strchr(conn_opt->dispchar, 'D') ||
+ conn_opt->val == NULL ||
+ conn_opt->val[0] == '\0')
+ continue;
+
+ /* Obfuscate security-sensitive options */
+ obfuscate = strchr(conn_opt->dispchar, '*') != NULL;
+
+ appendPQExpBuffer(&buf, "%s=%s ",
+ conn_opt->keyword,
+ obfuscate ? "" : conn_opt->val);
+ }
+
+ PQconninfoFree(conn_opts);
+
+ retval = PQExpBufferDataBroken(buf) ? NULL : pstrdup(buf.data);
+ termPQExpBuffer(&buf);
+ return retval;
+}
+
+/*
* Check that primary's system identifier matches ours, and fetch the current
* timeline ID of the primary.
*/
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index ce311cb..04569c2 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -75,6 +75,7 @@ bool hot_standby_feedback;
/* libpqreceiver hooks to these when loaded */
walrcv_connect_type walrcv_connect = NULL;
+walrcv_get_conninfo_type walrcv_get_conninfo = NULL;
walrcv_identify_system_type walrcv_identify_system = NULL;
walrcv_startstreaming_type walrcv_startstreaming = NULL;
walrcv_endstreaming_type walrcv_endstreaming = NULL;
@@ -192,6 +193,7 @@ void
WalReceiverMain(void)
{
char conninfo[MAXCONNINFO];
+ char *tmp_conninfo;
char slotname[NAMEDATALEN];
XLogRecPtr startpoint;
TimeLineID startpointTLI;
@@ -282,7 +284,9 @@ WalReceiverMain(void)
/* Load the libpq-specific functions */
load_file("libpqwalreceiver
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
On Wed, Jun 29, 2016 at 6:42 AM, Alvaro Herrera wrote: > Michael Paquier wrote: > >> I have been thinking more about that, and came up with the following >> idea... We do not want to link libpq directly to the server, so let's >> add a new routine to libpqwalreceiver that builds an obfuscated >> connection string and let's have walreceiver.c save it in shared >> memory. Then pg_stat_wal_receiver just makes use of this string. This >> results in a rather light patch, proposed as attached. Connection URIs >> get as well translated as connection strings via PQconninfo(), then >> the new interface routine of libpqwalreceiver looks at dispchar to >> determine if it should dump a field or not and obfuscates it with more >> or less ''. > > Seems a reasonable idea to me, but some details seem a bit strange: > > * Why obfuscate debug options instead of skipping them? Those are hidden in postgres_fdw/ and 'D' marks options only used for debugging purposes or options that should not be shown. That7s why I did so. > * why not use PQExpBuffer? Yes, that would be better. > * Why have the return param be an output argument instead of a plain > return value? i.e. static char *libpqrcv_get_conninfo(void). Oh, yes. That's something I forgot to change. We cannot be completely sure that the connstr will fit in MAXCONNINFO, so it makes little sense to store the result in a pre-allocated string. > On the security aspect of "conninfo" itself, which persists in shared > memory: do we absolutely need to keep that data? In my reading of the > code, it's only used once to establish the initial connection to the > walsender, and then never afterwards. We could remove the disclosure by > the simple expedient of overwriting that struct member with the > obfuscated one, right after establishing that connection. Then we don't > need an additional struct member safe_conninfo. Is there a reason why > this wouldn't work? [Wait a minute...] I don't see why that would not work. By reading the code we do not reattempt a connection, and leave WalReceiverMain if there is a disconnection. > I have already edited the patch following some of these ideas. Will > post a new version later. Cool, thanks. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
Michael Paquier wrote: > I have been thinking more about that, and came up with the following > idea... We do not want to link libpq directly to the server, so let's > add a new routine to libpqwalreceiver that builds an obfuscated > connection string and let's have walreceiver.c save it in shared > memory. Then pg_stat_wal_receiver just makes use of this string. This > results in a rather light patch, proposed as attached. Connection URIs > get as well translated as connection strings via PQconninfo(), then > the new interface routine of libpqwalreceiver looks at dispchar to > determine if it should dump a field or not and obfuscates it with more > or less ''. Seems a reasonable idea to me, but some details seem a bit strange: * Why obfuscate debug options instead of skipping them? * why not use PQExpBuffer? * Why have the return param be an output argument instead of a plain return value? i.e. static char *libpqrcv_get_conninfo(void). On the security aspect of "conninfo" itself, which persists in shared memory: do we absolutely need to keep that data? In my reading of the code, it's only used once to establish the initial connection to the walsender, and then never afterwards. We could remove the disclosure by the simple expedient of overwriting that struct member with the obfuscated one, right after establishing that connection. Then we don't need an additional struct member safe_conninfo. Is there a reason why this wouldn't work? I have already edited the patch following some of these ideas. Will post a new version later. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
Stephen Frost wrote: > * Tom Lane (t...@sss.pgh.pa.us) wrote: > > Michael Paquier writes: > > > On Tue, Jun 21, 2016 at 11:29 AM, Tom Lane wrote: > > >> What I would want to know is whether this specific change is actually a > > >> good idea. In particular, I'm concerned about the possible security > > >> implications of exposing primary_conninfo --- might it not contain a > > >> password, for example? > > > > > Yes it could, as a connection string, but we make the information of > > > this view only visible to superusers. For the others, that's just > > > NULL. > > > > Well, that's okay for now, but I'm curious to hear Stephen Frost's > > opinion on this. He's been on the warpath to decrease our dependence > > on superuser-ness for protection purposes. Seems to me that having > > one column in this view that is a lot more security-sensitive than > > the others is likely to be an issue someday. > > Ugh. I would certainly rather not have yet another special, hard-coded, > bit of logic that magically makes things available to a superuser when > it's not available to regular users. > > What that results in is the need to have a new default role to control > access to that column for the non-superuser case. FWIW we already have a superuser() check for the walsender stats view since 9.1 -- see commit f88a6381. To appease this we could create our second predefined role that controls access to both pg_stat_get_wal_senders and pg_stat_get_wal_receiver. I don't think my commit in 9.6 creates this problem, only exacerbates a pre-existing one, but I also think it's fair to fix both cases for 9.6. Not sure what to name the new predefined role though -- pg_wal_stats_reader? (I don't suppose we want to create it to cover *any* future privileged stats reads rather than just those WAL related, do we?) > As for the password showing up, sorry, but we need a solution for *that* > regardless of the rest- the password shouldn't be exposed to anyone, nor > should it be sent and kept in the backend's memory for longer than > necessary. I'm not suggesting we've got that figured out already, but > that's where we should be trying to go. I suppose Michael's proposed patch to copy the conninfo obscuring the password should be enough for this, but I'll go have a closer look. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
Noah Misch wrote: > On Sun, Jun 19, 2016 at 05:56:12PM +0900, Michael Paquier wrote: > > The new pg_stat_wal_receiver does not include primary_conninfo. > > Looking at that now, it looks almost stupid not to include it... > > Adding it now would require a catalog bump, so I am not sure if this > > is acceptable at this stage for 9.6... > > There is no value in avoiding catversion bumps at this time. I'm looking at this problem now and will report back by Wed 29th EOB. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
On Sun, Jun 19, 2016 at 05:56:12PM +0900, Michael Paquier wrote: > The new pg_stat_wal_receiver does not include primary_conninfo. > Looking at that now, it looks almost stupid not to include it... > Adding it now would require a catalog bump, so I am not sure if this > is acceptable at this stage for 9.6... There is no value in avoiding catversion bumps at this time. [Action required within 72 hours. This is a generic notification.] The above-described topic is currently a PostgreSQL 9.6 open item. Alvaro, since you committed the patch believed to have created it, you own this open item. If some other commit is more relevant or if this does not belong as a 9.6 open item, please let us know. Otherwise, please observe the policy on open item ownership[1] and send a status update within 72 hours of this message. Include a date for your subsequent status update. Testers may discover new open items at any time, and I want to plan to get them all fixed well in advance of shipping 9.6rc1. Consequently, I will appreciate your efforts toward speedy resolution. Thanks. [1] http://www.postgresql.org/message-id/20160527025039.ga447...@tornado.leadboat.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
On Wed, Jun 22, 2016 at 10:51 AM, Michael Paquier wrote: > This connection string is stored in shared memory in WalRcvData, and > that's the case for a couple of releases now, so it has already a high > footprint, though I agree that making that available at SQL level > makes it even worse. Looking at the code, as libpq does not link > directly to libpqcommon, I think that the cleanest solution is to do > something similar to wchar.c, aka split the parsing, deparsing > routines that we are going to use in a file located in src/backend/, > and then build libpq using it. Writing a patch for that would not be > that complicated. What is stored in WalRcvData is then the connection > string filtered of its password field, or we could just obfuscate it > with ***. It may still be useful to the user to know that a password > has been used. I have been thinking more about that, and came up with the following idea... We do not want to link libpq directly to the server, so let's add a new routine to libpqwalreceiver that builds an obfuscated connection string and let's have walreceiver.c save it in shared memory. Then pg_stat_wal_receiver just makes use of this string. This results in a rather light patch, proposed as attached. Connection URIs get as well translated as connection strings via PQconninfo(), then the new interface routine of libpqwalreceiver looks at dispchar to determine if it should dump a field or not and obfuscates it with more or less ''. Thoughts? -- Michael wal-receiver-conninfo-v3.patch Description: invalid/octet-stream -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
On Wed, Jun 22, 2016 at 12:04 AM, Stephen Frost wrote: > Ugh. I would certainly rather not have yet another special, hard-coded, > bit of logic that magically makes things available to a superuser when > it's not available to regular users. > What that results in is the need to have a new default role to control > access to that column for the non-superuser case. OK, we could always update system_views.sql to revoke all rights from public.. This ship has not sailed yet. > As for the password showing up, sorry, but we need a solution for *that* > regardless of the rest- the password shouldn't be exposed to anyone, nor > should it be sent and kept in the backend's memory for longer than > necessary. I'm not suggesting we've got that figured out already, but > that's where we should be trying to go. This connection string is stored in shared memory in WalRcvData, and that's the case for a couple of releases now, so it has already a high footprint, though I agree that making that available at SQL level makes it even worse. Looking at the code, as libpq does not link directly to libpqcommon, I think that the cleanest solution is to do something similar to wchar.c, aka split the parsing, deparsing routines that we are going to use in a file located in src/backend/, and then build libpq using it. Writing a patch for that would not be that complicated. What is stored in WalRcvData is then the connection string filtered of its password field, or we could just obfuscate it with ***. It may still be useful to the user to know that a password has been used. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
* Tom Lane (t...@sss.pgh.pa.us) wrote: > Michael Paquier writes: > > On Tue, Jun 21, 2016 at 11:29 AM, Tom Lane wrote: > >> What I would want to know is whether this specific change is actually a > >> good idea. In particular, I'm concerned about the possible security > >> implications of exposing primary_conninfo --- might it not contain a > >> password, for example? > > > Yes it could, as a connection string, but we make the information of > > this view only visible to superusers. For the others, that's just > > NULL. > > Well, that's okay for now, but I'm curious to hear Stephen Frost's > opinion on this. He's been on the warpath to decrease our dependence > on superuser-ness for protection purposes. Seems to me that having > one column in this view that is a lot more security-sensitive than > the others is likely to be an issue someday. Ugh. I would certainly rather not have yet another special, hard-coded, bit of logic that magically makes things available to a superuser when it's not available to regular users. What that results in is the need to have a new default role to control access to that column for the non-superuser case. As for the password showing up, sorry, but we need a solution for *that* regardless of the rest- the password shouldn't be exposed to anyone, nor should it be sent and kept in the backend's memory for longer than necessary. I'm not suggesting we've got that figured out already, but that's where we should be trying to go. Apologies, I've not followed this thread entirely, so these comments are based only on what's quoted above. I'll try to read the complete thread tomorrow. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
On Tue, Jun 21, 2016 at 12:06 PM, Tom Lane wrote: > Peter Eisentraut writes: >> On 6/20/16 10:29 PM, Tom Lane wrote: >>> What I would want to know is whether this specific change is actually a >>> good idea. In particular, I'm concerned about the possible security >>> implications of exposing primary_conninfo --- might it not contain a >>> password, for example? > >> That would have been my objection. This was also mentioned in the >> context of moving recovery.conf settings to postgresql.conf, because >> then the password would become visible in SHOW commands and the like. > >> Alternatively or additionally, implement a way to strip passwords out of >> conninfo information. libpq already has information about which >> connection items are sensitive. > > Yeah, I'd been wondering whether we could parse the conninfo string into > individual fields and then drop the password field. It's hard to see a > reason why this view needs to show passwords, since presumably everything > in it corresponds to successful connections --- if your password is wrong, > you aren't in it. walreceiver.c does not have a direct dependency to libpq yet, everything does through libpqwalreceiver. So a correct move would be to unplug the low-level routines of PQconninfoParse into something in src/common/, where both the backend and frontend could use it. And then we use it to rebuild a connection string. Thoughts? -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
Peter Eisentraut writes: > On 6/20/16 10:29 PM, Tom Lane wrote: >> What I would want to know is whether this specific change is actually a >> good idea. In particular, I'm concerned about the possible security >> implications of exposing primary_conninfo --- might it not contain a >> password, for example? > That would have been my objection. This was also mentioned in the > context of moving recovery.conf settings to postgresql.conf, because > then the password would become visible in SHOW commands and the like. > Alternatively or additionally, implement a way to strip passwords out of > conninfo information. libpq already has information about which > connection items are sensitive. Yeah, I'd been wondering whether we could parse the conninfo string into individual fields and then drop the password field. It's hard to see a reason why this view needs to show passwords, since presumably everything in it corresponds to successful connections --- if your password is wrong, you aren't in it. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
On 6/20/16 10:29 PM, Tom Lane wrote: What I would want to know is whether this specific change is actually a good idea. In particular, I'm concerned about the possible security implications of exposing primary_conninfo --- might it not contain a password, for example? That would have been my objection. This was also mentioned in the context of moving recovery.conf settings to postgresql.conf, because then the password would become visible in SHOW commands and the like. We would need a way to put the password in a separate place, like a primary_conn_password setting. Yes, you can already use .pgpass for that, but since pg_basebackup -R will happily copy a password out of .pgpass into recovery.conf, this makes accidentally leaking passwords way too easy. Alternatively or additionally, implement a way to strip passwords out of conninfo information. libpq already has information about which connection items are sensitive. Needs more thought, in any case. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
Michael Paquier writes: > On Tue, Jun 21, 2016 at 11:29 AM, Tom Lane wrote: >> What I would want to know is whether this specific change is actually a >> good idea. In particular, I'm concerned about the possible security >> implications of exposing primary_conninfo --- might it not contain a >> password, for example? > Yes it could, as a connection string, but we make the information of > this view only visible to superusers. For the others, that's just > NULL. Well, that's okay for now, but I'm curious to hear Stephen Frost's opinion on this. He's been on the warpath to decrease our dependence on superuser-ness for protection purposes. Seems to me that having one column in this view that is a lot more security-sensitive than the others is likely to be an issue someday. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
On Tue, Jun 21, 2016 at 11:29 AM, Tom Lane wrote: > Michael Paquier writes: >> On Tue, Jun 21, 2016 at 9:58 AM, Tatsuo Ishii wrote: >>> Even there seems to be ongoing discussions on changing version number >>> while in the beta period (and which definitely requires initdb). Why >>> not changing system catalog during beta?:-) > >> I am not directly against that to be honest, but I'd expect Tom's >> wraith showing up soon on this thread just by saying that. In the two >> last releases, catalog bumps before beta2 because there were no other >> choice. This issue is not really critical, just a stupid miss from me, >> and we can live with this mistake as well. > > Since pg_stat_wal_receiver is new in 9.6, it seems to me that it'd be > wise to try to get it right the first time. And it's not like we are > going to get to beta3 without another initdb --- we already know the > partial-aggregate design is broken and needs some more catalog changes. Amen. That's a sufficient argument to slip this one into 9.6 then. > What I would want to know is whether this specific change is actually a > good idea. In particular, I'm concerned about the possible security > implications of exposing primary_conninfo --- might it not contain a > password, for example? Yes it could, as a connection string, but we make the information of this view only visible to superusers. For the others, that's just NULL. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
Michael Paquier writes: > On Tue, Jun 21, 2016 at 9:58 AM, Tatsuo Ishii wrote: >> Even there seems to be ongoing discussions on changing version number >> while in the beta period (and which definitely requires initdb). Why >> not changing system catalog during beta?:-) > I am not directly against that to be honest, but I'd expect Tom's > wraith showing up soon on this thread just by saying that. In the two > last releases, catalog bumps before beta2 because there were no other > choice. This issue is not really critical, just a stupid miss from me, > and we can live with this mistake as well. Since pg_stat_wal_receiver is new in 9.6, it seems to me that it'd be wise to try to get it right the first time. And it's not like we are going to get to beta3 without another initdb --- we already know the partial-aggregate design is broken and needs some more catalog changes. What I would want to know is whether this specific change is actually a good idea. In particular, I'm concerned about the possible security implications of exposing primary_conninfo --- might it not contain a password, for example? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
On Tue, Jun 21, 2016 at 9:58 AM, Tatsuo Ishii wrote: >>> Thanks, this looks good. Could you please add it to the next commitfest >>> so that it doesn't get lost and also so I can do an official review of it? >> >> Yes, I just did that. That's too late for 9.6 anyway with beta2 close by. > > Even there seems to be ongoing discussions on changing version number > while in the beta period (and which definitely requires initdb). Why > not changing system catalog during beta?:-) I am not directly against that to be honest, but I'd expect Tom's wraith showing up soon on this thread just by saying that. In the two last releases, catalog bumps before beta2 because there were no other choice. This issue is not really critical, just a stupid miss from me, and we can live with this mistake as well. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
>> Thanks, this looks good. Could you please add it to the next commitfest >> so that it doesn't get lost and also so I can do an official review of it? > > Yes, I just did that. That's too late for 9.6 anyway with beta2 close by. Even there seems to be ongoing discussions on changing version number while in the beta period (and which definitely requires initdb). Why not changing system catalog during beta?:-) Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
On Mon, Jun 20, 2016 at 11:26 PM, Vik Fearing wrote: > On 19/06/16 13:02, Michael Paquier wrote: >> On Sun, Jun 19, 2016 at 7:38 PM, Vik Fearing wrote: >>> On 19/06/16 12:28, Michael Paquier wrote: On Sun, Jun 19, 2016 at 5:56 PM, Michael Paquier Or in short the attached. >>> >>> The code looks good to me but why no documentation? >> >> Because that's a long day... Thanks! Now you have it. > > Thanks, this looks good. Could you please add it to the next commitfest > so that it doesn't get lost and also so I can do an official review of it? Yes, I just did that. That's too late for 9.6 anyway with beta2 close by. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
On 19/06/16 13:02, Michael Paquier wrote: > On Sun, Jun 19, 2016 at 7:38 PM, Vik Fearing wrote: >> On 19/06/16 12:28, Michael Paquier wrote: >>> On Sun, Jun 19, 2016 at 5:56 PM, Michael Paquier >>> Or in short the attached. >> >> The code looks good to me but why no documentation? > > Because that's a long day... Thanks! Now you have it. Quick bikeshed: I think the column should be called conninfo and not conn_info to match other places it's used. -- Vik Fearing +33 6 46 75 15 36 http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
On 19/06/16 13:02, Michael Paquier wrote: > On Sun, Jun 19, 2016 at 7:38 PM, Vik Fearing wrote: >> On 19/06/16 12:28, Michael Paquier wrote: >>> On Sun, Jun 19, 2016 at 5:56 PM, Michael Paquier >>> Or in short the attached. >> >> The code looks good to me but why no documentation? > > Because that's a long day... Thanks! Now you have it. Thanks, this looks good. Could you please add it to the next commitfest so that it doesn't get lost and also so I can do an official review of it? -- Vik Fearing +33 6 46 75 15 36 http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
On Sun, Jun 19, 2016 at 7:38 PM, Vik Fearing wrote: > On 19/06/16 12:28, Michael Paquier wrote: >> On Sun, Jun 19, 2016 at 5:56 PM, Michael Paquier >> Or in short the attached. > > The code looks good to me but why no documentation? Because that's a long day... Thanks! Now you have it. -- Michael wal-receiver-conninfo-v2.patch Description: invalid/octet-stream -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
On 19/06/16 12:28, Michael Paquier wrote: > On Sun, Jun 19, 2016 at 5:56 PM, Michael Paquier > wrote: >> The new pg_stat_wal_receiver does not include primary_conninfo. >> Looking at that now, it looks almost stupid not to include it... >> Adding it now would require a catalog bump, so I am not sure if this >> is acceptable at this stage for 9.6... This definitely needs to go in, we get regular requests for it. The last one I've seen being https://www.postgresql.org/message-id/CAN1xZseiqNRh1ZE0seVk7UuHeWvFBEWG%2BFcW7Xfm_Nv%3Dd%2BfPGw%40mail.gmail.com Whether it goes into 9.6 or 10 is not for me to say. > Or in short the attached. The code looks good to me but why no documentation? -- Vik Fearing +33 6 46 75 15 36 http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver
On Sun, Jun 19, 2016 at 5:56 PM, Michael Paquier wrote: > The new pg_stat_wal_receiver does not include primary_conninfo. > Looking at that now, it looks almost stupid not to include it... > Adding it now would require a catalog bump, so I am not sure if this > is acceptable at this stage for 9.6... Or in short the attached. -- Michael wal-receiver-conninfo.patch Description: invalid/octet-stream -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] primary_conninfo missing from pg_stat_wal_receiver
Hi all, The new pg_stat_wal_receiver does not include primary_conninfo. Looking at that now, it looks almost stupid not to include it... Adding it now would require a catalog bump, so I am not sure if this is acceptable at this stage for 9.6... Regards, -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
