Re: [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.

On 13 October 2012 22:19, Phil Sorber <phil(at)omniti(dot)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(at)omniti(dot)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(at)2ndquadrant(dot)com> wrote:
> On Monday, October 15, 2012 11:28:36 PM Thom Brown wrote:
>> On 13 October 2012 22:19, Phil Sorber <phil(at)omniti(dot)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(at)2ndquadrant(dot)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(at)postgresql(dot)org [mailto:pgsql-hackers-
> owner(at)postgresql(dot)org] On Behalf Of Tom Lane
> Sent: Monday, October 15, 2012 7:13 PM
> To: Andres Freund
> Cc: pgsql-hackers(at)postgresql(dot)org; Thom Brown; Phil Sorber
> Subject: Re: [HACKERS] [WIP] pg_ping utility
>
> Andres Freund <andres(at)2ndquadrant(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Andres Freund <andres(at)2ndquadrant(dot)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(at)yahoo(dot)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(at)omniti(dot)com> writes:
> On Mon, Oct 15, 2012 at 7:12 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Phil Sorber <phil(at)omniti(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Phil Sorber <phil(at)omniti(dot)com> writes:
>> On Mon, Oct 15, 2012 at 7:12 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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.

Phil Sorber <phil(at)omniti(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Phil Sorber <phil(at)omniti(dot)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.

On 10/22/12 11:47 AM, Phil Sorber wrote:
> On Sun, Oct 21, 2012 at 6:20 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Phil Sorber <phil(at)omniti(dot)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(at)gmx(dot)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(at)gmx(dot)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.

On Tue, Oct 23, 2012 at 6:22 PM, Christopher Browne <cbbrowne(at)gmail(dot)com> wrote:
> On Tue, Oct 23, 2012 at 6:12 PM, Peter Eisentraut <peter_e(at)gmx(dot)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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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,

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.

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.
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.
3) Having an output close to what ping actually does would also be nice,
the current output like Accepting/Rejecting Connections are not that
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?
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 */
6) Please use exit(1) instead of exit(3) like the other script utilities.

Thanks,
--
Michael Paquier
http://michael.otacoo.com

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.

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

On Thu, Nov 15, 2012 at 10:55 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)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(at)omniti(dot)com> writes:
> On Thu, Nov 15, 2012 at 10:55 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Phil Sorber <phil(at)omniti(dot)com> writes:
>> On Thu, Nov 15, 2012 at 10:55 PM, Michael Paquier
>> <michael(dot)paquier(at)gmail(dot)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

On Thu, Nov 15, 2012 at 10:55 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Fri, Nov 16, 2012 at 12:34 PM, Phil Sorber <phil(at)omniti(dot)com> wrote:
>> On Thu, Nov 15, 2012 at 9:23 PM, Michael Paquier
>> <michael(dot)paquier(at)gmail(dot)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(at)omniti(dot)com> wrote:

> On Thu, Nov 15, 2012 at 10:55 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
> > On Fri, Nov 16, 2012 at 12:34 PM, Phil Sorber <phil(at)omniti(dot)com> wrote:
> >> On Thu, Nov 15, 2012 at 9:23 PM, Michael Paquier
> >> <michael(dot)paquier(at)gmail(dot)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?
>
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?
--
Michael Paquier
http://michael.otacoo.com

On Fri, Nov 16, 2012 at 2:28 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)omniti(dot)com> wrote:

> On Thu, Nov 15, 2012 at 10:55 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
> > On Fri, Nov 16, 2012 at 12:34 PM, Phil Sorber <phil(at)omniti(dot)com> wrote:
> >> On Thu, Nov 15, 2012 at 9:23 PM, Michael Paquier
> >> <michael(dot)paquier(at)gmail(dot)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

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(dot)paquier(at)gmail(dot)com> wrote:
>
>
> On Sat, Nov 17, 2012 at 2:48 AM, Phil Sorber <phil(at)omniti(dot)com> wrote:
>>
>> On Thu, Nov 15, 2012 at 10:55 PM, Michael Paquier
>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> > On Fri, Nov 16, 2012 at 12:34 PM, Phil Sorber <phil(at)omniti(dot)com> wrote:
>> >> On Thu, Nov 15, 2012 at 9:23 PM, Michael Paquier
>> >> <michael(dot)paquier(at)gmail(dot)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(at)omniti(dot)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(at)gmx(dot)net> 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.
>
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(at)momjian(dot)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(at)gmx(dot)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(at)2ndquadrant(dot)fr>wrote:

> Peter Eisentraut <peter_e(at)gmx(dot)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.
--
Michael Paquier
http://michael.otacoo.com

On Tue, Nov 27, 2012 at 8:45 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
>
>
> On Tue, Nov 27, 2012 at 7:35 PM, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>
> wrote:
>>
>> Peter Eisentraut <peter_e(at)gmx(dot)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(at)omniti(dot)com> wrote:
> On Tue, Nov 27, 2012 at 8:45 AM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>
>>
>> On Tue, Nov 27, 2012 at 7:35 PM, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>
>> wrote:
>>>
>>> Peter Eisentraut <peter_e(at)gmx(dot)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.

On Sun, Dec 2, 2012 at 5:56 AM, Phil Sorber <phil(at)omniti(dot)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;
- 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.
- 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.

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

On Mon, Dec 3, 2012 at 11:59 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Sun, Dec 2, 2012 at 5:56 AM, Phil Sorber <phil(at)omniti(dot)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

On 2012/12/05, at 14:46, Phil Sorber <phil(at)omniti(dot)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(dot)paquier(at)gmail(dot)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(at)2ndquadrant(dot)com> wrote:
> Phil Sorber escribió:
>> On Mon, Dec 3, 2012 at 11:59 PM, Michael Paquier
>> <michael(dot)paquier(at)gmail(dot)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(at)omniti(dot)com> wrote:

> On Wed, Dec 5, 2012 at 8:53 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)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,
--
Michael Paquier
http://michael.otacoo.com

On Thu, Dec 6, 2012 at 8:54 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
>
>
> On Thu, Dec 6, 2012 at 12:29 AM, Phil Sorber <phil(at)omniti(dot)com> wrote:
>>
>> On Wed, Dec 5, 2012 at 8:53 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)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.

On Fri, Dec 7, 2012 at 12:56 PM, Phil Sorber <phil(at)omniti(dot)com> wrote:

> 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,
>
> Those changes have been made.
>
Cool. Thanks.

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

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(dot)paquier(at)gmail(dot)com> wrote:
> On Fri, Dec 7, 2012 at 12:56 PM, Phil Sorber <phil(at)omniti(dot)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(dot)paquier(at)gmail(dot)com> wrote:
> > On Fri, Dec 7, 2012 at 12:56 PM, Phil Sorber <phil(at)omniti(dot)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(at)momjian(dot)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(at)momjian(dot)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(dot)paquier(at)gmail(dot)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.
--
Michael Paquier
http://michael.otacoo.com

On Wed, Dec 19, 2012 at 8:28 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
>
>
> On Wed, Dec 12, 2012 at 12:06 AM, Bruce Momjian <bruce(at)momjian(dot)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(dot)paquier(at)gmail(dot)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

On Sat, Dec 22, 2012 at 4:07 AM, Phil Sorber <phil(at)omniti(dot)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,
--
Michael Paquier
http://michael.otacoo.com

On Sun, Dec 23, 2012 at 9:29 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
>
>
> On Sat, Dec 22, 2012 at 4:07 AM, Phil Sorber <phil(at)omniti(dot)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(at)xs4all(dot)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(at)postgresql(dot)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(at)omniti(dot)com> wrote:
> On Sun, Dec 23, 2012 at 9:57 AM, Erik Rijkers <er(at)xs4all(dot)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(at)postgresql(dot)org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-hackers

Updated patch attached.

On Mon, Dec 24, 2012 at 12:44 AM, Phil Sorber <phil(at)omniti(dot)com> wrote:

> Updated patch attached.
>
Thanks. I am marking this patch as ready for committer.
--
Michael Paquier
http://michael.otacoo.com

On Tue, Dec 25, 2012 at 1:47 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
>
>
> On Mon, Dec 24, 2012 at 12:44 AM, Phil Sorber <phil(at)omniti(dot)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.

On Fri, Jan 18, 2013 at 4:17 PM, Phil Sorber <phil(at)omniti(dot)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(at)gmail(dot)com> wrote:
> On Fri, Jan 18, 2013 at 4:17 PM, Phil Sorber <phil(at)omniti(dot)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(at)omniti(dot)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(at)gmail(dot)com> wrote:
> On Sun, Jan 20, 2013 at 9:58 AM, Phil Sorber <phil(at)omniti(dot)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(at)omniti(dot)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(at)omniti(dot)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
connections on 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