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