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.