Thread: [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.
Attachment
On 13 October 2012 22:19, Phil Sorber <phil@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
On Monday, October 15, 2012 11:28:36 PM Thom Brown wrote: > On 13 October 2012 22:19, Phil Sorber <phil@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
On Mon, Oct 15, 2012 at 5:32 PM, Andres Freund <andres@2ndquadrant.com> wrote: > On Monday, October 15, 2012 11:28:36 PM Thom Brown wrote: >> On 13 October 2012 22:19, Phil Sorber <phil@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.
Andres Freund <andres@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
> -----Original Message----- > From: pgsql-hackers-owner@postgresql.org [mailto:pgsql-hackers- > owner@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 <andres@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.
On Mon, Oct 15, 2012 at 7:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Andres Freund <andres@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.
"David Johnston" <polobo@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
Phil Sorber <phil@omniti.com> writes: > On Mon, Oct 15, 2012 at 7:12 PM, Tom Lane <tgl@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
On Mon, Oct 15, 2012 at 9:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Phil Sorber <phil@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.
On Mon, Oct 15, 2012 at 9:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Phil Sorber <phil@omniti.com> writes: >> On Mon, Oct 15, 2012 at 7:12 PM, Tom Lane <tgl@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.
Attachment
Phil Sorber <phil@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
On Sun, Oct 21, 2012 at 6:20 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Phil Sorber <phil@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.
Attachment
On 10/22/12 11:47 AM, Phil Sorber wrote: > On Sun, Oct 21, 2012 at 6:20 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Phil Sorber <phil@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.
On Tue, Oct 23, 2012 at 6:12 PM, Peter Eisentraut <peter_e@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?"
On Tue, Oct 23, 2012 at 6:12 PM, Peter Eisentraut <peter_e@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.
Attachment
On Tue, Oct 23, 2012 at 6:22 PM, Christopher Browne <cbbrowne@gmail.com> wrote: > On Tue, Oct 23, 2012 at 6:12 PM, Peter Eisentraut <peter_e@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?
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 Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On 24 October 2012 15:24, Alvaro Herrera <alvherre@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
Thom Brown wrote: > On 24 October 2012 15:24, Alvaro Herrera <alvherre@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 Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Hi Phil,<br /><br />I am currently looking at your patch.<br />A lot of people already had a look at at, but I hope I willbe helpful in finalizing it and hand it over to a committer.<br /><br />Strangely I got the following error when usinggit apply:<br /> $ git apply ~/download/pg_ping_v3.patch <br />error: src/bin/scripts/.gitignore: already exists inworking directory<br />error: src/bin/scripts/Makefile: already exists in working directory<br />I don't really get fromwhere this error comes from, but using patch -p1 made the trick.<br /><br />So, regarding the review of the patch (v3):<br/>1) pg_ping.c uses 4 spaces instead of 4-space tabs, which is a PostgreSQL convention. You need to normalize yourcode respecting that.Take care of not changing the help output which needs 4 spaces at some places though.<br /> 2) Asmentionned by Christopher and Peter, pg_ping should perhaps not be seen as a simple wrapper calling only once PQPing, butas something that really enhance the libpq calls by incorporating options that could wrap it more efficiently and givethe users a tool to customize the ping to a server easily. Hence, the following options make sense:<br /> - c for thenumber of ping packets<br />- i for the interval between pings<br />- W for the time to wait for a response<br />- D outputa timestamp at the beginning of ping line<br />- q quiet the output<br />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.<br/> 3) Having an output close to what ping actually does would also be nice, the current output like Accepting/RejectingConnections are not that<br />4) I have also some security concerns about pg_ping. I noticed that evena 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?<br /> 5) You should not use comments like that:<br />/* Return UNKNOWN*/<br />Please add a spaceat the end of comment for clarity like this:<br />/* Return UNKNOWN */<br />6) Please use exit(1) instead of exit(3)like the other script utilities.<br /><br />Thanks,<br />-- <br />Michael Paquier<br /><a href="http://michael.otacoo.com"target="_blank">http://michael.otacoo.com</a><br />
Thanks for the review. On Thu, Nov 15, 2012 at 9:23 PM, Michael Paquier <michael.paquier@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.
On Fri, Nov 16, 2012 at 12:34 PM, Phil Sorber <phil@omniti.com> wrote:
-- Thanks for the review.That is strange. I will take a look to make sure I didn't do something silly.
On Thu, Nov 15, 2012 at 9:23 PM, Michael Paquier
<michael.paquier@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.
Don't worry, that is not a big deal. I might as well have something not correctly configured in my box.
Ok, so now 3 votes for this. I guess this is a desired feature. I'm
> 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.
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?
So other opinions?
> 3) Having an output close to what ping actually does would also be nice, theCould you be more specific? Are you saying you don't want to see
> current output like Accepting/Rejecting Connections are not that
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.
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.
This is desired and important behavior. It's not specific to pg_ping,
> 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?
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".We can't actually do this. It is important that it uses exit(3)
> 6) Please use exit(1) instead of exit(3) like the other script utilities.
(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
On Thu, Nov 15, 2012 at 10:55 PM, Michael Paquier <michael.paquier@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
Phil Sorber <phil@omniti.com> writes: > On Thu, Nov 15, 2012 at 10:55 PM, Michael Paquier > <michael.paquier@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. regards, tom lane
Attached is updated patch v4 with the changes Michael pointed out. On Fri, Nov 16, 2012 at 12:28 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Phil Sorber <phil@omniti.com> writes: >> On Thu, Nov 15, 2012 at 10:55 PM, Michael Paquier >> <michael.paquier@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
Attachment
On Thu, Nov 15, 2012 at 10:55 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Fri, Nov 16, 2012 at 12:34 PM, Phil Sorber <phil@omniti.com> wrote: >> On Thu, Nov 15, 2012 at 9:23 PM, Michael Paquier >> <michael.paquier@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
On Sat, Nov 17, 2012 at 2:48 AM, Phil Sorber <phil@omniti.com> wrote:
Michael PaquierOn Thu, Nov 15, 2012 at 10:55 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Fri, Nov 16, 2012 at 12:34 PM, Phil Sorber <phil@omniti.com> wrote:>> > 3) Having an output close to what ping actually does would also be nice,How does icmp_seq fit into this? Or was that an oversight?
>> > 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.
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?
--
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?
--
http://michael.otacoo.com
On Fri, Nov 16, 2012 at 2:28 PM, Tom Lane <tgl@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
On Sat, Nov 17, 2012 at 2:48 AM, Phil Sorber <phil@omniti.com> wrote:
-- On Thu, Nov 15, 2012 at 10:55 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Fri, Nov 16, 2012 at 12:34 PM, Phil Sorber <phil@omniti.com> wrote:>> > 3) Having an output close to what ping actually does would also be nice,How does icmp_seq fit into this? Or was that an oversight?
>> > 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.
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
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.paquier@gmail.com> wrote: > > > On Sat, Nov 17, 2012 at 2:48 AM, Phil Sorber <phil@omniti.com> wrote: >> >> On Thu, Nov 15, 2012 at 10:55 PM, Michael Paquier >> <michael.paquier@gmail.com> wrote: >> > On Fri, Nov 16, 2012 at 12:34 PM, Phil Sorber <phil@omniti.com> wrote: >> >> On Thu, Nov 15, 2012 at 9:23 PM, Michael Paquier >> >> <michael.paquier@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
On Mon, Nov 26, 2012 at 11:17 AM, Phil Sorber <phil@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
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.
On Tue, Nov 27, 2012 at 12:26 AM, Peter Eisentraut <peter_e@gmx.net> wrote:
-- On 11/23/12 9:48 AM, Michael Paquier wrote:Personally, I still don't see the general utility of this. For
> 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?
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
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 <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
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.
Peter Eisentraut <peter_e@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
On Tue, Nov 27, 2012 at 7:35 PM, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:
-- Peter Eisentraut <peter_e@gmx.net> writes:Rename the utility to pg_isready?
> 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
+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
On Tue, Nov 27, 2012 at 8:45 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > > > On Tue, Nov 27, 2012 at 7:35 PM, Dimitri Fontaine <dimitri@2ndquadrant.fr> > wrote: >> >> Peter Eisentraut <peter_e@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.
On Tue, Nov 27, 2012 at 9:43 AM, Phil Sorber <phil@omniti.com> wrote: > On Tue, Nov 27, 2012 at 8:45 AM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> >> >> On Tue, Nov 27, 2012 at 7:35 PM, Dimitri Fontaine <dimitri@2ndquadrant.fr> >> wrote: >>> >>> Peter Eisentraut <peter_e@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.
Attachment
On Sun, Dec 2, 2012 at 5:56 AM, Phil Sorber <span dir="ltr"><<a href="mailto:phil@omniti.com" target="_blank">phil@omniti.com</a>></span>wrote:<br /><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:00 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Here is the updated patch. I renamed it, but usingv5 to stay consistent.<br /></blockquote></div><br />After looking at this patch, I found the following problems:<br/>- There are a couple of whitespaces still in the code, particularly at the end of those lines<br />+ const char *pguser = NULL; <br /> + const char *pgdbname = NULL; <br />- When describing the values that arereturned by pg_isready, it is awkward to refer to the returning values as plain integers as those values are part of anenum. You should add references to PQPING_OK, PQPING_REJECT, PQPING_NO_RESPONSE and PQPING_NO_ATTEMPT instead. Also addto reference links in the docs redirecting to them, with things of the type <xref linkend="libpq-pqpingparams-pqping-ok">or related.<br /> - Same thing with this example:<br />+ <para><br />+ Standard Usage:<br />+ <screen><br />+ <prompt>$</prompt> <userinput>pg_isready</userinput><br/>+ <prompt>$</prompt> <userinput>echo $?</userinput><br/> + <computeroutput>0</computeroutput><br />+ </screen><br />+ </para><brclear="all" />For the time being PQPING_OK returns 0 because it is on top of the enum PGPing, but this mightchange if for a reason or another the order of outputs is changed.<br /><br />Once those things are fixed, I think thiswill be ready for committer review as everybody here seem to agree with your approach.<br />-- <br />Michael Paquier<br/><a href="http://michael.otacoo.com" target="_blank">http://michael.otacoo.com</a><br />
On Mon, Dec 3, 2012 at 11:59 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Sun, Dec 2, 2012 at 5:56 AM, Phil Sorber <phil@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> <userinput>pg_isready</userinput> > + <prompt>$</prompt> <userinput>echo $?</userinput> > + <computeroutput>0</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
Attachment
On 2012/12/05, at 14:46, Phil Sorber <phil@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
Phil Sorber escribió: > On Mon, Dec 3, 2012 at 11:59 PM, Michael Paquier > <michael.paquier@gmail.com> wrote: > > - Same thing with this example: > > + <para> > > + Standard Usage: > > + <screen> > > + <prompt>$</prompt> <userinput>pg_isready</userinput> > > + <prompt>$</prompt> <userinput>echo $?</userinput> > > + <computeroutput>0</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 <literal>0</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 Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Wed, Dec 5, 2012 at 8:53 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Phil Sorber escribió: >> On Mon, Dec 3, 2012 at 11:59 PM, Michael Paquier >> <michael.paquier@gmail.com> wrote: > >> > - Same thing with this example: >> > + <para> >> > + Standard Usage: >> > + <screen> >> > + <prompt>$</prompt> <userinput>pg_isready</userinput> >> > + <prompt>$</prompt> <userinput>echo $?</userinput> >> > + <computeroutput>0</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 > <literal>0</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 Herrera http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Training & Services
On Thu, Dec 6, 2012 at 12:29 AM, Phil Sorber <phil@omniti.com> wrote:
Thanks,+1 I'm on board with this.On Wed, Dec 5, 2012 at 8:53 AM, Alvaro Herrera <alvherre@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
> <literal>0</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.
OK. Let's do that and then mark this patch as ready for committer.
--
Michael Paquier
http://michael.otacoo.com
On Thu, Dec 6, 2012 at 8:54 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > > > On Thu, Dec 6, 2012 at 12:29 AM, Phil Sorber <phil@omniti.com> wrote: >> >> On Wed, Dec 5, 2012 at 8:53 AM, Alvaro Herrera <alvherre@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 >> > <literal>0</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.
Attachment
On Fri, Dec 7, 2012 at 12:56 PM, Phil Sorber <phil@omniti.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?Those changes have been made.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,
Cool. Thanks.
Something I was just thinking about while testing this again. Imentioned 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.
Except if you change the default behavior, let's change this patch status as ready to committer.
Thanks,
--
Michael Paquier
http://michael.otacoo.com
On Sat, Dec 8, 2012 at 7:50 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Fri, Dec 7, 2012 at 12:56 PM, Phil Sorber <phil@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
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.paquier@gmail.com> wrote: > > On Fri, Dec 7, 2012 at 12:56 PM, Phil Sorber <phil@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 <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On Wed, Dec 12, 2012 at 12:06 AM, Bruce Momjian <bruce@momjian.us> wrote:
-- On Sat, Dec 8, 2012 at 08:59:00AM -0500, Phil Sorber wrote:Right.
> On Sat, Dec 8, 2012 at 7:50 AM, Michael Paquier
> <michael.paquier@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.
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
On Wed, Dec 19, 2012 at 8:28 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > > > On Wed, Dec 12, 2012 at 12:06 AM, Bruce Momjian <bruce@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.paquier@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
Attachment
On Sat, Dec 22, 2012 at 4:07 AM, Phil Sorber <phil@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,
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
On Sun, Dec 23, 2012 at 9:29 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > > > On Sat, Dec 22, 2012 at 4:07 AM, Phil Sorber <phil@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
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
On Sun, Dec 23, 2012 at 9:57 AM, Erik Rijkers <er@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
On Sun, Dec 23, 2012 at 10:07 AM, Phil Sorber <phil@omniti.com> wrote: > On Sun, Dec 23, 2012 at 9:57 AM, Erik Rijkers <er@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.
Attachment
<br /><br /><div class="gmail_quote">On Mon, Dec 24, 2012 at 12:44 AM, Phil Sorber <span dir="ltr"><<a href="mailto:phil@omniti.com"target="_blank">phil@omniti.com</a>></span> wrote:<br /><blockquote class="gmail_quote" style="margin:00 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Updated patch attached.<br /></blockquote></div>Thanks.I am marking this patch as ready for committer.<br />-- <br />Michael Paquier<br /><a href="http://michael.otacoo.com"target="_blank">http://michael.otacoo.com</a>
On Tue, Dec 25, 2012 at 1:47 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > > > On Mon, Dec 24, 2012 at 12:44 AM, Phil Sorber <phil@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.
Attachment
On Fri, Jan 18, 2013 at 4:17 PM, Phil Sorber <phil@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
On Sun, Jan 20, 2013 at 8:40 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Fri, Jan 18, 2013 at 4:17 PM, Phil Sorber <phil@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
On Sun, Jan 20, 2013 at 9:58 AM, Phil Sorber <phil@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
On Sun, Jan 20, 2013 at 11:38 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Sun, Jan 20, 2013 at 9:58 AM, Phil Sorber <phil@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
On Sun, Jan 20, 2013 at 2:59 PM, Phil Sorber <phil@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
On 01/21/2013 11:26 AM, Robert Haas wrote: > On Sun, Jan 20, 2013 at 2:59 PM, Phil Sorber <phil@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 9999 psql: could not connect to server: No such file or directory Is the server running locally and accepting connectionson Unix domain socket "/tmp/.s.PGSQL.9999"? $ psql -h localhost -p 9999 psql: could not connect to server: Connection refused Is the server running on host "localhost" (::1) and accepting TCP/IP connections on port 9999? 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 9999? -- Craig Ringer http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services