Re: Support load balancing in libpq

Hi,

On Tue, Nov 29, 2022 at 02:57:08PM +0000, Jelte Fennema wrote:
> Attached is a new version with the tests cleaned up a bit (more
> comments mostly).
> 
> @Michael, did you have a chance to look at the last version? Because I
> feel that the patch is pretty much ready for a committer to look at,
> at this point.

I had another look; it still applies and tests pass. I still find the
distribution over three hosts a bit skewed (even when OpenSSL is
enabled, which wasn't the case when I first tested it), but couldn't
find a general fault and it worked well enough in my testing.

I wonder whether making the parameter a boolean will paint us into a
corner, and whether maybe additional modes could be envisioned in the
future, but I can't think of some right now (you can pretty neatly
restrict load-balancing to standbys by setting
target_session_attrs=standby in addition). Maybe a way to change the
behaviour if a dns hostname is backed by multiple entries?

Some further (mostly nitpicking) comments on the patch:

> From 6e20bb223012b666161521b5e7249c066467a5f3 Mon Sep 17 00:00:00 2001
> From: Jelte Fennema <github-tech@jeltef.nl>
> Date: Mon, 12 Sep 2022 09:44:06 +0200
> Subject: [PATCH v5] Support load balancing in libpq
> 
> Load balancing connections across multiple read replicas is a pretty
> common way of scaling out read queries. There are two main ways of doing
> so, both with their own advantages and disadvantages:
> 1. Load balancing at the client level
> 2. Load balancing by connecting to an intermediary load balancer
> 
> Both JBDC (Java) and Npgsql (C#) already support client level load
> balancing (option #1). This patch implements client level load balancing
> for libpq as well. To stay consistent with the JDBC and Npgsql part of
> the  ecosystem, a similar implementation and name for the option are
> used. 

I still think all of the above has no business in the commit message,
though maybe the first paragraph can stay as introduction.

> It contains two levels of load balancing:
> 1. The given hosts are randomly shuffled, before resolving them
>     one-by-one.
> 2. Once a host its addresses get resolved, those addresses are shuffled,
>     before trying to connect to them one-by-one.

That's fine.

What should be in the commit message is at least a mention of what the
new connection parameter is called and possibly what is done to
accomplish it.

But the committer will pick this up if needed I guess.

> diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
> index f9558dec3b..6ce7a0c9cc 100644
> --- a/doc/src/sgml/libpq.sgml
> +++ b/doc/src/sgml/libpq.sgml
> @@ -1316,6 +1316,54 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
>        </listitem>
>       </varlistentry>
>  
> +     <varlistentry id="libpq-load-balance-hosts" xreflabel="load_balance_hosts">
> +      <term><literal>load_balance_hosts</literal></term>
> +      <listitem>
> +       <para>
> +        Controls whether the client load balances connections across hosts and
> +        adresses. The default value is 0, meaning off, this means that hosts are
> +        tried in order they are provided and addresses are tried in the order
> +        they are received from DNS or a hosts file. If this value is set to 1,
> +        meaning on, the hosts and addresses that they resolve to are tried in
> +        random order. Subsequent queries once connected will still be sent to
> +        the same server. Setting this to 1, is mostly useful when opening
> +        multiple connections at the same time, possibly from different machines.
> +        This way connections can be load balanced across multiple Postgres
> +        servers.
> +       </para>
> +       <para>
> +        When providing multiple hosts, these hosts are resolved in random order.
> +        Then if that host resolves to multiple addresses, these addresses are
> +        connected to in random order. Only once all addresses for a single host
> +        have been tried, the addresses for the next random host will be
> +        resolved. This behaviour can lead to non-uniform address selection in
> +        certain cases. Such as when not all hosts resolve to the same number of
> +        addresses, or when multiple hosts resolve to the same address. So if you
> +        want uniform load balancing, this is something to keep in mind. However,
> +        non-uniform load balancing also has usecases, e.g. providing the
> +        hostname of a larger server multiple times in the host string so it gets
> +        more requests.
> +       </para>
> +       <para>
> +        When using this setting it's recommended to also configure a reasonable
> +        value for <xref linkend="libpq-connect-connect-timeout"/>. Because then,
> +        if one of the nodes that are used for load balancing is not responding,
> +        a new node will be tried.
> +       </para>
> +      </listitem>
> +     </varlistentry>

I think this whole section is generally fine, but needs some
copyediting.

> +     <varlistentry id="libpq-random-seed" xreflabel="random_seed">
> +      <term><literal>random_seed</literal></term>
> +      <listitem>
> +       <para>
> +        Sets the random seed that is used by <xref linkend="libpq-load-balance-hosts"/>
> +        to randomize the host order. This option is mostly useful when running
> +        tests that require a stable random order.
> +       </para>
> +      </listitem>
> +     </varlistentry>

I wonder whether this needs to be documented if it is mostly a
development/testing parameter?

> diff --git a/src/include/libpq/pqcomm.h b/src/include/libpq/pqcomm.h
> index fcf68df39b..39e93b1392 100644
> --- a/src/include/libpq/pqcomm.h
> +++ b/src/include/libpq/pqcomm.h
> @@ -27,6 +27,12 @@ typedef struct
>      socklen_t    salen;
>  } SockAddr;
>  
> +typedef struct
> +{
> +    int            family;
> +    SockAddr    addr;
> +}            AddrInfo;

That last line looks weirdly indented compared to SockAddr; in the
struct above.

>  /* Configure the UNIX socket location for the well known port. */
>  
>  #define UNIXSOCK_PATH(path, port, sockdir) \
> diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
> index f88d672c6c..b4d3613713 100644
> --- a/src/interfaces/libpq/fe-connect.c
> +++ b/src/interfaces/libpq/fe-connect.c
> @@ -241,6 +241,14 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
>          "Fallback-Application-Name", "", 64,
>      offsetof(struct pg_conn, fbappname)},
>  
> +    {"load_balance_hosts", NULL, NULL, NULL,
> +        "Load-Balance", "", 1,    /* should be just '0' or '1' */
> +    offsetof(struct pg_conn, loadbalance)},
> +
> +    {"random_seed", NULL, NULL, NULL,
> +        "Random-Seed", "", 10,    /* strlen(INT32_MAX) == 10 */
> +    offsetof(struct pg_conn, randomseed)},
> +
>      {"keepalives", NULL, NULL, NULL,
>          "TCP-Keepalives", "", 1,    /* should be just '0' or '1' */
>      offsetof(struct pg_conn, keepalives)},
> @@ -379,6 +387,7 @@ static bool fillPGconn(PGconn *conn, PQconninfoOption *connOptions);
>  static void freePGconn(PGconn *conn);
>  static void closePGconn(PGconn *conn);
>  static void release_conn_addrinfo(PGconn *conn);
> +static bool store_conn_addrinfo(PGconn *conn, struct addrinfo *addrlist);
>  static void sendTerminateConn(PGconn *conn);
>  static PQconninfoOption *conninfo_init(PQExpBuffer errorMessage);
>  static PQconninfoOption *parse_connection_string(const char *connstr,
> @@ -424,6 +433,9 @@ static void pgpassfileWarning(PGconn *conn);
>  static void default_threadlock(int acquire);
>  static bool sslVerifyProtocolVersion(const char *version);
>  static bool sslVerifyProtocolRange(const char *min, const char *max);
> +static int    loadBalance(PGconn *conn);
> +static bool parse_int_param(const char *value, int *result, PGconn *conn,
> +                            const char *context);
>  
>  
>  /* global variable because fe-auth.c needs to access it */
> @@ -1007,6 +1019,46 @@ parse_comma_separated_list(char **startptr, bool *more)
>      return p;
>  }
>  
> +/*
> + * Initializes the prng_state field of the connection. We want something
> + * unpredictable, so if possible, use high-quality random bits for the
> + * seed. Otherwise, fall back to a seed based on timestamp and PID.
> + */
> +static bool
> +libpq_prng_init(PGconn *conn)
> +{
> +    if (unlikely(conn->randomseed))
> +    {
> +        int            rseed;
> +
> +        if (!parse_int_param(conn->randomseed, &rseed, conn, "random_seed"))
> +        {
> +            return false;
> +        };

I think it's project policy to drop the braces for single statements in
if blocks.

> +        pg_prng_seed(&conn->prng_state, rseed);
> +    }
> +    else if (unlikely(!pg_prng_strong_seed(&conn->prng_state)))
> +    {
> +        uint64        rseed;
> +        time_t        now = time(NULL);
> +
> +        /*
> +         * Since PIDs and timestamps tend to change more frequently in their
> +         * least significant bits, shift the timestamp left to allow a larger
> +         * total number of seeds in a given time period.  Since that would
> +         * leave only 20 bits of the timestamp that cycle every ~1 second,
> +         * also mix in some higher bits.
> +         */
> +        rseed = ((uint64) getpid()) ^
> +            ((uint64) now << 12) ^
> +            ((uint64) now >> 20);
> +
> +        pg_prng_seed(&conn->prng_state, rseed);
> +    }
> +    return true;
> +}
> +
> +
>  /*

Additional newline.

> @@ -1164,6 +1217,36 @@ connectOptions2(PGconn *conn)
>          }
>      }
>  
> +    if (loadbalancehosts < 0)
> +    {
> +        appendPQExpBufferStr(&conn->errorMessage,
> +                             libpq_gettext("loadbalance parameter must be an integer\n"));
> +        return false;
> +    }
> +
> +    if (loadbalancehosts)
> +    {
> +        if (!libpq_prng_init(conn))
> +        {
> +            return false;
> +        }
> +
> +        /*
> +         * Shuffle connhost with a Durstenfeld/Knuth version of the
> +         * Fisher-Yates shuffle. Source:
> +         * https://en.wikipedia.org/wiki/Fisher%E2%80%93Yates_shuffle#The_modern_algorithm
> +         */
> +        for (i = conn->nconnhost - 1; i > 0; i--)
> +        {
> +            int            j = pg_prng_uint64_range(&conn->prng_state, 0, i);
> +            pg_conn_host temp = conn->connhost[j];
> +
> +            conn->connhost[j] = conn->connhost[i];
> +            conn->connhost[i] = temp;
> +        }
> +    }
> +
> +
>      /*

Additional newline.

> @@ -1726,6 +1809,27 @@ connectFailureMessage(PGconn *conn, int errorno)
>          libpq_append_conn_error(conn, "\tIs the server running on that host and accepting TCP/IP connections?");
>  }
>  
> +/*
> + * Should we load balance across hosts? Returns 1 if yes, 0 if no, and -1 if
> + * conn->loadbalance is set to a value which is not parseable as an integer.
> + */
> +static int
> +loadBalance(PGconn *conn)
> +{
> +    char       *ep;
> +    int            val;
> +
> +    if (conn->loadbalance == NULL)
> +    {
> +        return 0;
> +    }

Another case of additional braces.

> +    val = strtol(conn->loadbalance, &ep, 10);
> +    if (*ep)
> +        return -1;
> +    return val != 0 ? 1 : 0;
> +}
> +
> +
>  /*

Additional newline.

> @@ -4041,6 +4154,63 @@ freePGconn(PGconn *conn)
>      free(conn);
>  }
>  
> +
> +/*

Additional newline.

> + * Copies over the AddrInfos from addrlist to the PGconn.
> + */
> +static bool
> +store_conn_addrinfo(PGconn *conn, struct addrinfo *addrlist)
> +{
> +    struct addrinfo *ai = addrlist;
> +
> +    conn->whichaddr = 0;
> +
> +    conn->naddr = 0;
> +    while (ai)
> +    {
> +        ai = ai->ai_next;
> +        conn->naddr++;
> +    }
> +
> +    conn->addr = calloc(conn->naddr, sizeof(AddrInfo));
> +    if (conn->addr == NULL)
> +    {
> +        return false;
> +    }

Additional braces.

> @@ -4048,11 +4218,10 @@ freePGconn(PGconn *conn)
>  static void
>  release_conn_addrinfo(PGconn *conn)
>  {
> -    if (conn->addrlist)
> +    if (conn->addr)
>      {
> -        pg_freeaddrinfo_all(conn->addrlist_family, conn->addrlist);
> -        conn->addrlist = NULL;
> -        conn->addr_cur = NULL;    /* for safety */
> +        free(conn->addr);
> +        conn->addr = NULL;
>      }
>  }
>  
> diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
> index 512762f999..76ee988038 100644
> --- a/src/interfaces/libpq/libpq-int.h
> +++ b/src/interfaces/libpq/libpq-int.h
> @@ -82,6 +82,8 @@ typedef struct
>  #endif
>  #endif                            /* USE_OPENSSL */
>  
> +#include "common/pg_prng.h"
> +
>  /*
>   * POSTGRES backend dependent Constants.
>   */
> @@ -373,6 +375,8 @@ struct pg_conn
>      char       *pgpassfile;        /* path to a file containing password(s) */
>      char       *channel_binding;    /* channel binding mode
>                                       * (require,prefer,disable) */
> +    char       *loadbalance;    /* load balance over hosts */
> +    char       *randomseed;        /* seed for randomization of load balancing */
>      char       *keepalives;        /* use TCP keepalives? */

A bit unclear why you put the variables at this point in the list, but
the list doesn't seem to be ordered strictly anyway; still, maybe they
would fit best at the bottom below target_session_attrs?

>      char       *keepalives_idle;    /* time between TCP keepalives */
>      char       *keepalives_interval;    /* time between TCP keepalive
> @@ -461,8 +465,10 @@ struct pg_conn
>      PGTargetServerType target_server_type;    /* desired session properties */
>      bool        try_next_addr;    /* time to advance to next address/host? */
>      bool        try_next_host;    /* time to advance to next connhost[]? */
> -    struct addrinfo *addrlist;    /* list of addresses for current connhost */
> -    struct addrinfo *addr_cur;    /* the one currently being tried */
> +    int            naddr;            /* number of addrs returned by getaddrinfo */
> +    int            whichaddr;        /* the addr currently being tried */

Address(es) is always spelt out in the comments, those two addr(s)
should also I think.

> diff --git a/src/interfaces/libpq/t/003_loadbalance.pl b/src/interfaces/libpq/t/003_loadbalance.pl
> new file mode 100644
> index 0000000000..07eddbe9cc
> --- /dev/null
> +++ b/src/interfaces/libpq/t/003_loadbalance.pl
> @@ -0,0 +1,167 @@
> +# Copyright (c) 2022, PostgreSQL Global Development Group

Copyright bump needed.


Cheers,

Michael