On Sun, Mar 25, 2018 at 12:56 AM, Michael Paquier <michael@paquier.xyz> wrote:
On Sat, Mar 24, 2018 at 01:49:28AM +1100, Haribabu Kommi wrote: > Here I attached the updated patch that returns either the connected > host/hostaddr > or NULL in case if the connection is not established. > > I removed the returning default host details, because the default host > details are also available with the connhost member itself.
Thanks for the review.
As shaped, PQhost generates complains from gcc with -Wreturn-type. So I would suggest to return NULL for the default code path. As far as I can see from the code, PGconn->connhost cannot be NULL and it should have at least one "host" or "hostaddr" defined, so I think that we could consider adding an assertion about that and comment out this cannot normally be reached.
Ok. Added an assert with an explanation comment.
If the connection is bad, then whichhost points to 0, which would cause PQhost to return the first host or hostaddr value. I think that we should document properly to not trust the value of PQhost if the status is CONNECTION_BAD, or to return NULL if the connection is bad as this would have no real value for multiple hosts. I am a bit afraid of potential breakages if we do that, so the first method may make the most sense.
The existing behavior is currently returning wrong value when the connection
status is CONNECTION_BAD. As we are changing the behavior of the function,
it may be better to handle the CONNECTION_BAD scenario also instead of
providing note in the manual?
The same things apply to PQport as multiple ports can be defined. Thoughts?
Yes, I changed PQport also to return the connected port or NULL,
Removed the returning of all the ports specified in the connection string.
I have quickly looked at the callers of PQhost in the core code and those seem safe. Something to keep in mind.
More details in the documentation would be nice. Let's detail the following: - PQhost returns NULL if the connection is not established yet. - PQhost may return an incorrect value when with CONNECTION_BAD as status. - If both hostaddr and host are precised in the conneciton string, then host has the priority. - If only hostaddr is precised, then this is the value returned.
Docs are updated with the new behavior of the functions.
Updated patch attached with behavior of returning NULL for connections of
