Re: [PATCH] pgpassfile connection option

Hello Julian,

>> The documentation needs to be updated. 
> I've written a couple lines now.
> I aligned the definition of the connection option and the environment 
> variable with that of other (conn.opt&env.var.) pairs and added mention of 
> the different options to the doc of the "Password File".

Ok.

> [...] That was indeed an Error on my side, I hadn't updated the 
> errormessages to inform you which file has been used.
>
> So attached is an updated version of the patch.

Patch applies, compiles, "make check" ok (although the feature is not 
tested there).

Documentation compiled to html and looks ok.

I tested the feature with psql by resetting the dynamic library path.
The error message error in the previous review is fixed.

I have a problem with how the pass file name is managed: there
are three possible sources:
  1) pgpassfile connection string option
  2) PGPASSFILE environment variable
  3) the default (~/.pgpass or something else on windows)

Now function getPgPassFilename() is a misnomer, as it does not
always return the pass filename, but only in cases 2 and 3.

I think that PGPASSFILE is somehow checked twice: once explicitely
in getPgPassFilename and once because of the struct declaration
"pgpassfile". Once should be enough.

So I think some cleanup would be welcome. The mess is preexisting to your 
patch because the PGPASSFILE environment variable was managed differently 
from other options.

I would suggest that the struct gets the value (from option, environment 
or default) and is always used elsewhere. The getPgPassFilename function 
should disappear and PasswordFromFile should be simplified significantly.

> I'd like to ask for some input on how to handle invalid files - right 
> now no message is shown, the user just gets a password prompt as a 
> result, however I think a message when the custom pgpassfile hasn't been 
> found would be helpful.

I agree that it is currently unconvincing. I'm unclear about the correct 
level of messages that should be displayed for a library. I think that it 
should be pretty silent by default if all is well but that some 
environment variable should be able to put a more verbose level...

Libpq connection options seem not to be tested anywhere. There should be 
TAP tests in src/interfaces/libpq. This remark is independent of your 
patch.

-- 
Fabien.