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

On Wed, Feb 6, 2013 at 11:36 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Thu, Feb 7, 2013 at 1:15 AM, Phil Sorber <phil(at)omniti(dot)com> wrote:
>> On Wed, Feb 6, 2013 at 11:11 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>> On Thu, Feb 7, 2013 at 12:05 AM, Phil Sorber <phil(at)omniti(dot)com> wrote:
>>>> On Tue, Feb 5, 2013 at 12:44 PM, Phil Sorber <phil(at)omniti(dot)com> wrote:
>>>>> On Tue, Feb 5, 2013 at 9:08 AM, Phil Sorber <phil(at)omniti(dot)com> wrote:
>>>>>> On Tue, Feb 5, 2013 at 9:06 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
>>>>>>> Phil Sorber escribió:
>>>>>>>> On Tue, Feb 5, 2013 at 6:41 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>>>>>>> > On Sat, Feb 2, 2013 at 9:55 PM, Phil Sorber <phil(at)omniti(dot)com> wrote:
>>>>>>>> >> OK, here is the patch that handles the connection string in dbname.
>>>>>>>> >> I'll post the other patch under a different posting because I am sure
>>>>>>>> >> it will get plenty of debate on it's own.
>>>>>>>> >
>>>>>>>> > I'm sorry, can you remind me what this does for us vs. the existing coding?
>>>>>>>> >
>>>>>>>>
>>>>>>>> It's supposed to handle the connection string passed as dbname case to
>>>>>>>> be able to get the right output for host:port.
>>>>>>>
>>>>>>> Surely the idea is that you can also give it a postgres:// URI, right?
>>>>>>
>>>>>> Absolutely.
>>>>>
>>>>> Here is it. I like this approach more than the previous one, but I'd
>>>>> like some feedback.
>>>
>>> The patch looks complicated to me. I was thinking that we can address
>>> the problem
>>> just by using PQconninfoParse() and PQconndefaults() like uri-regress.c does.
>>> The patch should be very simple. Why do we need so complicated code?
>>
>> Did you like the previous version better?
>>
>> http://www.postgresql.org/message-id/CADAkt-hnB3OhCpkR+PCg1c_Bjrsb7J__BPV+-jrjS5opjR2oww@mail.gmail.com
>
> Yes because that version is simpler. But which version is better depends on
> the reason why you implemented new version. If you have some idea about
> the merit and demerit of each version, could you elaborate them?

I didn't like the way that I had to hard code the options in the first
one as you pointed out below. I also was looking through the code for
something else and saw that a lot of the apps were starting with
defaults then building from there, rather than trying to add the
defaults at the end. I think they were still doing it wrong because
they were using getenv() on their own rather than asking libpq for the
defaults though. So the new version gets the defaults at the beginning
and also makes it easy to add new params without changing function
definitions.

>
> + set_connect_options(connect_options, &pgdbname, &pghost,
> &pgport, &connect_timeout, &pguser);
>
> This code prevents us from giving options other than the above, for example
> application_name, in the conninfo. I think that pg_isready should accept all
> the libpq options.
>

I'm with you there. The new version fixes that as well.

> When more than one -d options are specified, psql always prefer the last one
> and ignore the others. OTOH, pg_isready with this patch seems to merge them.
> I'm not sure if there is specific rule about the priority order of -d
> option. But
> it seems better to follow the existing way, i.e., always prefer the
> last -d option.
>

The problem I am having here is resolving the differences between
different -d options and other command line options. For example:

-h foo -p 4321 -d "host=bar port=1234" -d "host=baz"

I would expect that to be 'baz:1234' but you are saying it should be 'baz:4321'?

I look at -d as just a way to pass in multiple options (when you
aren't strictly passing in dbname) and should be able to expand the
above example to:

-h foo -p 4321 -h bar -p 1234 -h baz

If we hold off on parsing the value of -d until the end so we are sure
we have the last one, then we might lose other parameters that were
after the -d option. For example:

-h foo -p 4321 -d "host=bar port=1234" -d "host=baz user=you" -U me

Should this be 'me(at)baz:1234' or 'you(at)baz:4321' or 'me(at)baz:4321'?

So we would have to track the last instance of a parameter as well as
the order those final versions came in. Sound to me like there is no
clear answer there, but maybe there is a project consensus that has
already been reached with regard to this? Or some general computing
wisdom that applies?

> Regards,
>
> --
> Fujii Masao