Re: [PATCH] pgpassfile connection option
От | Fabien COELHO |
---|---|
Тема | Re: [PATCH] pgpassfile connection option |
Дата | |
Msg-id | alpine.DEB.2.20.1610261309120.9442@lancre обсуждение исходный текст |
Ответ на | Re: [PATCH] pgpassfile connection option (Julian Markwort <julian.markwort@uni-muenster.de>) |
Ответы |
Re: [PATCH] pgpassfile connection option
|
Список | pgsql-hackers |
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.
В списке pgsql-hackers по дате отправления: