Re: [WIP] pg_ping utility

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.

>
> 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.