Re: -d option for pg_isready is broken

On Wed, Nov 20, 2013 at 3:12 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Nov 19, 2013 at 1:10 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> On Tue, Nov 19, 2013 at 11:51 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>> On Fri, Nov 15, 2013 at 9:01 PM, Fabrízio de Royes Mello
>>> <fabriziomello@gmail.com> wrote:
>>>> On Wed, Nov 13, 2013 at 9:37 PM, Josh Berkus <josh@agliodbs.com> wrote:
>>>>> handyrep@john:~/handyrep$ pg_isready --version
>>>>> pg_isready (PostgreSQL) 9.3.1
>>>>>
>>>>> handyrep@john:~/handyrep$ pg_isready -h john -p 5432 -U postgres -d
>>>>> postgres -q
>>>>> pg_isready: missing "=" after "postgres" in connection info string
>>>>>
>>>>> handyrep@john:~/handyrep$ pg_isready --host=john --port=5432
>>>>> --user=postgres --dbname=postgres
>>>>> pg_isready: missing "=" after "postgres" in connection info string
>>>>>
>>>>> handyrep@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@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.

Yeah, I agree that we should make the logic of pg_isready more future-proof
in 9.4dev. One idea is to expose internal_ping() as a libpq function. Then,
instead of just calling PQpingParams(), we can do PQconnectStartParams(),
libpq-function-version-of-internal_ping(), PQhost(), PQport(), and PQfinish().
That is, we get to know the host and port information by calling
PQhost() and PQport(),
after trying to connect to the server.

But we cannot use this idea in 9.3 because it's too late to add new
libpq function there.
Also I don't think that the minor version release would change that
libpq's logic in 9.3.
So, barring any objections, I will commit the latest version of the
patch in 9.3.

Regards,

--
Fujii Masao