Re: -d option for pg_isready is broken

On Tue, Nov 19, 2013 at 1:10 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Tue, Nov 19, 2013 at 11:51 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Fri, Nov 15, 2013 at 9:01 PM, Fabrízio de Royes Mello
>> <fabriziomello(at)gmail(dot)com> wrote:
>>> On Wed, Nov 13, 2013 at 9:37 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>>>> handyrep(at)john:~/handyrep$ pg_isready --version
>>>> pg_isready (PostgreSQL) 9.3.1
>>>>
>>>> handyrep(at)john:~/handyrep$ pg_isready -h john -p 5432 -U postgres -d
>>>> postgres -q
>>>> pg_isready: missing "=" after "postgres" in connection info string
>>>>
>>>> handyrep(at)john:~/handyrep$ pg_isready --host=john --port=5432
>>>> --user=postgres --dbname=postgres
>>>> pg_isready: missing "=" after "postgres" in connection info string
>>>>
>>>> handyrep(at)john:~/handyrep$ pg_isready -h john -p 5432 -U postgres
>>>> john:5432 - accepting connections
>>>>
>>>> so apparently the -d option:
>>>>
>>>> a) doesn't work, and
>>>> b) doesn't do anything
>>>>
>>>> I suggest simply removing it from the utility.
>>>>
>>>> I'll note that the -U option doesn't appear to do anything relevant
>>>> either, but at least it doesn't error unnecessarily:
>>>>
>>>> handyrep(at)john:~/handyrep$ pg_isready -h john -p 5432 -U no_such_user
>>>> john:5432 - accepting connections
>>>>
>>>
>>> The attached patch fix it.
>>
>> Well, there still seem to be some problems.
>>
>> [rhaas pgsql]$ src/bin/scripts/pg_isready -d host=sgdasasg
>> /tmp:5432 - no attempt
>>
>> I added some debugging instrumentation and it looks like there's a
>> problem with this code:
>>
>> if (strcmp(def->keyword, "hostaddr") == 0 ||
>> strcmp(def->keyword, "host") == 0)
>>
>> The problem is that we end up executing the code contained in that
>> block twice, once for host and once for hostaddr. Thus:
>>
>> [rhaas pgsql]$ src/bin/scripts/pg_isready -d host=sgdasasg
>> def->keyword=host pghost_str=sgdasasg
>> def->keyword=hostaddr pghost_str=/tmp
>> def->keyword=port pgport_str=5432
>> /tmp:5432 - no attempt
>>
>> Oops.
>>
>> Nevertheless, that's kind of a separate problem, so we could address
>> in a separate commit. But even ignoring that, this still isn't right:
>> according to the fine documentation, -d will be expanded not only if
>> it contains an =, but also if it starts with a valid URI prefix. This
>> would break that documented behavior.
>>
>> http://www.postgresql.org/docs/9.3/static/app-pg-isready.html
>
> Attached is the updated version of the patch. Is there any other bug?

Not that I can see, but it's not very future-proof. If libpq changes
its idea of what will provoke database-name expansion, this will again
be broken.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company