Re: [WIP] pg_ping utility - Mailing list pgsql-hackers

On Fri, Jan 18, 2013 at 4:17 PM, Phil Sorber <phil@omniti.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.

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.

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.

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