Re: [HACKERS] [WIP] pg_ping utility

2013-01-20 Thread Robert Haas
On Fri, Jan 18, 2013 at 4:17 PM, Phil Sorber p...@omniti.com wrote:
 Updated patch is rebased against current master and copyright year is updated.

I took a look at this.  According to the documentation for
PQpingParams: It accepts connection parameters identical to those of
PQconnectdbParams, described above. It is not, however, necessary to
supply correct user name, password, or database name values to obtain
the server status.  The -U option therefore seems quite useless
except as a source of user confusion, and -d is only useful in that
you could instead pass a connections string.  That is definitely a
useful thing to be able to do, but calling the option -d for database
might be viewed as confusing.  Or, it might be viewed as consistent
with other commands, if you tilt your head just the right way.

I would be tempted to change the syntax synopsis of the command to
pg_isready [OPTIONS]... [CONNSTR] and delete -d and -U from the list
of options, along the way that psql already works, but making
allowance for the fact that database and username are apparently not
relevant.

I am also a little bit baffled by the loop that begins here:

+   while (conn_opt_ptr-keyword)

It appears to me that what this loop does is iterate through all of
the possible connection options and then, when we get to the host,
port, user, or dbname options, add them to the keywords and values
arrays.  But... what do we get out of iterating through all of the
other options, then?  It seems to me that you could dispense with the
loop and just keep the stuff that actually adds the non-default values
to the arrays:

+   if (pghost != NULL)
+   {
+   keywords[opt_index] = host;
+   values[opt_index] = pghost;
+   opt_index++;
+   }
+   if (pgport != NULL)
+   {
+   keywords[opt_index] = port;
+   values[opt_index] = pgport;
+   opt_index++;
+   }
+   if (pgconnstr != NULL)
+   {
+   keywords[opt_index] = dbname;
+   values[opt_index] = pgconnstr;
+   opt_index++;
+   }

Maybe there's some purpose to this I'm not seeing, but from here the
loop looks like an unnecessary frammish.

Finally, I think there should be a check that the user hasn't supplied
more command-line arguments than we know what to do with, similar to
this:

[rhaas pgsql]$ vacuumdb foo bar baz
vacuumdb: too many command-line arguments (first is bar)
Try vacuumdb --help for more information.

That error message text seems like it might not have been given
sufficient thought, but for purposes of this patch it's probably
better to copy it than to invent something new.

-- 
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] [WIP] pg_ping utility

2013-01-20 Thread Phil Sorber
On Sun, Jan 20, 2013 at 8:40 AM, Robert Haas robertmh...@gmail.com wrote:
 On Fri, Jan 18, 2013 at 4:17 PM, Phil Sorber p...@omniti.com wrote:
 Updated patch is rebased against current master and copyright year is 
 updated.

 I took a look at this.  According to the documentation for
 PQpingParams: It accepts connection parameters identical to those of
 PQconnectdbParams, described above. It is not, however, necessary to
 supply correct user name, password, or database name values to obtain
 the server status.  The -U option therefore seems quite useless
 except as a source of user confusion, and -d is only useful in that
 you could instead pass a connections string.  That is definitely a
 useful thing to be able to do, but calling the option -d for database
 might be viewed as confusing.  Or, it might be viewed as consistent
 with other commands, if you tilt your head just the right way.

 I would be tempted to change the syntax synopsis of the command to
 pg_isready [OPTIONS]... [CONNSTR] and delete -d and -U from the list
 of options, along the way that psql already works, but making
 allowance for the fact that database and username are apparently not
 relevant.


This was done to silence useless error messages in the logs. If you
attempt to connect as some user that does not exist, or to some
database that does not exist, it throws an error in the logs, even
with PQping. You could fix it with env vars, but since the point is to
change the user/database that we were connecting as, I figured it
should be consistent with all the other methods to do that.

 I am also a little bit baffled by the loop that begins here:

 +   while (conn_opt_ptr-keyword)

 It appears to me that what this loop does is iterate through all of
 the possible connection options and then, when we get to the host,
 port, user, or dbname options, add them to the keywords and values
 arrays.  But... what do we get out of iterating through all of the
 other options, then?  It seems to me that you could dispense with the
 loop and just keep the stuff that actually adds the non-default values
 to the arrays:

 +   if (pghost != NULL)
 +   {
 +   keywords[opt_index] = host;
 +   values[opt_index] = pghost;
 +   opt_index++;
 +   }
 +   if (pgport != NULL)
 +   {
 +   keywords[opt_index] = port;
 +   values[opt_index] = pgport;
 +   opt_index++;
 +   }
 +   if (pgconnstr != NULL)
 +   {
 +   keywords[opt_index] = dbname;
 +   values[opt_index] = pgconnstr;
 +   opt_index++;
 +   }

 Maybe there's some purpose to this I'm not seeing, but from here the
 loop looks like an unnecessary frammish.

I use this to find the defaults if they don't pass anything in, so I
know what to put in the status message at the end. I could devise my
own way to come up with those values as I have seen in some other
code, but I thought it was better to ask libpq directly what defaults
it was going to use.


 Finally, I think there should be a check that the user hasn't supplied
 more command-line arguments than we know what to do with, similar to
 this:

 [rhaas pgsql]$ vacuumdb foo bar baz
 vacuumdb: too many command-line arguments (first is bar)
 Try vacuumdb --help for more information.

 That error message text seems like it might not have been given
 sufficient thought, but for purposes of this patch it's probably
 better to copy it than to invent something new.


I had not considered this. I will take a look and provide an updated patch.

 --
 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] [WIP] pg_ping utility

2013-01-20 Thread Robert Haas
On Sun, Jan 20, 2013 at 9:58 AM, Phil Sorber p...@omniti.com wrote:
 This was done to silence useless error messages in the logs. If you
 attempt to connect as some user that does not exist, or to some
 database that does not exist, it throws an error in the logs, even
 with PQping. You could fix it with env vars, but since the point is to
 change the user/database that we were connecting as, I figured it
 should be consistent with all the other methods to do that.

Uh, OK.  Well, in that case, I'm inclined to think that a
documentation mention is in order, and perhaps an update to the
PQpingParams documentation as well.  Because that's hardly obvious.
:-(

 I use this to find the defaults if they don't pass anything in, so I
 know what to put in the status message at the end. I could devise my
 own way to come up with those values as I have seen in some other
 code, but I thought it was better to ask libpq directly what defaults
 it was going to use.

Oh, I see.  Is it really important to have the host and port in the
output, or should we trim that down to just e.g. accepting
connections?  It seems useful to have that if a human is looking at
the output, but maybe not if a machine is looking at the output.  And
if somebody doesn't want it, getting rid of it with sed or awk is
nontrivial - imagine:

pg_isready -d /tmp:5432 - accepting connections

 I had not considered this. I will take a look and provide an updated patch.

Sounds good.

-- 
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] [WIP] pg_ping utility

2013-01-20 Thread Phil Sorber
On Sun, Jan 20, 2013 at 11:38 AM, Robert Haas robertmh...@gmail.com wrote:
 On Sun, Jan 20, 2013 at 9:58 AM, Phil Sorber p...@omniti.com wrote:
 This was done to silence useless error messages in the logs. If you
 attempt to connect as some user that does not exist, or to some
 database that does not exist, it throws an error in the logs, even
 with PQping. You could fix it with env vars, but since the point is to
 change the user/database that we were connecting as, I figured it
 should be consistent with all the other methods to do that.

 Uh, OK.  Well, in that case, I'm inclined to think that a
 documentation mention is in order, and perhaps an update to the
 PQpingParams documentation as well.  Because that's hardly obvious.
 :-(


Ok. I can add something to the notes section of the docs. I can also
add some code comments for this and for grabbing the default params.

 I use this to find the defaults if they don't pass anything in, so I
 know what to put in the status message at the end. I could devise my
 own way to come up with those values as I have seen in some other
 code, but I thought it was better to ask libpq directly what defaults
 it was going to use.

 Oh, I see.  Is it really important to have the host and port in the
 output, or should we trim that down to just e.g. accepting
 connections?  It seems useful to have that if a human is looking at
 the output, but maybe not if a machine is looking at the output.  And
 if somebody doesn't want it, getting rid of it with sed or awk is
 nontrivial - imagine:

 pg_isready -d /tmp:5432 - accepting connections


If you are scripting I'd assume you would use the return code value.
It might be reasonable to make adding the host and port the verbose
method and have just accepting connections as the default output,
but my concern there is a misdiagnosis because someone doesn't realize
what server they are connecting to. This way they can't miss it and
they don't have to add another command line option to get that output.

The other thing I thought about when you mentioned this is not doing
the default lookups if it's in quiet mode. I could move things around
to accomplish this, but not sure it is worth the effort and
complexity. Thoughts?

 I had not considered this. I will take a look and provide an updated patch.

 Sounds good.

 --
 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] [WIP] pg_ping utility

2013-01-20 Thread Robert Haas
On Sun, Jan 20, 2013 at 2:59 PM, Phil Sorber p...@omniti.com wrote:
 Ok. I can add something to the notes section of the docs. I can also
 add some code comments for this and for grabbing the default params.

Sounds good.

 Oh, I see.  Is it really important to have the host and port in the
 output, or should we trim that down to just e.g. accepting
 connections?  It seems useful to have that if a human is looking at
 the output, but maybe not if a machine is looking at the output.  And
 if somebody doesn't want it, getting rid of it with sed or awk is
 nontrivial - imagine:

 pg_isready -d /tmp:5432 - accepting connections

 If you are scripting I'd assume you would use the return code value.
 It might be reasonable to make adding the host and port the verbose
 method and have just accepting connections as the default output,
 but my concern there is a misdiagnosis because someone doesn't realize
 what server they are connecting to. This way they can't miss it and
 they don't have to add another command line option to get that output.

It's a fair concern.  Does anyone else have an opinion on this?

 The other thing I thought about when you mentioned this is not doing
 the default lookups if it's in quiet mode. I could move things around
 to accomplish this, but not sure it is worth the effort and
 complexity. Thoughts?

That doesn't seem to buy us anything.  I'm fine with the code, now
that I see what it's intended to do.  It doesn't cost anything
noticeable in terms of efficiency; I think, I just didn't want to make
things complicated without a reason.

-- 
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] [WIP] pg_ping utility

2013-01-20 Thread Craig Ringer
On 01/21/2013 11:26 AM, Robert Haas wrote:
 On Sun, Jan 20, 2013 at 2:59 PM, Phil Sorber p...@omniti.com wrote:
 Ok. I can add something to the notes section of the docs. I can also
 add some code comments for this and for grabbing the default params.
 Sounds good.

 Oh, I see.  Is it really important to have the host and port in the
 output, or should we trim that down to just e.g. accepting
 connections?  It seems useful to have that if a human is looking at
 the output, but maybe not if a machine is looking at the output.  And
 if somebody doesn't want it, getting rid of it with sed or awk is
 nontrivial - imagine:

 pg_isready -d /tmp:5432 - accepting connections
 If you are scripting I'd assume you would use the return code value.
 It might be reasonable to make adding the host and port the verbose
 method and have just accepting connections as the default output,
 but my concern there is a misdiagnosis because someone doesn't realize
 what server they are connecting to. This way they can't miss it and
 they don't have to add another command line option to get that output.
 It's a fair concern.  Does anyone else have an opinion on this?
I have a strong view that the host and port *should* be printed in output.

One of the most common issues on Stack Overflow questions from new users
is with I can't connect problems that turn out to be them connecting
to the wrong host, port, or socket path.

It is absolutely vital that the unix socket path being used be printed
if unix socket connections are tested, as Mac OS X's insane setup means
that PostgreSQL tool builds on that platform regularly use the system
libpq not the user-installed PostgreSQL's libpq, and it defaults to a
different socket path. If users use pg_ping to verify that their
PostgreSQL instance is running it'll use the user-installed libpq -
fine, that's what we want. But the user will then wonder why the heck
Ruby on Rails's `pg' gem reports it can't connect to the unix socket.
They can't compare the unix socket paths printed in the error message if
pg_ping doesn't show it.

I would like to see pg_ping produce a similar error to psql on
connection failure.

$ psql -p 
psql: could not connect to server: No such file or directory
Is the server running locally and accepting
connections on Unix domain socket /tmp/.s.PGSQL.?

$ psql -h localhost -p 
psql: could not connect to server: Connection refused
Is the server running on host localhost (::1) and accepting
TCP/IP connections on port ?
could not connect to server: Connection refused
Is the server running on host localhost (127.0.0.1) and accepting
TCP/IP connections on port ?

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



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [WIP] pg_ping utility

2013-01-18 Thread Phil Sorber
On Tue, Dec 25, 2012 at 1:47 AM, Michael Paquier
michael.paqu...@gmail.com wrote:


 On Mon, Dec 24, 2012 at 12:44 AM, Phil Sorber p...@omniti.com wrote:

 Updated patch attached.

 Thanks. I am marking this patch as ready for committer.

 --
 Michael Paquier
 http://michael.otacoo.com

Updated patch is rebased against current master and copyright year is updated.


pg_isready_bin_v10.diff
Description: Binary data


pg_isready_docs_v10.diff
Description: Binary data

-- 
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] [WIP] pg_ping utility

2012-12-24 Thread Michael Paquier
On Mon, Dec 24, 2012 at 12:44 AM, Phil Sorber p...@omniti.com wrote:

 Updated patch attached.

Thanks. I am marking this patch as ready for committer.
-- 
Michael Paquier
http://michael.otacoo.com


Re: [HACKERS] [WIP] pg_ping utility

2012-12-23 Thread Michael Paquier
On Sat, Dec 22, 2012 at 4:07 AM, Phil Sorber p...@omniti.com wrote:


 Added new version with default verbose and quiet option. Also updated
 docs to reflect changes.

Thanks for the updated patches.

Here is the status about the binary patch:
- Code compiles without any warnings
- After testing the patch, it behaves as expected, default option is now
verbose, the output can be hidden using -q or --quiet
However, I still have the following comments:
- in pg_isready.c, the function help needs to be static.
- the list of options called with getopt_long should be classified in
alphabetical order (the option q is not at the correct position)
d:h:p:U:qV = d:h:p:qU:V

Then, about the doc patch:
- docs compile correctly (I did a manual check)
- I am not a native English speaker, but the docs look correct to me. There
are enough examples and description is enough. No problems either with the
sgml format.

Once the 2 small things I noticed are fixed, this patch can be marked as
ready for committer.
Thanks,
-- 
Michael Paquier
http://michael.otacoo.com


Re: [HACKERS] [WIP] pg_ping utility

2012-12-23 Thread Phil Sorber
On Sun, Dec 23, 2012 at 9:29 AM, Michael Paquier
michael.paqu...@gmail.com wrote:


 On Sat, Dec 22, 2012 at 4:07 AM, Phil Sorber p...@omniti.com wrote:


 Added new version with default verbose and quiet option. Also updated
 docs to reflect changes.

 Thanks for the updated patches.

 Here is the status about the binary patch:
 - Code compiles without any warnings
 - After testing the patch, it behaves as expected, default option is now
 verbose, the output can be hidden using -q or --quiet
 However, I still have the following comments:
 - in pg_isready.c, the function help needs to be static.

I have no objection to making this static, but curious what is your
reason for it here?

 - the list of options called with getopt_long should be classified in
 alphabetical order (the option q is not at the correct position)
 d:h:p:U:qV = d:h:p:qU:V

 Then, about the doc patch:
 - docs compile correctly (I did a manual check)
 - I am not a native English speaker, but the docs look correct to me. There
 are enough examples and description is enough. No problems either with the
 sgml format.

 Once the 2 small things I noticed are fixed, this patch can be marked as
 ready for committer.

Ok, I will get an updated version later today.

 Thanks,
 --
 Michael Paquier
 http://michael.otacoo.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] [WIP] pg_ping utility

2012-12-23 Thread Erik Rijkers
On Sun, December 23, 2012 15:29, Michael Paquier wrote:

 Once the 2 small things I noticed are fixed, this patch can be marked as
 ready for committer.

I wasn't going to complain about it, but if we're going for small things 
anyway...

The output is now capitalised:

/tmp:6543 - Accepting Connections
/tmp:6000 - No Response

which looks unusual to me, could we please make it all lower-case?


TYhanks,

Erik Rijkers





-- 
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] [WIP] pg_ping utility

2012-12-23 Thread Phil Sorber
On Sun, Dec 23, 2012 at 9:57 AM, Erik Rijkers e...@xs4all.nl wrote:
 On Sun, December 23, 2012 15:29, Michael Paquier wrote:

 Once the 2 small things I noticed are fixed, this patch can be marked as
 ready for committer.

 I wasn't going to complain about it, but if we're going for small things 
 anyway...

 The output is now capitalised:

 /tmp:6543 - Accepting Connections
 /tmp:6000 - No Response

 which looks unusual to me, could we please make it all lower-case?

I'm not apposed to it in principal. Is that how it is done elsewhere
in the code? If there are no objections from anyone else I will roll
that change in with the others.



 TYhanks,

 Erik Rijkers





 --
 Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
 To make changes to your subscription:
 http://www.postgresql.org/mailpref/pgsql-hackers


-- 
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] [WIP] pg_ping utility

2012-12-23 Thread Phil Sorber
On Sun, Dec 23, 2012 at 10:07 AM, Phil Sorber p...@omniti.com wrote:
 On Sun, Dec 23, 2012 at 9:57 AM, Erik Rijkers e...@xs4all.nl wrote:
 On Sun, December 23, 2012 15:29, Michael Paquier wrote:

 Once the 2 small things I noticed are fixed, this patch can be marked as
 ready for committer.

 I wasn't going to complain about it, but if we're going for small things 
 anyway...

 The output is now capitalised:

 /tmp:6543 - Accepting Connections
 /tmp:6000 - No Response

 which looks unusual to me, could we please make it all lower-case?

 I'm not apposed to it in principal. Is that how it is done elsewhere
 in the code? If there are no objections from anyone else I will roll
 that change in with the others.



 TYhanks,

 Erik Rijkers





 --
 Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
 To make changes to your subscription:
 http://www.postgresql.org/mailpref/pgsql-hackers

Updated patch attached.


pg_isready_bin_v9.diff
Description: Binary data


pg_isready_docs_v9.diff
Description: Binary data

-- 
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] [WIP] pg_ping utility

2012-12-21 Thread Phil Sorber
On Wed, Dec 19, 2012 at 8:28 AM, Michael Paquier
michael.paqu...@gmail.com wrote:


 On Wed, Dec 12, 2012 at 12:06 AM, Bruce Momjian br...@momjian.us wrote:

 On Sat, Dec  8, 2012 at 08:59:00AM -0500, Phil Sorber wrote:
  On Sat, Dec 8, 2012 at 7:50 AM, Michael Paquier
  michael.paqu...@gmail.com wrote:
  
   Bruce mentionned that pg_isready could be used directly by pg_ctl -w.
   Default as being non-verbose would make sense. What are the other
   tools you
   are thinking about? Some utilities in core?
 
  I think Bruce meant that PQPing() is used by pg_ctl -w, not that he
  would use pg_isready.

 Right.

  OK cool. If you have some spare room to write a new version with verbose
 option as default, I'll be pleased to review it and then write it as ready
 for committer.

Added new version with default verbose and quiet option. Also updated
docs to reflect changes.

 --
 Michael Paquier
 http://michael.otacoo.com


pg_isready_bin_v8.diff
Description: Binary data


pg_isready_docs_v8.diff
Description: Binary data

-- 
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] [WIP] pg_ping utility

2012-12-19 Thread Michael Paquier
On Wed, Dec 12, 2012 at 12:06 AM, Bruce Momjian br...@momjian.us wrote:

 On Sat, Dec  8, 2012 at 08:59:00AM -0500, Phil Sorber wrote:
  On Sat, Dec 8, 2012 at 7:50 AM, Michael Paquier
  michael.paqu...@gmail.com wrote:
  
   Bruce mentionned that pg_isready could be used directly by pg_ctl -w.
   Default as being non-verbose would make sense. What are the other
 tools you
   are thinking about? Some utilities in core?
 
  I think Bruce meant that PQPing() is used by pg_ctl -w, not that he
  would use pg_isready.

 Right.

 OK cool. If you have some spare room to write a new version with verbose
option as default, I'll be pleased to review it and then write it as ready
for committer.
-- 
Michael Paquier
http://michael.otacoo.com


Re: [HACKERS] [WIP] pg_ping utility

2012-12-11 Thread Bruce Momjian
On Sat, Dec  8, 2012 at 08:59:00AM -0500, Phil Sorber wrote:
 On Sat, Dec 8, 2012 at 7:50 AM, Michael Paquier
 michael.paqu...@gmail.com wrote:
  On Fri, Dec 7, 2012 at 12:56 PM, Phil Sorber p...@omniti.com wrote:
 
  Something I was just thinking about while testing this again. I
  mentioned the issue before about someone meaning to put -v and putting
  -V instead and it being a potential source of problems. What about
  making verbose the default and removing -v and adding -q to make it
  quiet? This would also match other tools behavior. I want to get this
  wrapped up and I am fine with it as is, but just wanted to ask what
  others thought.
 
  Bruce mentionned that pg_isready could be used directly by pg_ctl -w.
  Default as being non-verbose would make sense. What are the other tools you
  are thinking about? Some utilities in core?
 
 I think Bruce meant that PQPing() is used by pg_ctl -w, not that he
 would use pg_isready.

Right.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +


-- 
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] [WIP] pg_ping utility

2012-12-08 Thread Michael Paquier
On Fri, Dec 7, 2012 at 12:56 PM, Phil Sorber p...@omniti.com wrote:

 On Thu, Dec 6, 2012 at 8:54 PM, Michael Paquier
  OK. Let's do that and then mark this patch as ready for committer.
  Thanks,

 Those changes have been made.

Cool. Thanks.


 Something I was just thinking about while testing this again. I
 mentioned the issue before about someone meaning to put -v and putting
 -V instead and it being a potential source of problems. What about
 making verbose the default and removing -v and adding -q to make it
 quiet? This would also match other tools behavior. I want to get this
 wrapped up and I am fine with it as is, but just wanted to ask what
 others thought.

Bruce mentionned that pg_isready could be used directly by pg_ctl -w.
Default as being non-verbose would make sense. What are the other tools you
are thinking about? Some utilities in core?

Except if you change the default behavior, let's change this patch status
as ready to committer.

Thanks,
-- 
Michael Paquier
http://michael.otacoo.com


Re: [HACKERS] [WIP] pg_ping utility

2012-12-08 Thread Phil Sorber
On Sat, Dec 8, 2012 at 7:50 AM, Michael Paquier
michael.paqu...@gmail.com wrote:
 On Fri, Dec 7, 2012 at 12:56 PM, Phil Sorber p...@omniti.com wrote:

 Something I was just thinking about while testing this again. I
 mentioned the issue before about someone meaning to put -v and putting
 -V instead and it being a potential source of problems. What about
 making verbose the default and removing -v and adding -q to make it
 quiet? This would also match other tools behavior. I want to get this
 wrapped up and I am fine with it as is, but just wanted to ask what
 others thought.

 Bruce mentionned that pg_isready could be used directly by pg_ctl -w.
 Default as being non-verbose would make sense. What are the other tools you
 are thinking about? Some utilities in core?

I think Bruce meant that PQPing() is used by pg_ctl -w, not that he
would use pg_isready.

I was just thinking that if someone is going to use it in a script
adding -q won't be a big deal. If someone wants to use it by hand
adding -v could become cumbersome.


 Except if you change the default behavior, let's change this patch status as
 ready to committer.

 Thanks,

 --
 Michael Paquier
 http://michael.otacoo.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] [WIP] pg_ping utility

2012-12-06 Thread Michael Paquier
On Thu, Dec 6, 2012 at 12:29 AM, Phil Sorber p...@omniti.com wrote:

 On Wed, Dec 5, 2012 at 8:53 AM, Alvaro Herrera alvhe...@2ndquadrant.com
 wrote:
  No, I think it is the reference docs on the returned value that must be
  fixed.  That is, instead of saying that the return value correspond to
  the enum values, you should be saying that it will return
  literal0/literal if it's okay, 1 in another case and 2 in yet
  another case.  And then next to the PQping() enum, add a comment that
  the values must not be messed around with because pg_isready exposes
  them to users and shell scripts.

 +1 I'm on board with this.

OK. Let's do that and then mark this patch as ready for committer.
Thanks,
-- 
Michael Paquier
http://michael.otacoo.com


Re: [HACKERS] [WIP] pg_ping utility

2012-12-06 Thread Phil Sorber
On Thu, Dec 6, 2012 at 8:54 PM, Michael Paquier
michael.paqu...@gmail.com wrote:


 On Thu, Dec 6, 2012 at 12:29 AM, Phil Sorber p...@omniti.com wrote:

 On Wed, Dec 5, 2012 at 8:53 AM, Alvaro Herrera alvhe...@2ndquadrant.com
 wrote:
  No, I think it is the reference docs on the returned value that must be
  fixed.  That is, instead of saying that the return value correspond to
  the enum values, you should be saying that it will return
  literal0/literal if it's okay, 1 in another case and 2 in yet
  another case.  And then next to the PQping() enum, add a comment that
  the values must not be messed around with because pg_isready exposes
  them to users and shell scripts.

 +1 I'm on board with this.

 OK. Let's do that and then mark this patch as ready for committer.
 Thanks,

Those changes have been made.


 --
 Michael Paquier
 http://michael.otacoo.com

Something I was just thinking about while testing this again. I
mentioned the issue before about someone meaning to put -v and putting
-V instead and it being a potential source of problems. What about
making verbose the default and removing -v and adding -q to make it
quiet? This would also match other tools behavior. I want to get this
wrapped up and I am fine with it as is, but just wanted to ask what
others thought.

Thanks.


pg_isready_bin_v7.diff
Description: Binary data


pg_isready_docs_v7.diff
Description: Binary data

-- 
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] [WIP] pg_ping utility

2012-12-05 Thread Alvaro Herrera
Phil Sorber escribió:
 On Mon, Dec 3, 2012 at 11:59 PM, Michael Paquier
 michael.paqu...@gmail.com wrote:

  - Same thing with this example:
  +   para
  +Standard Usage:
  +screen
  + prompt$/prompt userinputpg_isready/userinput
  + prompt$/prompt userinputecho $?/userinput
  + computeroutput0/computeroutput
  +/screen
  +   /para
  For the time being PQPING_OK returns 0 because it is on top of the enum
  PGPing, but this might change if for a reason or another the order of
  outputs is changed.
 
 So I understand what you mean by the ordering might change, but this
 is actual output from the shell. I'm not sure how to convey that
 sentiment properly here and still have a real example. Perhaps just
 remove the example?

No, I think it is the reference docs on the returned value that must be
fixed.  That is, instead of saying that the return value correspond to
the enum values, you should be saying that it will return
literal0/literal if it's okay, 1 in another case and 2 in yet
another case.  And then next to the PQping() enum, add a comment that
the values must not be messed around with because pg_isready exposes
them to users and shell scripts.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [WIP] pg_ping utility

2012-12-05 Thread Phil Sorber
On Wed, Dec 5, 2012 at 8:53 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote:
 Phil Sorber escribió:
 On Mon, Dec 3, 2012 at 11:59 PM, Michael Paquier
 michael.paqu...@gmail.com wrote:

  - Same thing with this example:
  +   para
  +Standard Usage:
  +screen
  + prompt$/prompt userinputpg_isready/userinput
  + prompt$/prompt userinputecho $?/userinput
  + computeroutput0/computeroutput
  +/screen
  +   /para
  For the time being PQPING_OK returns 0 because it is on top of the enum
  PGPing, but this might change if for a reason or another the order of
  outputs is changed.

 So I understand what you mean by the ordering might change, but this
 is actual output from the shell. I'm not sure how to convey that
 sentiment properly here and still have a real example. Perhaps just
 remove the example?

 No, I think it is the reference docs on the returned value that must be
 fixed.  That is, instead of saying that the return value correspond to
 the enum values, you should be saying that it will return
 literal0/literal if it's okay, 1 in another case and 2 in yet
 another case.  And then next to the PQping() enum, add a comment that
 the values must not be messed around with because pg_isready exposes
 them to users and shell scripts.

+1 I'm on board with this.


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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [WIP] pg_ping utility

2012-12-04 Thread Phil Sorber
On Mon, Dec 3, 2012 at 11:59 PM, Michael Paquier
michael.paqu...@gmail.com wrote:
 On Sun, Dec 2, 2012 at 5:56 AM, Phil Sorber p...@omniti.com wrote:

 Here is the updated patch. I renamed it, but using v5 to stay consistent.


 After looking at this patch, I found the following problems:
 - There are a couple of whitespaces still in the code, particularly at the
 end of those lines
 +   const char *pguser = NULL;
 +   const char *pgdbname = NULL;

Mystery how those got in there. Fixed.

 - When describing the values that are returned by pg_isready, it is awkward
 to refer to the returning values as plain integers as those values are part
 of an enum. You should add references to PQPING_OK, PQPING_REJECT,
 PQPING_NO_RESPONSE and PQPING_NO_ATTEMPT instead. Also add to reference
 links in the docs redirecting to them, with things of the type xref
 linkend=libpq-pqpingparams-pqping-ok or related.

Fixed. I changed the wording a little too.

 - Same thing with this example:
 +   para
 +Standard Usage:
 +screen
 + prompt$/prompt userinputpg_isready/userinput
 + prompt$/prompt userinputecho $?/userinput
 + computeroutput0/computeroutput
 +/screen
 +   /para
 For the time being PQPING_OK returns 0 because it is on top of the enum
 PGPing, but this might change if for a reason or another the order of
 outputs is changed.

So I understand what you mean by the ordering might change, but this
is actual output from the shell. I'm not sure how to convey that
sentiment properly here and still have a real example. Perhaps just
remove the example?


 Once those things are fixed, I think this will be ready for committer review
 as everybody here seem to agree with your approach.

 --
 Michael Paquier
 http://michael.otacoo.com


pg_isready_bin_v6.diff
Description: Binary data


pg_isready_docs_v6.diff
Description: Binary data

-- 
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] [WIP] pg_ping utility

2012-12-04 Thread Michael Paquier
On 2012/12/05, at 14:46, Phil Sorber p...@omniti.com wrote:

 On Mon, Dec 3, 2012 at 11:59 PM, Michael Paquier
 
 So I understand what you mean by the ordering might change, but this
 is actual output from the shell. I'm not sure how to convey that
 sentiment properly here and still have a real example. Perhaps just
 remove the example?
Yes, removing the example would be ok.

Does someone have another idea of example?

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] [WIP] pg_ping utility

2012-12-03 Thread Michael Paquier
On Sun, Dec 2, 2012 at 5:56 AM, Phil Sorber p...@omniti.com wrote:

 Here is the updated patch. I renamed it, but using v5 to stay consistent.


After looking at this patch, I found the following problems:
- There are a couple of whitespaces still in the code, particularly at the
end of those lines
+   const char *pguser = NULL;
+   const char *pgdbname = NULL;
- When describing the values that are returned by pg_isready, it is awkward
to refer to the returning values as plain integers as those values are part
of an enum. You should add references to PQPING_OK, PQPING_REJECT,
PQPING_NO_RESPONSE and PQPING_NO_ATTEMPT instead. Also add to reference
links in the docs redirecting to them, with things of the type xref
linkend=libpq-pqpingparams-pqping-ok or related.
- Same thing with this example:
+   para
+Standard Usage:
+screen
+ prompt$/prompt userinputpg_isready/userinput
+ prompt$/prompt userinputecho $?/userinput
+ computeroutput0/computeroutput
+/screen
+   /para
For the time being PQPING_OK returns 0 because it is on top of the enum
PGPing, but this might change if for a reason or another the order of
outputs is changed.

Once those things are fixed, I think this will be ready for committer
review as everybody here seem to agree with your approach.
-- 
Michael Paquier
http://michael.otacoo.com


Re: [HACKERS] [WIP] pg_ping utility

2012-12-01 Thread Phil Sorber
On Tue, Nov 27, 2012 at 9:43 AM, Phil Sorber p...@omniti.com wrote:
 On Tue, Nov 27, 2012 at 8:45 AM, Michael Paquier
 michael.paqu...@gmail.com wrote:


 On Tue, Nov 27, 2012 at 7:35 PM, Dimitri Fontaine dimi...@2ndquadrant.fr
 wrote:

 Peter Eisentraut pete...@gmx.net writes:
  Sure, PQping is useful for this very specific use case of seeing whether
  the server has finished starting up.  If someone came with a plausible

 Rename the utility to pg_isready?

 +1, the current version of the patch is already fitted for that and would
 not need extra options like the number of packages sent.

 I am 100% fine with this. I can make this change tomorrow.

 --
 Michael Paquier
 http://michael.otacoo.com

 Sorry I haven't jumped in on this thread more, I have limited
 connectivity where I am.

Here is the updated patch. I renamed it, but using v5 to stay consistent.


pg_isready_bin_v5.diff
Description: Binary data


pg_isready_docs_v5.diff
Description: Binary data

-- 
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] [WIP] pg_ping utility

2012-11-27 Thread Dimitri Fontaine
Peter Eisentraut pete...@gmx.net writes:
 Sure, PQping is useful for this very specific use case of seeing whether
 the server has finished starting up.  If someone came with a plausible

Rename the utility to pg_isready?

-- 
Dimitri Fontaine
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] [WIP] pg_ping utility

2012-11-27 Thread Michael Paquier
On Tue, Nov 27, 2012 at 7:35 PM, Dimitri Fontaine dimi...@2ndquadrant.frwrote:

 Peter Eisentraut pete...@gmx.net writes:
  Sure, PQping is useful for this very specific use case of seeing whether
  the server has finished starting up.  If someone came with a plausible

 Rename the utility to pg_isready?

+1, the current version of the patch is already fitted for that and would
not need extra options like the number of packages sent.
-- 
Michael Paquier
http://michael.otacoo.com


Re: [HACKERS] [WIP] pg_ping utility

2012-11-27 Thread Phil Sorber
On Tue, Nov 27, 2012 at 8:45 AM, Michael Paquier
michael.paqu...@gmail.com wrote:


 On Tue, Nov 27, 2012 at 7:35 PM, Dimitri Fontaine dimi...@2ndquadrant.fr
 wrote:

 Peter Eisentraut pete...@gmx.net writes:
  Sure, PQping is useful for this very specific use case of seeing whether
  the server has finished starting up.  If someone came with a plausible

 Rename the utility to pg_isready?

 +1, the current version of the patch is already fitted for that and would
 not need extra options like the number of packages sent.

I am 100% fine with this. I can make this change tomorrow.

 --
 Michael Paquier
 http://michael.otacoo.com

Sorry I haven't jumped in on this thread more, I have limited
connectivity where I am.


-- 
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] [WIP] pg_ping utility

2012-11-26 Thread Peter Eisentraut
On 11/23/12 9:48 AM, Michael Paquier wrote:
 We waited a couple of days for feedback for this feature. So based on
 all the comments provided by everybody on this thread, perhaps we should
 move on and implement the options that would make pg_ping a better
 wrapper for PQPing. Comments?

Personally, I still don't see the general utility of this.  For
monitoring, psql -c 'select 1' is much more useful.  For network
analysis, you can use network analysis tools.  The niche for pg_ping in
between those is so narrow that I cannot see it.



-- 
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] [WIP] pg_ping utility

2012-11-26 Thread Michael Paquier
On Tue, Nov 27, 2012 at 12:26 AM, Peter Eisentraut pete...@gmx.net wrote:

 On 11/23/12 9:48 AM, Michael Paquier wrote:
  We waited a couple of days for feedback for this feature. So based on
  all the comments provided by everybody on this thread, perhaps we should
  move on and implement the options that would make pg_ping a better
  wrapper for PQPing. Comments?

 Personally, I still don't see the general utility of this.  For
 monitoring, psql -c 'select 1' is much more useful.  For network
 analysis, you can use network analysis tools.  The niche for pg_ping in
 between those is so narrow that I cannot see it.

As a wrapper for PQPing, you can get different server status specific to
libpq which are PQPING_OK, PQPING_REJECT and PQPING_NO_RESPONSE, and
perhaps more in the future if PQPing is extended in a way or another. So
the purpose of this feature is to allow users to put there hands on a core
feature that would allow them to get a libpq-specific server status, and to
check the accessibility to the server with something lighter than a psql
client connection. Any additional comments Phil?
-- 
Michael Paquier
http://michael.otacoo.com


Re: [HACKERS] [WIP] pg_ping utility

2012-11-26 Thread Bruce Momjian
On Mon, Nov 26, 2012 at 10:26:27AM -0500, Peter Eisentraut wrote:
 On 11/23/12 9:48 AM, Michael Paquier wrote:
  We waited a couple of days for feedback for this feature. So based on
  all the comments provided by everybody on this thread, perhaps we should
  move on and implement the options that would make pg_ping a better
  wrapper for PQPing. Comments?
 
 Personally, I still don't see the general utility of this.  For
 monitoring, psql -c 'select 1' is much more useful.  For network
 analysis, you can use network analysis tools.  The niche for pg_ping in
 between those is so narrow that I cannot see it.

I would normally agree with this analysis, but pg_ctl -w certainly need
this ping functionality, so it kind of makes sense that others might
need it too.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +


-- 
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] [WIP] pg_ping utility

2012-11-26 Thread Peter Eisentraut
On Mon, 2012-11-26 at 13:14 -0500, Bruce Momjian wrote:
 I would normally agree with this analysis, but pg_ctl -w certainly
 need this ping functionality, so it kind of makes sense that others
 might need it too. 

Sure, PQping is useful for this very specific use case of seeing whether
the server has finished starting up.  If someone came with a plausible
use case for a startup script that couldn't use pg_ctl but wanted ping
functionality available in a shell script, then pg_ping could be
provided.  But that would also determine what options to provide.  For
example, we might not need repeated ping in that case.

But I think people see PQping and will see pg_ping as a monitoring
facility, and I think that's a mistake.




-- 
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] [WIP] pg_ping utility

2012-11-25 Thread Phil Sorber
I am going to be unavailable until Wednesday, so maybe gives us a few
more days for feedback.

On Fri, Nov 23, 2012 at 9:48 AM, Michael Paquier
michael.paqu...@gmail.com wrote:


 On Sat, Nov 17, 2012 at 2:48 AM, Phil Sorber p...@omniti.com wrote:

 On Thu, Nov 15, 2012 at 10:55 PM, Michael Paquier
 michael.paqu...@gmail.com wrote:
  On Fri, Nov 16, 2012 at 12:34 PM, Phil Sorber p...@omniti.com wrote:
  On Thu, Nov 15, 2012 at 9:23 PM, Michael Paquier
  michael.paqu...@gmail.com wrote:
   3) Having an output close to what ping actually does would also be
   nice,
   the
   current output like Accepting/Rejecting Connections are not that
 
  Could you be more specific? Are you saying you don't want to see
  accepting/rejecting info output?
 
  OK sorry.
 
  I meant something like that for an accessible server:
  $ pg_ping -c 3 -h server.com
  PING server.com (192.168.1.3)
  accept from 192.168.1.3: icmp_seq=0 time=0.241 ms
  accept from 192.168.1.3: icmp_seq=0 time=0.240 ms
  accept from 192.168.1.3: icmp_seq=0 time=0.242 ms
 
  Like that for a rejected connection:
  reject from 192.168.1.3: icmp_seq=0 time=0.241 ms
 
  Like that for a timeout:
  timeout from 192.168.1.3: icmp_seq=0
  Then print 1 line for each ping taken to stdout.

 How does icmp_seq fit into this? Or was that an oversight?

 Also, in standard ping if you don't pass -c it will continue to loop
 until interrupted. Would you suggest that pg_ping mimic that, or that
 we add an additional flag for that behavior?

 FWIW, I would use 'watch' with the existing output for cases that I
 would need something like that.

 We waited a couple of days for feedback for this feature. So based on all
 the comments provided by everybody on this thread, perhaps we should move on
 and implement the options that would make pg_ping a better wrapper for
 PQPing. Comments?
 --
 Michael Paquier
 http://michael.otacoo.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] [WIP] pg_ping utility

2012-11-25 Thread Michael Paquier
On Mon, Nov 26, 2012 at 11:17 AM, Phil Sorber p...@omniti.com wrote:

 I am going to be unavailable until Wednesday, so maybe gives us a few
 more days for feedback.

Sure no problem. Thanks.
-- 
Michael Paquier
http://michael.otacoo.com


Re: [HACKERS] [WIP] pg_ping utility

2012-11-23 Thread Michael Paquier
On Sat, Nov 17, 2012 at 2:48 AM, Phil Sorber p...@omniti.com wrote:

 On Thu, Nov 15, 2012 at 10:55 PM, Michael Paquier
 michael.paqu...@gmail.com wrote:
  On Fri, Nov 16, 2012 at 12:34 PM, Phil Sorber p...@omniti.com wrote:
  On Thu, Nov 15, 2012 at 9:23 PM, Michael Paquier
  michael.paqu...@gmail.com wrote:
   3) Having an output close to what ping actually does would also be
 nice,
   the
   current output like Accepting/Rejecting Connections are not that
 
  Could you be more specific? Are you saying you don't want to see
  accepting/rejecting info output?
 
  OK sorry.
 
  I meant something like that for an accessible server:
  $ pg_ping -c 3 -h server.com
  PING server.com (192.168.1.3)
  accept from 192.168.1.3: icmp_seq=0 time=0.241 ms
  accept from 192.168.1.3: icmp_seq=0 time=0.240 ms
  accept from 192.168.1.3: icmp_seq=0 time=0.242 ms
 
  Like that for a rejected connection:
  reject from 192.168.1.3: icmp_seq=0 time=0.241 ms
 
  Like that for a timeout:
  timeout from 192.168.1.3: icmp_seq=0
  Then print 1 line for each ping taken to stdout.

 How does icmp_seq fit into this? Or was that an oversight?

 Also, in standard ping if you don't pass -c it will continue to loop
 until interrupted. Would you suggest that pg_ping mimic that, or that
 we add an additional flag for that behavior?

 FWIW, I would use 'watch' with the existing output for cases that I
 would need something like that.

We waited a couple of days for feedback for this feature. So based on all
the comments provided by everybody on this thread, perhaps we should move
on and implement the options that would make pg_ping a better wrapper for
PQPing. Comments?
-- 
Michael Paquier
http://michael.otacoo.com


Re: [HACKERS] [WIP] pg_ping utility

2012-11-18 Thread Michael Paquier
On Sat, Nov 17, 2012 at 2:48 AM, Phil Sorber p...@omniti.com wrote:

 On Thu, Nov 15, 2012 at 10:55 PM, Michael Paquier
 michael.paqu...@gmail.com wrote:
  On Fri, Nov 16, 2012 at 12:34 PM, Phil Sorber p...@omniti.com wrote:
  On Thu, Nov 15, 2012 at 9:23 PM, Michael Paquier
  michael.paqu...@gmail.com wrote:
   3) Having an output close to what ping actually does would also be
 nice,
   the
   current output like Accepting/Rejecting Connections are not that
 
  Could you be more specific? Are you saying you don't want to see
  accepting/rejecting info output?
 
  OK sorry.
 
  I meant something like that for an accessible server:
  $ pg_ping -c 3 -h server.com
  PING server.com (192.168.1.3)
  accept from 192.168.1.3: icmp_seq=0 time=0.241 ms
  accept from 192.168.1.3: icmp_seq=0 time=0.240 ms
  accept from 192.168.1.3: icmp_seq=0 time=0.242 ms
 
  Like that for a rejected connection:
  reject from 192.168.1.3: icmp_seq=0 time=0.241 ms
 
  Like that for a timeout:
  timeout from 192.168.1.3: icmp_seq=0
  Then print 1 line for each ping taken to stdout.

 How does icmp_seq fit into this? Or was that an oversight?

Yes, sorry it doesn't fit in this model. Please forget about it.


 Also, in standard ping if you don't pass -c it will continue to loop
 until interrupted. Would you suggest that pg_ping mimic that, or that
 we add an additional flag for that behavior?

By targeting pg_ping as a clone of ping, yes it would mean that we target
it to loop indefinitely if no c flags is given.


 FWIW, I would use 'watch' with the existing output for cases that I
 would need something like that.

watch allows you to launch a program given a certain time period. I am not
sure this is related with pinging a server.
When pinging a server, what you are looking for is not only the
connectivity to it but also the latency you have with it, no?
-- 
Michael Paquier
http://michael.otacoo.com


Re: [HACKERS] [WIP] pg_ping utility

2012-11-18 Thread Michael Paquier
On Fri, Nov 16, 2012 at 2:28 PM, Tom Lane t...@sss.pgh.pa.us wrote:

 Maybe I missed something here, but I believe it's standard that
 program --help should result in exit(0), no matter what the program's
 exitcode conventions are for live-fire exercises.

Yes indeed you are right. Thanks.
-- 
Michael Paquier
http://michael.otacoo.com


Re: [HACKERS] [WIP] pg_ping utility

2012-11-16 Thread Phil Sorber
Attached is updated patch v4 with the changes Michael pointed out.

On Fri, Nov 16, 2012 at 12:28 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 Phil Sorber p...@omniti.com writes:
 On Thu, Nov 15, 2012 at 10:55 PM, Michael Paquier
 michael.paqu...@gmail.com wrote:
 Hum, it is not really consistent to use a magic number here, particularly in
 the case where an additional state would be added in the enum PGPing. So why
 not simply return PQPING_NO_ATTEMPT when there are incorrect options or you
 show the help and exit? Looks like the best option here.

 Good point. I will make that change as well.

 Maybe I missed something here, but I believe it's standard that
 program --help should result in exit(0), no matter what the program's
 exitcode conventions are for live-fire exercises.  (See
 handle_help_version_opts() in the bin/scripts/ programs, for example.)
 Okay to use NO_ATTEMPT for erroneous arguments, though.

This seems unfortunate. If someone were to put 'pg_ping -V' instead of
'pg_ping -v' into a monitoring script, however misguided, it would
make it appear as though the server were accepting connections even if
it were not. Doesn't really seem to follow our goal of least surprise.

Since this is the standard I have updated the patch to use this
behavior, though I'm not really happy with this. Not sure if I'd
rather break convention, or perhaps leave 0 sacred and add 1 to all
the other return codes to offset them.

Thoughts?


 regards, tom lane


pg_ping_bin_v4.diff
Description: Binary data

-- 
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] [WIP] pg_ping utility

2012-11-16 Thread Phil Sorber
On Thu, Nov 15, 2012 at 10:55 PM, Michael Paquier
michael.paqu...@gmail.com wrote:
 On Fri, Nov 16, 2012 at 12:34 PM, Phil Sorber p...@omniti.com wrote:
 On Thu, Nov 15, 2012 at 9:23 PM, Michael Paquier
 michael.paqu...@gmail.com wrote:
  3) Having an output close to what ping actually does would also be nice,
  the
  current output like Accepting/Rejecting Connections are not that

 Could you be more specific? Are you saying you don't want to see
 accepting/rejecting info output?

 OK sorry.

 I meant something like that for an accessible server:
 $ pg_ping -c 3 -h server.com
 PING server.com (192.168.1.3)
 accept from 192.168.1.3: icmp_seq=0 time=0.241 ms
 accept from 192.168.1.3: icmp_seq=0 time=0.240 ms
 accept from 192.168.1.3: icmp_seq=0 time=0.242 ms

 Like that for a rejected connection:
 reject from 192.168.1.3: icmp_seq=0 time=0.241 ms

 Like that for a timeout:
 timeout from 192.168.1.3: icmp_seq=0
 Then print 1 line for each ping taken to stdout.

How does icmp_seq fit into this? Or was that an oversight?

Also, in standard ping if you don't pass -c it will continue to loop
until interrupted. Would you suggest that pg_ping mimic that, or that
we add an additional flag for that behavior?

FWIW, I would use 'watch' with the existing output for cases that I
would need something like that.

 --
 Michael Paquier
 http://michael.otacoo.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] [WIP] pg_ping utility

2012-11-15 Thread Michael Paquier
Hi Phil,

I am currently looking at your patch.
A lot of people already had a look at at, but I hope I will be helpful in
finalizing it and hand it over to a committer.

Strangely I got the following error when using git apply:
$ git apply ~/download/pg_ping_v3.patch
error: src/bin/scripts/.gitignore: already exists in working directory
error: src/bin/scripts/Makefile: already exists in working directory
I don't really get from where this error comes from, but using patch -p1
made the trick.

So, regarding the review of the patch (v3):
1) pg_ping.c uses 4 spaces instead of 4-space tabs, which is a PostgreSQL
convention. You need to normalize your code respecting that.Take care of
not changing the help output which needs 4 spaces at some places though.
2) As mentionned by Christopher and Peter, pg_ping should perhaps not be
seen as a simple wrapper calling only once PQPing, but as something that
really enhance the libpq calls by incorporating options that could wrap it
more efficiently and give the users a tool to customize the ping to a
server easily. Hence, the following options make sense:
- c for the number of ping packets
- i for the interval between pings
- W for the time to wait for a response
- D output a timestamp at the beginning of ping line
- q quiet the output
Please also not that at the current state, we could do the same thing with
a simple psql -c 'SELECT 1' postgres, so those additional things look
really necessary.
3) Having an output close to what ping actually does would also be nice,
the current output like Accepting/Rejecting Connections are not that
4) I have also some security concerns about pg_ping. I noticed that even a
user who is rejected by pg_hba.conf can actually ping the server using
pg_ping or PQPing. Is this a wanted behavior? Some input here?
5) You should not use comments like that:
/* Return UNKNOWN*/
Please add a space at the end of comment for clarity like this:
/* Return UNKNOWN */
6) Please use exit(1) instead of exit(3) like the other script utilities.

Thanks,
-- 
Michael Paquier
http://michael.otacoo.com


Re: [HACKERS] [WIP] pg_ping utility

2012-11-15 Thread Phil Sorber
Thanks for the review.

On Thu, Nov 15, 2012 at 9:23 PM, Michael Paquier
michael.paqu...@gmail.com wrote:
 Hi Phil,

 I am currently looking at your patch.
 A lot of people already had a look at at, but I hope I will be helpful in
 finalizing it and hand it over to a committer.

 Strangely I got the following error when using git apply:
 $ git apply ~/download/pg_ping_v3.patch
 error: src/bin/scripts/.gitignore: already exists in working directory
 error: src/bin/scripts/Makefile: already exists in working directory
 I don't really get from where this error comes from, but using patch -p1
 made the trick.

That is strange. I will take a look to make sure I didn't do something silly.


 So, regarding the review of the patch (v3):
 1) pg_ping.c uses 4 spaces instead of 4-space tabs, which is a PostgreSQL
 convention. You need to normalize your code respecting that.Take care of not
 changing the help output which needs 4 spaces at some places though.

Ah yes, good catch. Will fix.

 2) As mentionned by Christopher and Peter, pg_ping should perhaps not be
 seen as a simple wrapper calling only once PQPing, but as something that
 really enhance the libpq calls by incorporating options that could wrap it
 more efficiently and give the users a tool to customize the ping to a server
 easily. Hence, the following options make sense:
 - c for the number of ping packets
 - i for the interval between pings
 - W for the time to wait for a response
 - D output a timestamp at the beginning of ping line
 - q quiet the output
 Please also not that at the current state, we could do the same thing with a
 simple psql -c 'SELECT 1' postgres, so those additional things look really
 necessary.

Ok, so now 3 votes for this. I guess this is a desired feature. I'm
still not clear on the use case for this. I could see something like
wanting to run the command repeatedly so you could see when a server
was out of recovery and accepting connections, but couldn't that be
achieved with watch? I'm also not clear what the return code would be
if it has mixed results. We could return the last result, or perhaps a
new return code for the mixed case.

Since more people want it, I will make a version with this and see how
others feel.

 3) Having an output close to what ping actually does would also be nice, the
 current output like Accepting/Rejecting Connections are not that

Could you be more specific? Are you saying you don't want to see
accepting/rejecting info output?

 4) I have also some security concerns about pg_ping. I noticed that even a
 user who is rejected by pg_hba.conf can actually ping the server using
 pg_ping or PQPing. Is this a wanted behavior? Some input here?

This is desired and important behavior. It's not specific to pg_ping,
but written into libpq. Also, it's not a special part of the protocol,
it just detects how far in the connection process it was able to get
to decide if the server is accepting connections. It's not really
leaking any extra information that someone couldn't figure out already
with the regular connection facilities.

This is also why I feel pg_ping is more useful than psql -c 'SELECT
1' postgres.

 5) You should not use comments like that:
 /* Return UNKNOWN*/
 Please add a space at the end of comment for clarity like this:
 /* Return UNKNOWN */

Will fix.

 6) Please use exit(1) instead of exit(3) like the other script utilities.

We can't actually do this. It is important that it uses exit(3)
(UNKNOWN). What this really says to me is the comment from the
previous item should be expanded to explain this further. If we
exit(1) it will imply some other state (rejecting connections) that is
not known for the cases where we exit(3). The return code of pg_ping
is an important feature. Or are you suggesting that we merely reorder
these so that it aligns with the return code of other utilities and is
not aligned with the return value of PQping?


 Thanks,
 --
 Michael Paquier
 http://michael.otacoo.com

Thanks again for the review.


-- 
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] [WIP] pg_ping utility

2012-11-15 Thread Michael Paquier
On Fri, Nov 16, 2012 at 12:34 PM, Phil Sorber p...@omniti.com wrote:

 Thanks for the review.

 On Thu, Nov 15, 2012 at 9:23 PM, Michael Paquier
 michael.paqu...@gmail.com wrote:
  Hi Phil,
 
  I am currently looking at your patch.
  A lot of people already had a look at at, but I hope I will be helpful in
  finalizing it and hand it over to a committer.
 
  Strangely I got the following error when using git apply:
  $ git apply ~/download/pg_ping_v3.patch
  error: src/bin/scripts/.gitignore: already exists in working directory
  error: src/bin/scripts/Makefile: already exists in working directory
  I don't really get from where this error comes from, but using patch -p1
  made the trick.

 That is strange. I will take a look to make sure I didn't do something
 silly.

Don't worry, that is not a big deal. I might as well have something not
correctly configured in my box.



  2) As mentionned by Christopher and Peter, pg_ping should perhaps not be
  seen as a simple wrapper calling only once PQPing, but as something that
  really enhance the libpq calls by incorporating options that could wrap
 it
  more efficiently and give the users a tool to customize the ping to a
 server
  easily. Hence, the following options make sense:
  - c for the number of ping packets
  - i for the interval between pings
  - W for the time to wait for a response
  - D output a timestamp at the beginning of ping line
  - q quiet the output
  Please also not that at the current state, we could do the same thing
 with a
  simple psql -c 'SELECT 1' postgres, so those additional things look
 really
  necessary.

 Ok, so now 3 votes for this. I guess this is a desired feature. I'm
 still not clear on the use case for this. I could see something like
 wanting to run the command repeatedly so you could see when a server
 was out of recovery and accepting connections, but couldn't that be
 achieved with watch? I'm also not clear what the return code would be
 if it has mixed results. We could return the last result, or perhaps a
 new return code for the mixed case.

 Since more people want it, I will make a version with this and see how
 others feel.

Before coding, let's be sure that people agree on that. It is better to
avoid unnecessary coding. At least 3 people, including me, feel that way
based on this thread.
So other opinions?

 3) Having an output close to what ping actually does would also be nice,
 the
  current output like Accepting/Rejecting Connections are not that

 Could you be more specific? Are you saying you don't want to see
 accepting/rejecting info output?

OK sorry.

I meant something like that for an accessible server:
$ pg_ping -c 3 -h server.com
PING server.com (192.168.1.3)
accept from 192.168.1.3: icmp_seq=0 time=0.241 ms
accept from 192.168.1.3: icmp_seq=0 time=0.240 ms
accept from 192.168.1.3: icmp_seq=0 time=0.242 ms

Like that for a rejected connection:
reject from 192.168.1.3: icmp_seq=0 time=0.241 ms

Like that for a timeout:
timeout from 192.168.1.3: icmp_seq=0
Then print 1 line for each ping taken to stdout.


  4) I have also some security concerns about pg_ping. I noticed that even
 a
  user who is rejected by pg_hba.conf can actually ping the server using
  pg_ping or PQPing. Is this a wanted behavior? Some input here?

 This is desired and important behavior. It's not specific to pg_ping,
 but written into libpq. Also, it's not a special part of the protocol,
 it just detects how far in the connection process it was able to get
 to decide if the server is accepting connections. It's not really
 leaking any extra information that someone couldn't figure out already
 with the regular connection facilities.

OK understood. I was only wondering about it.

This is also why I feel pg_ping is more useful than psql -c 'SELECT
 1' postgres.

  6) Please use exit(1) instead of exit(3) like the other script utilities.

 We can't actually do this. It is important that it uses exit(3)
 (UNKNOWN). What this really says to me is the comment from the
 previous item should be expanded to explain this further. If we
 exit(1) it will imply some other state (rejecting connections) that is
 not known for the cases where we exit(3). The return code of pg_ping
 is an important feature. Or are you suggesting that we merely reorder
 these so that it aligns with the return code of other utilities and is
 not aligned with the return value of PQping?

Hum, it is not really consistent to use a magic number here, particularly
in the case where an additional state would be added in the enum PGPing. So
why not simply return PQPING_NO_ATTEMPT when there are incorrect options or
you show the help and exit? Looks like the best option here.
-- 
Michael Paquier
http://michael.otacoo.com


Re: [HACKERS] [WIP] pg_ping utility

2012-11-15 Thread Phil Sorber
On Thu, Nov 15, 2012 at 10:55 PM, Michael Paquier
michael.paqu...@gmail.com wrote:
 Hum, it is not really consistent to use a magic number here, particularly in
 the case where an additional state would be added in the enum PGPing. So why
 not simply return PQPING_NO_ATTEMPT when there are incorrect options or you
 show the help and exit? Looks like the best option here.

Good point. I will make that change as well.

 --
 Michael Paquier
 http://michael.otacoo.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] [WIP] pg_ping utility

2012-10-24 Thread Alvaro Herrera
Guys,

May I remind everyone that we still have an commitfest open, which to
date has 23 patches needing some effort, and that this patch, while
probably very useful and interesting, belongs to the next commitfest
which is not yet in progress.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [WIP] pg_ping utility

2012-10-24 Thread Thom Brown
On 24 October 2012 15:24, Alvaro Herrera alvhe...@2ndquadrant.com wrote:
 Guys,

 May I remind everyone that we still have an commitfest open, which to
 date has 23 patches needing some effort, and that this patch, while
 probably very useful and interesting, belongs to the next commitfest
 which is not yet in progress.

Phil added it to the 2012-11 commitfest, which appears to be the
correct one.  The in progress one is 2012-09.

-- 
Thom


-- 
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] [WIP] pg_ping utility

2012-10-24 Thread Alvaro Herrera
Thom Brown wrote:
 On 24 October 2012 15:24, Alvaro Herrera alvhe...@2ndquadrant.com wrote:
  Guys,
 
  May I remind everyone that we still have an commitfest open, which to
  date has 23 patches needing some effort, and that this patch, while
  probably very useful and interesting, belongs to the next commitfest
  which is not yet in progress.
 
 Phil added it to the 2012-11 commitfest, which appears to be the
 correct one.  The in progress one is 2012-09.

You're correct.  So am I -- except that the word open in my paragraph
above should have been in progress.  We do need people to work on the
patches in the 2012-09 commitfest.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [WIP] pg_ping utility

2012-10-23 Thread Peter Eisentraut
On 10/22/12 11:47 AM, Phil Sorber wrote:
 On Sun, Oct 21, 2012 at 6:20 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Phil Sorber p...@omniti.com writes:
 Here is the new patch. I renamed the utility from pg_ping to pingdb to
 go along with the naming convention of src/bin/scripts.

 Uh, no, that's not a step forward.  Leaving out a pg prefix from those
 script names is universally agreed to have been a mistake.  We've not
 felt that changing the legacy names is worth the amount of pain it'd
 cause, but that doesn't mean that we should propagate the mistake into
 brand new executable names.

 regards, tom lane
 
 Ok. I asked about this and got no response so I assume there were no
 strong opinions. I have reverted to the pg_ping name. Patches
 attached.

Quick review ...

Code:

*** install: all installdirs
*** 54,59 
--- 55,61 
$(INSTALL_PROGRAM) clusterdb$(X)  '$(DESTDIR)$(bindir)'/clusterdb$(X)
$(INSTALL_PROGRAM) vacuumdb$(X)   '$(DESTDIR)$(bindir)'/vacuumdb$(X)
$(INSTALL_PROGRAM) reindexdb$(X)  '$(DESTDIR)$(bindir)'/reindexdb$(X)
+   $(INSTALL_PROGRAM) pg_ping$(X) '$(DESTDIR)$(bindir)'/pg_ping$(X)
  
  installdirs:
$(MKDIR_P) '$(DESTDIR)$(bindir)'

Whitespace misaligned

+ exit(3); // Return UNKNOWN

No // comments.

+ while (NULL != conn_opt_ptr-keyword)

Better writte as

while (conn_opt_ptr-keyword != NULL)

or

while (conn_opt_ptr-keyword)


Also, it seems that about 75% of the patch is connection options processing.  
How about
we get rid of all that and just have them specify a connection string?  It 
would be a break
with tradition, but maybe it's time for something new.


Functionality:

I'm missing the typical ping functionality to ping continuously.  If we're 
going to call
it pg_ping, it ought to do something similar to ping, I think.


-- 
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] [WIP] pg_ping utility

2012-10-23 Thread Christopher Browne
On Tue, Oct 23, 2012 at 6:12 PM, Peter Eisentraut pete...@gmx.net wrote:
 On 10/22/12 11:47 AM, Phil Sorber wrote:
 Also, it seems that about 75% of the patch is connection options processing.  
 How about
 we get rid of all that and just have them specify a connection string?  It 
 would be a break
 with tradition, but maybe it's time for something new.

I'd be pretty pleased if it had just two ways to get configured:
a) A connection string (which might, in the new order of things, be a
JDBC-like URI), or
b) Environment values drawn in from PGHOST/PGPORT/...

That's pretty much enough configurability, I'd think.

 Functionality:

 I'm missing the typical ping functionality to ping continuously.  If we're 
 going to call
 it pg_ping, it ought to do something similar to ping, I think.

Yep, should have equivalents to:
 -i, an interval between pings,
 -c, a count
 -w/-W, a timeout interval

Might be nice to have analogues to:
-D printing timestamp before each line
-q quiets output
-v verbose output (got it, check!)
-V version (got it, check!)
-- 
When confronted by a difficult problem, solve it by reducing it to the
question, How would the Lone Ranger handle this?


-- 
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] [WIP] pg_ping utility

2012-10-23 Thread Phil Sorber
On Tue, Oct 23, 2012 at 6:12 PM, Peter Eisentraut pete...@gmx.net wrote:
 Quick review ...

 Code:

 *** install: all installdirs
 *** 54,59 
 --- 55,61 
 $(INSTALL_PROGRAM) clusterdb$(X)  '$(DESTDIR)$(bindir)'/clusterdb$(X)
 $(INSTALL_PROGRAM) vacuumdb$(X)   '$(DESTDIR)$(bindir)'/vacuumdb$(X)
 $(INSTALL_PROGRAM) reindexdb$(X)  '$(DESTDIR)$(bindir)'/reindexdb$(X)
 +   $(INSTALL_PROGRAM) pg_ping$(X) '$(DESTDIR)$(bindir)'/pg_ping$(X)

   installdirs:
 $(MKDIR_P) '$(DESTDIR)$(bindir)'

 Whitespace misaligned

Fixed.


 + exit(3); // Return UNKNOWN

 No // comments.

Changed.


 + while (NULL != conn_opt_ptr-keyword)

 Better writte as

 while (conn_opt_ptr-keyword != NULL)

 or

 while (conn_opt_ptr-keyword)

Changed to the latter.



 Also, it seems that about 75% of the patch is connection options processing.  
 How about
 we get rid of all that and just have them specify a connection string?  It 
 would be a break
 with tradition, but maybe it's time for something new.

I don't think that should be a part of this patch. If we think that we
should remove connection params and just pass a connection string we
should probably deprecate connection params and remove them everywhere
together over a period of time like with other features. I don't think
we should introduce a new binary that doesn't work the same way as all
the other binaries.

I went back and checked, and realized I didn't do it correctly, but
this patch now does allow a connection string to be passed to -d. This
seems to be the accepted way to do this.



 Functionality:

 I'm missing the typical ping functionality to ping continuously.  If we're 
 going to call
 it pg_ping, it ought to do something similar to ping, I think.

It's not named after the network utility but after the libpq function
PQping. Since this doesn't output latencies or really much of anything
else like the network ping, I'm not sure it makes sense to do that,
but I could be convinced otherwise.

Attaching an updated patch.


pg_ping_bin_v3.diff
Description: Binary data

-- 
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] [WIP] pg_ping utility

2012-10-23 Thread Phil Sorber
On Tue, Oct 23, 2012 at 6:22 PM, Christopher Browne cbbro...@gmail.com wrote:
 On Tue, Oct 23, 2012 at 6:12 PM, Peter Eisentraut pete...@gmx.net wrote:
 On 10/22/12 11:47 AM, Phil Sorber wrote:
 Also, it seems that about 75% of the patch is connection options processing. 
  How about
 we get rid of all that and just have them specify a connection string?  It 
 would be a break
 with tradition, but maybe it's time for something new.

 I'd be pretty pleased if it had just two ways to get configured:
 a) A connection string (which might, in the new order of things, be a
 JDBC-like URI), or
 b) Environment values drawn in from PGHOST/PGPORT/...

When I first wrote this for my own purposes it was basically 'return
PQping();' with the necessary glue around it and I used the env
var's exactly as you describe. I ended up adding connection parameters
to satisfy the ops guy who was having trouble integrating it how we
wanted to use it. I figured that to go in core it would need that
anyway. I'm not sure why we would want to prevent people from using
command line options like that. That is often the most intuitive way
to use a tool. Either way I think this is probably a separate debate
entirely.


 That's pretty much enough configurability, I'd think.

 Functionality:

 I'm missing the typical ping functionality to ping continuously.  If we're 
 going to call
 it pg_ping, it ought to do something similar to ping, I think.

 Yep, should have equivalents to:
  -i, an interval between pings,
  -c, a count
  -w/-W, a timeout interval

Like I replied to Peter above, I'm not sure it fits the mold of the
ping network utility. If you think it needs a different name please
propose one. I'm not totally closed off to this idea, it's just not
what I had in mind when I put it together. If people find it useful, I
can add it.


 Might be nice to have analogues to:
 -D printing timestamp before each line
 -q quiets output
 -v verbose output (got it, check!)
 -V version (got it, check!)

Right now it outputs nothing by default. I suppose I could change it
to output Accepting/Rejecting Connections by default, and verbose
can add the connection info. Thoughts?


-- 
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] [WIP] pg_ping utility

2012-10-22 Thread Phil Sorber
On Sun, Oct 21, 2012 at 6:20 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Phil Sorber p...@omniti.com writes:
 Here is the new patch. I renamed the utility from pg_ping to pingdb to
 go along with the naming convention of src/bin/scripts.

 Uh, no, that's not a step forward.  Leaving out a pg prefix from those
 script names is universally agreed to have been a mistake.  We've not
 felt that changing the legacy names is worth the amount of pain it'd
 cause, but that doesn't mean that we should propagate the mistake into
 brand new executable names.

 regards, tom lane

Ok. I asked about this and got no response so I assume there were no
strong opinions. I have reverted to the pg_ping name. Patches
attached.


pg_ping_bin_v2.diff
Description: Binary data


pg_ping_docs_v2.diff
Description: Binary data

-- 
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] [WIP] pg_ping utility

2012-10-21 Thread Phil Sorber
On Mon, Oct 15, 2012 at 9:18 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Phil Sorber p...@omniti.com writes:
 On Mon, Oct 15, 2012 at 7:12 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Yeah, I know a whole new executable is kind of a pain, and the amount of
 infrastructure and added maintenance seems a bit high compared to what
 this does.  But a lot of the programs in src/bin/scripts are not much
 bigger.  (In fact that might be the best place for this.)

 I considered src/bin/scripts but all those are for maintenance tasks
 on the DB. createdb/vacuumdb/reindexdb etc. It doesn't need any of the
 bits in common.h/common.c, nor does it need some of the includes that
 the build process has.

 Well, we classify all those programs as client-side tools in the
 documentation, so I don't see that pg_ping doesn't belong there.

 The alternative is to give it its very own subdirectory under src/bin/;
 which increases the infrastructure burden *significantly* (eg, now it
 needs its own NLS message catalog) for not a lot of value IMO.

 I would also like it to have a regression test
 which none of those seem to have.

 [ shrug... ]  There is nothing in the current regression infrastructure
 that would work for this, so that desire is pie-in-the-sky regardless of
 where you put it in the source tree.  Also, PQping itself is exercised
 in every buildfarm run as part of pg_ctl start, so I don't feel a real
 strong need to test pg_ping separately.

 regards, tom lane

Here is the new patch. I renamed the utility from pg_ping to pingdb to
go along with the naming convention of src/bin/scripts. Updated docs
and made some other minor improvements.


pingdb-bin.diff
Description: Binary data


pingdb-doc.diff
Description: Binary data

-- 
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] [WIP] pg_ping utility

2012-10-21 Thread Tom Lane
Phil Sorber p...@omniti.com writes:
 Here is the new patch. I renamed the utility from pg_ping to pingdb to
 go along with the naming convention of src/bin/scripts.

Uh, no, that's not a step forward.  Leaving out a pg prefix from those
script names is universally agreed to have been a mistake.  We've not
felt that changing the legacy names is worth the amount of pain it'd
cause, but that doesn't mean that we should propagate the mistake into
brand new executable names.

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] [WIP] pg_ping utility

2012-10-15 Thread Thom Brown
On 13 October 2012 22:19, Phil Sorber p...@omniti.com wrote:
 Based on a previous thread
 (http://archives.postgresql.org/pgsql-hackers/2012-10/msg00131.php) I
 have put together a first attempt of a pg_ping utility. I am attaching
 two patches. One for the executable and one for the docs.

 I would also like to make a regression tests and translations, but
 wanted to get feedback on what I had so far.

pg_pong:

1 packets transmitted, 1 received, 0% packet loss, time 2 days

Well this works for me, and I raised a couple typos directly to Phil.
The advantage of this over pg_ctl status is that it doesn't have to
be run on the machine local to the database, and access to the data
directory isn't required if it is run locally.  The advantage over
connecting using a regular connection is that authentication and
authorisation isn't necessary, and if all connections are in use, it
will still return the desired result.  And it does what it says on the
tin.

So +1 from me.

-- 
Thom


-- 
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] [WIP] pg_ping utility

2012-10-15 Thread Andres Freund
On Monday, October 15, 2012 11:28:36 PM Thom Brown wrote:
 On 13 October 2012 22:19, Phil Sorber p...@omniti.com wrote:
  Based on a previous thread
  (http://archives.postgresql.org/pgsql-hackers/2012-10/msg00131.php) I
  have put together a first attempt of a pg_ping utility. I am attaching
  two patches. One for the executable and one for the docs.
  
  I would also like to make a regression tests and translations, but
  wanted to get feedback on what I had so far.
 
 pg_pong:
 
 1 packets transmitted, 1 received, 0% packet loss, time 2 days
 
 Well this works for me, and I raised a couple typos directly to Phil.
 The advantage of this over pg_ctl status is that it doesn't have to
 be run on the machine local to the database, and access to the data
 directory isn't required if it is run locally.  The advantage over
 connecting using a regular connection is that authentication and
 authorisation isn't necessary, and if all connections are in use, it
 will still return the desired result.  And it does what it says on the
 tin.
 
 So +1 from me.

Why not add a pg_ctl subcommand for that? For me that sounds like a good place 
for it...

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [WIP] pg_ping utility

2012-10-15 Thread Phil Sorber
On Mon, Oct 15, 2012 at 5:32 PM, Andres Freund and...@2ndquadrant.com wrote:
 On Monday, October 15, 2012 11:28:36 PM Thom Brown wrote:
 On 13 October 2012 22:19, Phil Sorber p...@omniti.com wrote:
  Based on a previous thread
  (http://archives.postgresql.org/pgsql-hackers/2012-10/msg00131.php) I
  have put together a first attempt of a pg_ping utility. I am attaching
  two patches. One for the executable and one for the docs.
 
  I would also like to make a regression tests and translations, but
  wanted to get feedback on what I had so far.

 pg_pong:

 1 packets transmitted, 1 received, 0% packet loss, time 2 days

 Well this works for me, and I raised a couple typos directly to Phil.
 The advantage of this over pg_ctl status is that it doesn't have to
 be run on the machine local to the database, and access to the data
 directory isn't required if it is run locally.  The advantage over
 connecting using a regular connection is that authentication and
 authorisation isn't necessary, and if all connections are in use, it
 will still return the desired result.  And it does what it says on the
 tin.

 So +1 from me.

 Why not add a pg_ctl subcommand for that? For me that sounds like a good place
 for it...

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

We discussed that in the other thread. pg_ctl is often only (always?)
packaged with the server binaries and not client. Discussed adding it
to psql, but Tom said he'd prefer to see it as a standalone binary
anyway. I don't have any real strong opinion about it going into an
existing binary like psql (I have a patch for this too) or being
standalone, I just think we should have some way to do this from the
command line on a client. It seems trivial, but I think it's very
useful and if our libpq already supports this, why not?

FWIW pg_ctl does use the same API (PQping), but it doesn't expose it
as an option you can use exclusively. It just uses it to make sure the
server is up/down depending on what it is trying to do.


-- 
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] [WIP] pg_ping utility

2012-10-15 Thread Tom Lane
Andres Freund and...@2ndquadrant.com writes:
 Why not add a pg_ctl subcommand for that? For me that sounds like a good 
 place 
 for it...

I think that's a bad fit, because every other pg_ctl subcommand requires
access to the data directory.  It would be very confusing if this one
subcommand worked remotely when the others didn't.

There was also some discussion of wedging it into psql, which would at
least have the advantage that it'd typically be installed on the right
side of the client/server divide.  But I still think wedging into is
the appropriate verb there: psql is a tool for making a connection and
executing some SQL commands, and ping is not that.

Yeah, I know a whole new executable is kind of a pain, and the amount of
infrastructure and added maintenance seems a bit high compared to what
this does.  But a lot of the programs in src/bin/scripts are not much
bigger.  (In fact that might be the best place for this.)

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] [WIP] pg_ping utility

2012-10-15 Thread David Johnston
 -Original Message-
 From: pgsql-hackers-ow...@postgresql.org [mailto:pgsql-hackers-
 ow...@postgresql.org] On Behalf Of Tom Lane
 Sent: Monday, October 15, 2012 7:13 PM
 To: Andres Freund
 Cc: pgsql-hackers@postgresql.org; Thom Brown; Phil Sorber
 Subject: Re: [HACKERS] [WIP] pg_ping utility
 
 Andres Freund and...@2ndquadrant.com writes:
  Why not add a pg_ctl subcommand for that? For me that sounds like a
  good place for it...
 
 I think that's a bad fit, because every other pg_ctl subcommand requires
 access to the data directory.  It would be very confusing if this one
 subcommand worked remotely when the others didn't.
 
 There was also some discussion of wedging it into psql, which would at
least
 have the advantage that it'd typically be installed on the right side of
the
 client/server divide.  But I still think wedging into is the appropriate
verb
 there: psql is a tool for making a connection and executing some SQL
 commands, and ping is not that.
 
 Yeah, I know a whole new executable is kind of a pain, and the amount of
 infrastructure and added maintenance seems a bit high compared to what
 this does.  But a lot of the programs in src/bin/scripts are not much
bigger.
 (In fact that might be the best place for this.)
 
   regards, tom lane
 

This seems to be begging for a canonical pg_monitor command where
pg_ping would be one sub-command.  A bit much for a single command but it
would provide a frame onto which additional user interfaces could be hung -
though I am lacking for concrete examples at the moment.  pg_monitor would
be focused on database monitoring and not cluster monitoring generally
but pg_ping would be a necessary pre-requisite since if the cluster is not
available database monitoring doesn't make any sense.

With the recent focus on pg_stat_statements and the current WIP on
pg_lwlocks having an official UI for accessing much of this kind data has
merit.  Encapsulating the queries into commands makes actually using them
easier and there can be associated documentation discussing how to interpret
those specific commands and some level of consistency when asking for data
for bug and performance reports.  It may be that psql already does much of
this as I am just not that familiar with the program but if that is the case
then classifying it as making a connection and executing some SQL commands
is a limited description.  pg_ping is arguably doing at least the first part
of that.

David J.





-- 
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] [WIP] pg_ping utility

2012-10-15 Thread Phil Sorber
On Mon, Oct 15, 2012 at 7:12 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Andres Freund and...@2ndquadrant.com writes:
 Why not add a pg_ctl subcommand for that? For me that sounds like a good 
 place
 for it...

 I think that's a bad fit, because every other pg_ctl subcommand requires
 access to the data directory.  It would be very confusing if this one
 subcommand worked remotely when the others didn't.

 There was also some discussion of wedging it into psql, which would at
 least have the advantage that it'd typically be installed on the right
 side of the client/server divide.  But I still think wedging into is
 the appropriate verb there: psql is a tool for making a connection and
 executing some SQL commands, and ping is not that.

 Yeah, I know a whole new executable is kind of a pain, and the amount of
 infrastructure and added maintenance seems a bit high compared to what
 this does.  But a lot of the programs in src/bin/scripts are not much
 bigger.  (In fact that might be the best place for this.)

 regards, tom lane

I considered src/bin/scripts but all those are for maintenance tasks
on the DB. createdb/vacuumdb/reindexdb etc. It doesn't need any of the
bits in common.h/common.c, nor does it need some of the includes that
the build process has. I would also like it to have a regression test
which none of those seem to have. Seems like it would be a bit of a
wedge there as well, but if we did, maybe we call it pingdb instead?

If there is consensus that we want it to live there, I can write a
patch for that as well. Though just to be clear my preference at this
point is still for a standalone binary.


-- 
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] [WIP] pg_ping utility

2012-10-15 Thread Tom Lane
David Johnston pol...@yahoo.com writes:
 Yeah, I know a whole new executable is kind of a pain, and the amount of
 infrastructure and added maintenance seems a bit high compared to what
 this does.  But a lot of the programs in src/bin/scripts are not much
 bigger. (In fact that might be the best place for this.)

 This seems to be begging for a canonical pg_monitor command where
 pg_ping would be one sub-command.  A bit much for a single command but it
 would provide a frame onto which additional user interfaces could be hung -
 though I am lacking for concrete examples at the moment.

Meh.  If we had near-term plans for more such subcommands, that might
make sense.  But I think all that's really wanted here is a command-line
wrapper around the libpq PQping() functionality.  People who are trying
to build more complex monitoring tools are likely to be using PQping()
directly anyway, rather than invoking a command-line tool.

 With the recent focus on pg_stat_statements and the current WIP on
 pg_lwlocks having an official UI for accessing much of this kind data has
 merit.

None of that stuff is accessible without opening up an actual database
connection, and IMO the whole point of PQping is that it *doesn't* open
a connection and thus doesn't get into problems of user authentication.
So I really think it's a different sort of animal that needs a separate
API.

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] [WIP] pg_ping utility

2012-10-15 Thread Tom Lane
Phil Sorber p...@omniti.com writes:
 On Mon, Oct 15, 2012 at 7:12 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Yeah, I know a whole new executable is kind of a pain, and the amount of
 infrastructure and added maintenance seems a bit high compared to what
 this does.  But a lot of the programs in src/bin/scripts are not much
 bigger.  (In fact that might be the best place for this.)

 I considered src/bin/scripts but all those are for maintenance tasks
 on the DB. createdb/vacuumdb/reindexdb etc. It doesn't need any of the
 bits in common.h/common.c, nor does it need some of the includes that
 the build process has.

Well, we classify all those programs as client-side tools in the
documentation, so I don't see that pg_ping doesn't belong there.

The alternative is to give it its very own subdirectory under src/bin/;
which increases the infrastructure burden *significantly* (eg, now it
needs its own NLS message catalog) for not a lot of value IMO.

 I would also like it to have a regression test
 which none of those seem to have.

[ shrug... ]  There is nothing in the current regression infrastructure
that would work for this, so that desire is pie-in-the-sky regardless of
where you put it in the source tree.  Also, PQping itself is exercised
in every buildfarm run as part of pg_ctl start, so I don't feel a real
strong need to test pg_ping separately.

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] [WIP] pg_ping utility

2012-10-15 Thread Phil Sorber
On Mon, Oct 15, 2012 at 9:18 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Phil Sorber p...@omniti.com writes:
 I would also like it to have a regression test
 which none of those seem to have.

 [ shrug... ]  There is nothing in the current regression infrastructure
 that would work for this, so that desire is pie-in-the-sky regardless of
 where you put it in the source tree.  Also, PQping itself is exercised
 in every buildfarm run as part of pg_ctl start, so I don't feel a real
 strong need to test pg_ping separately.

My plan was to borrow heavily from the pg_upgrade test. I want to
verify the exit status based on known database state as presumably
people would be using this for monitoring/load balancing, etc. Hoping
to prevent silly breakage like the help output from returning an
'Accepting Connections' exit status.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers