Re: [WIP] pg_ping utility

On Sun, Jan 20, 2013 at 8:40 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Jan 18, 2013 at 4:17 PM, Phil Sorber <phil(at)omniti(dot)com> wrote:
>> Updated patch is rebased against current master and copyright year is updated.
>
> I took a look at this. According to the documentation for
> PQpingParams: "It accepts connection parameters identical to those of
> PQconnectdbParams, described above. It is not, however, necessary to
> supply correct user name, password, or database name values to obtain
> the server status." The -U option therefore seems quite useless
> except as a source of user confusion, and -d is only useful in that
> you could instead pass a connections string. That is definitely a
> useful thing to be able to do, but calling the option -d for database
> might be viewed as confusing. Or, it might be viewed as consistent
> with other commands, if you tilt your head just the right way.
>
> I would be tempted to change the syntax synopsis of the command to
> pg_isready [OPTIONS]... [CONNSTR] and delete -d and -U from the list
> of options, along the way that psql already works, but making
> allowance for the fact that database and username are apparently not
> relevant.
>

This was done to silence useless error messages in the logs. If you
attempt to connect as some user that does not exist, or to some
database that does not exist, it throws an error in the logs, even
with PQping. You could fix it with env vars, but since the point is to
change the user/database that we were connecting as, I figured it
should be consistent with all the other methods to do that.

> I am also a little bit baffled by the loop that begins here:
>
> + while (conn_opt_ptr->keyword)
>
> It appears to me that what this loop does is iterate through all of
> the possible connection options and then, when we get to the host,
> port, user, or dbname options, add them to the "keywords" and "values"
> arrays. But... what do we get out of iterating through all of the
> other options, then? It seems to me that you could dispense with the
> loop and just keep the stuff that actually adds the non-default values
> to the arrays:
>
> + if (pghost != NULL)
> + {
> + keywords[opt_index] = "host";
> + values[opt_index] = pghost;
> + opt_index++;
> + }
> + if (pgport != NULL)
> + {
> + keywords[opt_index] = "port";
> + values[opt_index] = pgport;
> + opt_index++;
> + }
> + if (pgconnstr != NULL)
> + {
> + keywords[opt_index] = "dbname";
> + values[opt_index] = pgconnstr;
> + opt_index++;
> + }
>
> Maybe there's some purpose to this I'm not seeing, but from here the
> loop looks like an unnecessary frammish.

I use this to find the defaults if they don't pass anything in, so I
know what to put in the status message at the end. I could devise my
own way to come up with those values as I have seen in some other
code, but I thought it was better to ask libpq directly what defaults
it was going to use.

>
> Finally, I think there should be a check that the user hasn't supplied
> more command-line arguments than we know what to do with, similar to
> this:
>
> [rhaas pgsql]$ vacuumdb foo bar baz
> vacuumdb: too many command-line arguments (first is "bar")
> Try "vacuumdb --help" for more information.
>
> That error message text seems like it might not have been given
> sufficient thought, but for purposes of this patch it's probably
> better to copy it than to invent something new.
>

I had not considered this. I will take a look and provide an updated patch.

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