Joe Conway <mail@joeconway.com> writes:
> If "=" is found in the dbname psql argument, the argument is assumed to
> be a conninfo string. In that case, append application_name to the
> conninfo and use PQsetdbLogin() as before. Otherwise use the new
> PQconnectdbParams().
This seems bogus on a couple of levels. First off, I thought the idea
was to get away from using PQsetdbLogin at all. If we go down this path
we'll never be rid of it. Second, to preserve backwards compatibility
we will have to duplicate this same type of logic in any of the other
places we want to replace PQsetdbLogin (because it's actually
PQsetdbLogin that implements the dbname-containing-equal-sign special
case in prior releases). I count nine remaining calls of that function
between bin/ and contrib/, at least some of which were supposed to get
application_name-ified before release.
While I'm looking at this, there's another bogosity in the original
patch: it neglected the PQsetdbLogin call in psql/command.c, meaning
that doing \c would result in losing the application_name setting.
I'm not entirely sure about a better way to do it, but this approach
seems rather unsatisfactory.
> Also only uses static assignments for array constructors.
Uh, no, you're still depending on a non-static constructor for the
keywords[] array. I'm not at all certain whether my portability
concern is still valid in 2010, but if it is, this doesn't fix it.
This doesn't do very much to help with the maintenance risk about
keeping the two arrays in step, either --- now there's neither a
direct nor a visual matchup between the entries. I'd suggest
something like
keywords[0] = "host";
values[0] = options.host;
keywords[1] = "port";
values[1] = options.port;
...
regards, tom lane
