Re: Patch: Implement failover on libpq connect level.

On Sun, 25 Sep 2016 17:31:53 +0530
Mithun Cy <mithun.cy@enterprisedb.com> wrote:

> I have some more comments on libpq-failover-8.patch
> 
> + /* FIXME need to check that port is numeric */
> 
> Is this still applicable?.
> 

Unfortunately, it was. I've fixed this problem in 9-th version of patch
(attached)
> 
> I think we need comments to know why we change default value based on
> number of elements in connection string. why default in not “any"
> when node count > 1.

Fixed.

> 
> + /* loop over all the host specs in the node variable */
> 
> + for (node = nodes; node->host != NULL || node->port != NULL; node++)
> 
>   {
> 
> I think instead of loop if we can move these code into a separate
> function say pg_add_to_addresslist(host, port, &addrs) this helps in
> removing temp variables like "struct node info” and several heap
> calls around it.

For some reason DNS resolving was concentrated in one place before my
changes. So, I've tried to not change this decision.

 
> 3)
> 
> +static char *
> 
> +get_port_from_addrinfo(struct addrinfo * ai)
> 
> 
> Need some comments for this function.

Done.
 
> We use strdup in many places no where we handle memory allocation
> failure.

Added some more memory allocation error checks.
> 
> Comments not in sink with code.

Fixed.