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.


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)


 + 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?


Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to