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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
