|From:||Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>|
|To:||Julian Markwort <julian(dot)markwort(at)uni-muenster(dot)de>|
|Cc:||PostgreSQL Developers <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: [PATCH] pgpassfile connection option|
|Views:||Raw Message | Whole Thread | Download mbox|
>> 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
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
|Next Message||Amit Kapila||2016-10-26 11:15:26||Re: 9.6, background worker processes, and PL/Java|
|Previous Message||Ashutosh Bapat||2016-10-26 11:12:20||Re: [RFC] Should we fix postmaster to avoid slow shutdown?|