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.