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,host3user=test dbname=test"  psql: FATAL:  password authentication failed for user "test"  password
retrievedfrom 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
reasonsuch 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.