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