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
(~/.pgpassor 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.
