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".


[...] 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.


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

Reply via email to