Re: Patch: Implement failover on libpq connect level.

> I've tried to deal with some of these problems.
>
> My patch have support for following things:
>
> 1. Check whether database instance is in the recovery/standby mode and
> try to find another one if so.
> 2. Let cluster management software to have some time to promote one of
> the standbys to master. I.e. there can be failover timeout specified to
> allow retry after some time if no working master found.
>
> Really there is room for some improvements in handling of connect
> timeout (which became much more important thing when ability to try
> next host appears). Now it is handled only by blocking-mode connect
> functions, not by async state machine. But I decided to publish patch
> without these improvements to get feedback from community.
A bit of testing on this turns up a problem.

Consider a connection string that specifies two hosts and a read/write
connection:

postgresql://korry(at)127(dot)0(dot)0(dot)1:5301,127.0.0.1:5300/edb?readonly=0

If the first host is a healthy standby (meaning that I can connect to it
but pg_is_in_recovery() returns 't'), the state machine will never move
on to the second host.

The problem seems to be in PQconnectPoll() in the case for
CONNECTION_AUTH_OK, specifically this code:

/* We can release the address list now. */
pg_freeaddrinfo_all(conn->addrlist_family, conn->addrlist);
conn->addrlist = NULL;
conn->addr_cur = NULL;

That frees up the list of alternative host addresses. The state machine
then progresses to CONNECTION_CHECK_RO (which invokes
pg_is_in_recovery()), then CONNECTION_CHECK_RW (waiting for the response
from the server). Since we just connected to a standby replica,
pg_is_in_recovery() returns 't' and the state changes to
CONNECTION_NEEDED. The next call to try_next_address() will fail to
find a next address because we freed the list in the case for
CONNECTION_AUTH_OK.

A related issue: when the above sequence occurs, no error message is
returned (because the case for CONNECTION_NEEDED thinks "an appropriate
error message is already set up").

In short, if you successfully connect to standby replica (and specify
readonly=0), the remaining hosts are ignored, even though one of those
hosts is a master.

And one comment about the code itself - in connectDBStart(), you've
added quite a bit of code to parse multiple hosts/ports. I would
recommend adding a comment that shows the expected format, and then
choosing better variable names (better than 'p', 'q', and 'r'); perhaps
the variable names could refer to components of the connection string
that you are parsing (like 'host', 'port', 'delimiter', ...). That
would make the code much easier to read/maintain.

Thanks.

-- Korry