Re: [WIP] pg_ping utility

On Fri, Nov 16, 2012 at 12:34 PM, Phil Sorber <phil(at)omniti(dot)com> wrote:

> Thanks for the review.
>
> On Thu, Nov 15, 2012 at 9:23 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)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