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