Date: Fri, 27 Dec 2019 10:08:28 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: "tsunakawa.takay@fujitsu.com" <tsunakawa.takay@fujitsu.com>
Cc: Greg Nancarrow <gregn4422@gmail.com>,
	Haribabu Kommi <kommi.haribabu@gmail.com>,
	Robert Haas <robertmhaas@gmail.com>,
	Laurenz Albe <laurenz.albe@cybertec.at>,
	Tom Lane <tgl@sss.pgh.pa.us>, Dave Cramer <pg@fastcrypt.com>,
	Jing Wang <jingwangian@gmail.com>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>,
	Elvis Pranskevichus <elprans@gmail.com>
Subject: Re: Libpq support to connect to standby server as priority
Message-ID: <20191227130828.GA21647@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <OSAPR01MB5073A42A6540FB1861CFB35EFE2A0@OSAPR01MB5073.jpnprd01.prod.outlook.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On 2019-Dec-27, tsunakawa.takay@fujitsu.com wrote:

> From: Alvaro Herrera <alvherre@2ndquadrant.com>
> > 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.
> 
> That's because the distinction read/write is different from
> primary/standby.  If default_transaction_read_only is on, even the
> primary is read-only.  That's why the syntax target_session_attrs =
> {read-write | read-only} was introduced instead of target_server_type
> = {primary | standby}.  Personally, I only want target_server_type =
> {primary | standby | prefer-standby}, and discard target_session_attrs
> for simplicity of the functional specification and the code.

So, we can know whether server is primary/standby by checking
in_recovery, as opposed to knowing whether read-write which is done by
checking transaction_read_only.  So we can keep read-write as a synonym
for "primary", and check in_recovery when used in servers that support
the new GUC, and check transaction_read_only in older servers.

It seems there's a lot of code that we can discard from the patch:
first, we can discard checking for "read-only" altogether.  Second, have
us check transaction_read_only *only* if the server is of an older
version.

I would discard the whole thing about checking "SELECT pg_is_in_recovery()"
also; let's skip straight to checking SHOW in_recovery (patch 0003).
Let's not introduce a mechanism that ends up obsolete immediately.

By the same token, I propose we don't mark transaction_read_only as a
GUC_REPORT option, since we only do that to let it become obsolete
immediately.  If we connect to a server older than 13, just keep sending
the SHOW query.

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