Re: Libpq support to connect to standby server as priority

On 2019-Oct-01, Greg Nancarrow wrote:

> On Wed, Sep 11, 2019 at 10:17 AM Alvaro Herrera from 2ndQuadrant
> <alvherre@alvh.no-ip.org> wrote:
> >
> > Oh, oops. Here they are then.
> 
> With the permission of the original patch author, Haribabu Kommi, I’ve
> rationalized the existing 8 patches into 3 patches, merging patches
> 1-5 and 6-7, and tidying up some documentation and code comments. I
> also rebased them to the latest PG12 source code (as of October 1,
> 2019). The patch code itself is the same, except for some version
> checks that I have updated to target the features for PG13 instead of
> PG12.

I've spent some time the last few days going over these patches and the
prior discussion.

I'm not sure I understand why we end up with "prefer-read" in addition
to "prefer-standby" (and similar seeming redundancy between "primary"
and "read-write").  Do we really need more than one way to identify
hosts' roles?  It seems 0001 adds the "prefer-read" modes by checking
transaction_read_only, and later 0002 adds the "prefer-standby" modes by
checking in_recovery.  I'm not sure that we're serving our users very
well by giving them choice that ends up being confusing.  In other words
I think we should do only one of these things, not both.  Maybe merge
0001 and 0002 in a single patch, and get rid of redundant modes.

There were other comments that I think went largely unaddressed, such as
the point that the JDBC driver seems to offer a different syntax for the
configuration, and should we offer a compatibility shim of some sort.
(Frankly, I don't think we need to stress over this too much, but it
seems that it wasn't even discussed.)

0003 contains parts written by Elvis Pranskevichus.  It would be good to
confirm that he is satisfied with how the whole thing ends up working.

Also, Ishii-san said:
https://postgr.es/m/20190116.150236.2304777214520289427.t-ishii@sraoss.co.jp
  - When looking for a primary, find a node where pg_is_in_recovery is
    false; if none, libpq should retry until a timeout expires.  Did we
    reject this idea altogether, or is it just unimplemented?


Looking at 0001, I would move the new "desired connection mode" to
libpq-int.h (from libpq-fe.h), and rename like this

/* Desired connection type */
typedef enum
{
    TGT_CONN_TYPE_ANY = 0,        /* Any session (default) */
    TGT_CONN_TYPE_READ_WRITE,    /* Read-write session */
    TGT_CONN_TYPE_PREFER_READ,    /* Prefer read only session */
    TGT_CONN_TYPE_READ_ONLY        /* Read only session */
} TargetConnectionType;

The name of the label "consume_checked_write_connection" is not very
descriptive.  I propose "conn_succeeded" instead.

"read_write_host_index" seems a very unimaginative struct member name.
Following "whichhost" I propose to rename this to "which_rw_host", and
rewrite its comment to something like this:

    /*
     * Status indicator for read-write host.  The initial value of -1
     * indicates that we don't know which server is the read-write one; a
     * non-negative number (set as soon as we discover one) indicates which
     * server is the read-write one; -2 indicates that the server being tested
     * (whichhost???) is the read-write one.
     */
    int            which_rw_host;

(I'm not sure that the explanation for value -2 is correct. Please
rewrite that if it isn't.)


I think the if/then/else maze in the CONNECTION_CHECK_TARGET case in
PQconnectPoll() is a nigh unreadable rat's nest after these patches.
Maybe some extra states in the state machine are needed; and probably
that would be helped by some small subroutines to reduce the
duplication.  PQconnectPoll is already 1700 lines long; our job is not
made easier by making it 2000 lines long.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services