Re: [PATCH] pgpassfile connection option

Hello Julian,

> I've updated my patch to work with the changes introduced to libpq by
> allowing multiple hosts.

Ok. Patch applies cleanly, compiles & checks (although yet again the
feature is not tested).

Feature tested and works for me, although I'm not sure how the multi-host
warning about pgpassfile works, but I could not find a wrong warning.

Independently of your patch, while testing I concluded that the multi-host
feature and documentation should be improved:

- If a connection fails, it does not say for which host/port.

sh> PGPASSFILE=$HOME/.pgpass.test \
LD_LIBRARY_PATH=$PWD/src/interfaces/libpq \
psql -d "host=host1,host2,host3 user=test dbname=test"
psql: FATAL: password authentication failed for user "test"
password retrieved from file "$HOME/.pgpass.test"

- doc says "If multiple host names are specified, each will be tried in
turn in the order given.".

In fact they are tried in turn if the network connection fails, but not
if the connection fails for some other reason such as the auth.
The documentation is not precise enough.

> On Fabien's recommendations, I've kept the variable dot_pgpassfile_used,
> however I renamed that to pgpassfile_used, to keep a consistent naming
> scheme.

Ok.

> I'm still not sure about the amount of error messages produced by libpq,
> I think it would be ideal to report an error while accessing a file
> provided in the connection string, however I would not report that same
> type of error when the .pgpass file has been tried to retrieve a
> password. (Else, you'd get an error message on every connection string
> that doesn't specify a pgpassfile or password, since .pgpass will be
> checked every time, before prompting the user to input the password)

Yep. This issue is independent of your patch as it is already there.
There could be a boolean set when the pass file is explicitely provided
that could trigger a warning only in this case.

A few detailed comments:

Beware of spacing:
. "if(" -> "if (" (2 instances)
. " =='\0')" -> " == '\0')" (at least one instance)

Elsewhere:

+ if (conn->pgpassfile_used && conn->password_needed && conn->result &&
+ conn->pgpassfile && conn->pgpassfile[0]!='\0' &&

ISTM that if pgpassfile_used is true then pgpassfile is necessarily
defined, so the second line tests are redundant? Or am I missing
something?

--
Fabien.