Re: [PATCH] pg_isready (was: [WIP] pg_ping utility)

On Wed, Jan 23, 2013 at 2:51 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> On Wed, Jan 23, 2013 at 02:50:01PM -0500, Phil Sorber wrote:
>> On Wed, Jan 23, 2013 at 1:58 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>> > On Wed, Jan 23, 2013 at 12:27:45PM -0500, Tom Lane wrote:
>> >> Phil Sorber <phil(at)omniti(dot)com> writes:
>> >> > On Wed, Jan 23, 2013 at 11:07 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> >> >> [rhaas pgsql]$ pg_isready -h www.google.com
>> >> >> <grows old, dies>
>> >>
>> >> > Do you think we should have a default timeout, or only have one if
>> >> > specified at the command line?
>> >>
>> >> +1 for default timeout --- if this isn't like "ping" where you are
>> >> expecting to run indefinitely, I can't see that it's a good idea for it
>> >> to sit very long by default, in any circumstance.
>> >
>> > FYI, the pg_ctl -w (wait) default is 60 seconds:
>> >
>> > from pg_ctl.c:
>> >
>> > #define DEFAULT_WAIT 60
>> >
>>
>> Great. That is what I came to on my own as well. Figured that might be
>> a sticking point, but as there is precedent, I'm happy with it.
>
> Yeah, being able to point to precedent is always helpful.
>
> --
> Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
> EnterpriseDB http://enterprisedb.com
>
> + It's impossible for everything to be true. +

Attached is the patch to add a connect_timeout.

I also factored out the setting of user and dbname from the default
gathering loop as we only need host and port defaults and made more
sense to handle user/dbname in the same area of the code as
connect_timeout. This was something mentioned by Robert upthread.

This also coincidentally fixes a bug in the size of the keywords and
values arrays. Must have added an option during review and not
extended that array. Is there a best practice to making sure that
doesn't happen in the future? I was thinking define MAX_PARAMS and
then setting the array size to MAX_PARAMS+1 and then checking in the
getopt loop to see how many params we expect to process and erroring
if we are doing to many, but that only works at runtime.

One other thing I noticed while refactoring the defaults gathering
code. If someone passes in a connect string for dbname, we output the
wrong info at the end. This is not addressed in this patch because I
wanted to get some feedback before fixing and make a separate patch. I
see the only real option being to use PQconninfoParse to get the
params from the connect string. If someone passes in a connect string
via dbname should that have precedence over other values passed in via
individual command line options? Should ordering of the command line
options matter?

For example if someone did: pg_isready -h foo -d "host=bar port=4321" -p 1234

What should the connection parameters be?