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

On Thu, Feb 7, 2013 at 2:14 AM, Phil Sorber <phil(at)omniti(dot)com> wrote:
> 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?

No maybe. But I think that all the client commands should follow the
same rule. Otherwise a user would get confused when specifying
options.

Regards,

--
Fujii Masao