Re: [HACKERS] [WIP] pg_ping utility
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
-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
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
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
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
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
[HACKERS] [WIP] pg_ping utility
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. Thanks. pg_ping_bin.diff Description: Binary data pg_ping_docs.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
