On Mon, Mar 26, 2018 at 4:17 PM, Michael Paquier <mich...@paquier.xyz> wrote:
> On Mon, Mar 26, 2018 at 11:28:41AM +0900, Kyotaro HORIGUCHI wrote: > > At Sun, 25 Mar 2018 22:27:09 +1100, Haribabu Kommi < > kommi.harib...@gmail.com> wrote in <CAJrrPGeaQxWsU3RZ0yxGa=WY+ > wna+nve_ypukzbdaqrmut9...@mail.gmail.com> > >> On Sun, Mar 25, 2018 at 12:56 AM, Michael Paquier <mich...@paquier.xyz> > >> wrote: > Thanks for the review. > > As the commit message of 50cb21f70, the function is intended not > > to return NULL in order to prevent the user functions from a > > crash in corner cases. > > The commit number is not correct here. You mean 40cb21f. > > > https://www.postgresql.org/message-id/19297.1493660213%40sss.pgh.pa.us > > I quite like the idea of using an empty string value in those cases. > This could prevent crashes at leat for applications not doing NULL-ness > checks. > I also agree to return an empty string. I did this only for the cases where the conn is not NULL but the status is not proper or the connhost is NULL. > > Since the > > purpose of PGhost is not strictly defined, especially on > > connection failure, returning the given host list can be another > > candidate (and the same can be said for returning ""). I think > > users shouldn't (and I believe no one does) rely on the values > > from the functions when CONNECTION_BAD, anyway. > > Yeah, this should really be documented and also should refer to the fact > that this happens when specifying multiple hosts. > Added. > > My opinion is to add a description that is saying like "these > > functions return unreliable values for a failed connection". > > At the same time I don't think that this is sufficient either, because > for multiple hosts, PQhost() just returns the first one, which makes > absolutely no sense because the value is wrong. So I think that using a > third, separate value has some advantages: > - If NULL, this just means that the initialization did not happen. > - If using an empty string, then the value cannot be evaluated. > - If this returns a host or hostaddr (if host has not been specified), > then that's the host which is actually used for the connection. > Having those three states has value for applications in my opinion. > > The same can apply to PQport, or any other functions which for whatever > reason add support for multiple values like host, hostaddr or port. > I hope that I updated the documentation properly to explain all the above cases. > >> Updated patch attached with behavior of returning NULL for connections > of > >> CONNECTION_BAD status. > > > > The patch does Assert() in PQhost. I suppose that Assert() in > > client library is usable only when no more (library's) operation > > cannot continue. It would be better to return a fallback value in > > this criteria. > > The patch has to return a value as well. A shaped the patch causes > again compilation warnings because not all code paths have a return > value. So my previous remark has not been fixed. Hari, what do you use > as compiler, my gcc blows a warning and reading the patch that's > obviously incorrect. > In my assert enabled build, I didn't get any warning. Yes that patch to fix the warning is clearly wrong. I corrected in a different way of adding Assert checks for the hostaddr, because definitely host or hostaddr must be present. > + PQHost returns NULL when the connection is not established or > In the docs, this is wrong for two reasons: > - There should be a <function> markup. > - The name of the function is PQhost, not PGHost. > Corrected. Updated patch attached. Regards, Hari Babu Fujitsu Australia
PQhost-to-return-active-connected-host-and-hostaddr_v5.patch
Description: Binary data
