Message-ID: <360dcd4eff43a302e04148abca0c94f56a233df9.camel@cybertec.at>
Subject: Re: Libpq support to connect to standby server as priority
From: Laurenz Albe <laurenz.albe@cybertec.at>
To: Haribabu Kommi <kommi.haribabu@gmail.com>, Robert Haas
	 <robertmhaas@gmail.com>
Cc: Jing Wang <jingwangian@gmail.com>, Pg Hackers
 <pgsql-hackers@postgresql.org>,  "Tsunakawa, Takayuki"
 <tsunakawa.takay@jp.fujitsu.com>
Date: Fri, 05 Oct 2018 16:34:34 +0200
In-Reply-To: 
 <CAJrrPGcMq9YgG-aMzLK051EndUgjdnpp8_dJwJaNDZbu=sS0qQ@mail.gmail.com>
References: 
	<CAF3+xM+8-ztOkaV9gHiJ3wfgENTq97QcjXQt+rbFQ6F7oNzt9A@mail.gmail.com>
	 <CAJrrPGfzSD4kmedadcDx+3vckj_kJ_jAuufgi+NsemTkWSU5Tw@mail.gmail.com>
	 <1530710078.2411.29.camel@cybertec.at>
	 <CA+TgmobfBaNaMrT5Qx+nQ=JKYYy-VOZ2xxXAhAGUSSWZ=fkm4g@mail.gmail.com>
	 <CAJrrPGea1hX5HmZyVzhJ3HbT8x3+2zoNp48HhPVFzQUYy5PU_w@mail.gmail.com>
	 <CAJrrPGcMq9YgG-aMzLK051EndUgjdnpp8_dJwJaNDZbu=sS0qQ@mail.gmail.com>
Organization: Cybertec
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

Haribabu Kommi wrote:
> On Thu, Jul 19, 2018 at 10:59 PM Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
> > On Wed, Jul 18, 2018 at 10:53 PM Robert Haas <robertmhaas@gmail.com> wrote:
> > > On Wed, Jul 4, 2018 at 9:14 AM, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
> > > >   What about keeping the first successful connection open and storing it in a
> > > >   variable if we are in "prefer-read" mode.
> > > >   If we get the read-only connection we desire, close that cached connection,
> > > >   otherwise use it.
> > > 
> > > I like this idea.  If I recall correctly, the logic in this area is
> > > getting pretty complex, so we might need to refactor it for better
> > > readability and maintainability.
> > 
> > OK. I will work on the code refactoring first and then provide the
> > prefer-read option on top it.
> 
> commits d1c6a14bacf and 5ca00774194 have refactored the logic
> of handling the different connection states.
> 
> Attached is a rebased patch after further refactoring the new option
> code for easier maintenance.

The code is much more readable now, thanks.

--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -397,6 +397,7 @@ struct pg_conn
    int         nconnhost;      /* # of hosts named in conn string */
    int         whichhost;      /* host we're currently trying/connected to */
    pg_conn_host *connhost;     /* details about each named host */
+   int         read_write_host_index; /* index for first read-write host in connhost */
 
    /* Connection data */
    pgsocket    sock;           /* FD for socket, PGINVALID_SOCKET if

I think the comment could use more love.

This would be the place to document the logic:
Initial value is -1, then then index of the first working server
we found, and -2 for the second attempt to connect to that server.


I notice that you don't keep the first connection open, but close
and reopen it.  I guess that is a matter of taste, but it would be
easier on resources (and reduce connection time) if the connection
were kept open.
Admittedly, it would be more difficult and might further complicate
code that is not very clear as it is.

If you work on some more on the comment above, I will mark it as
ready for committer.

Yours,
Laurenz Albe