Re: Patch: Implement failover on libpq connect level.

Hello, Victor.

> As community shows an interest with this patch, and it has been
> included in the current commitfest, I've to submit it.

Naturally we show interest in this patch. It's a great feature that
would be very nice to have!

> This version of patch includes
> 
> 1. Most important - a tap test suite (really small enough and has a
> room for improvements).
> 
> 2. Compatibility with older versions - now readonly check is skipped if
> only one hostname is specified in the connect string
> 
> 3. pg_host and pg_port variables now show host name and port library
> was connected to.
> 
> 4. Your fix with compilation of ecpg tests.
> 
> Patch is attached.

After a brief examination I've found following ways to improve the
patch.

1) It looks like code is not properly formatted.

2)

> +      <term><literal>readonly</literal></term>
> +      <listitem>
> +      <para>
> +      If this parameter is 0 (the default), upon successful connection
> +      library checks if host is in recovery state, and if it is so, 
> +      tries next host in the connect string. If this parameter is
> +      non-zero, connection to warm standby nodes are considered
> +      successful.

IIRC the are "warm" and "hot" standby's in PostgreSQL terminology. The
difference is that you can't execute any queries on a "warm" standby. So
the description is probably wrong. I suggest to fix it to just "...
connection to standby nodes are...".

3)

> +     <term><literal>falover_timeout</literal></term>
> +     <listitem>
> +     <para>
> +     Maximum time to cycilically retry all the hosts in commect string.
> +     (as decimal integer number of seconds). If not specified, then
> +     hosts are tried just once.
> +     </para>

Typos: cycilically, commect

4) 

>  static int 
>  connectDBStart(PGconn *conn)
>  {
> +       struct nodeinfo
> +       {
> +               char       *host;
> +               char       *port;
> +       };

Not sure if all compilers we support can parse this. I suggest to
declare nodinfo structure outside of procedure, just to be safe.

5)

> +               nodes = calloc(sizeof(struct nodeinfo), 2); 
> +               nodes->host = strdup(conn->pghostaddr);
>                 hint.ai_family = AF_UNSPEC;

> +               /* Parse comma-separated list of host-port pairs into
> +                  function-local array of records */
> +               nodes = malloc(sizeof(struct nodeinfo) * 4); 


>                 /* pghostaddr and pghost are NULL, so use Unix domain
>  * socket */
> -               node = NULL;
> +               nodes = calloc(sizeof(struct nodeinfo), 2); 

> +       sprintf(port,"%d", htons(((struct sockaddr_in
> *)ai->ai_addr)->sin_port));
> +       return strdup(port);

You should check return values of malloc, calloc and strdup procedures,
or use safe pg_* equivalents.

6) 

> +                       for (p = addrs; p->ai_next != NULL; p =
>   p->ai_next);
> +                       p->ai_next = this_node_addrs;

Looks a bit scary. I suggest to add an empty scope ( {} ) and a comment
to make our intention clear here.

7) Code compiles with following warnings (CLang 3.8):

> 1 warning generated.
> fe-connect.c:5239:39: warning: if statement has empty body
> [-Wempty-body]
>                                                                    errorMessage,
> false, true));
>                                                                                               ^
> fe-connect.c:5239:39: note: put the semicolon on a separate line to
> silence this warning
> 1 warning generated.

8) get_next_element procedure implementation is way too smart (read
"complicated"). You could probably just store current list length and
generate a random number between 0 and length-1.

-- 
Best regards,
Aleksander Alekseev