Thread: [HACKERS] [PATCH v1] Add and report the new "in_hot_standby" GUC pseudo-variable.

Hi,

Currently, clients wishing to know when the server exits hot standby
have to resort to polling, which is often suboptimal.

This adds the new "in_hot_standby" GUC variable that is reported via a
ParameterStatus message.  This allows the clients to:

   (a) know right away that they are connected to a server in hot
       standby; and

   (b) know immediately when a server exits hot standby.

This change will be most beneficial to various connection pooling
systems (pgpool etc.)


                              Elvis


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sat, Mar 18, 2017 at 2:56 AM, Elvis Pranskevichus <elprans@gmail.com> wrote:
> Currently, clients wishing to know when the server exits hot standby
> have to resort to polling, which is often suboptimal.
>
> This adds the new "in_hot_standby" GUC variable that is reported via a
> ParameterStatus message.  This allows the clients to:
>
>    (a) know right away that they are connected to a server in hot
>        standby; and
>
>    (b) know immediately when a server exits hot standby.
>
> This change will be most beneficial to various connection pooling
> systems (pgpool etc.)

Why adding a good chunk of code instead of using pg_is_in_recovery(),
which switches to false once a server exits recovery?
-- 
Michael

On Saturday, March 18, 2017 3:33:16 AM EDT Michael Paquier wrote:
> On Sat, Mar 18, 2017 at 2:56 AM, Elvis Pranskevichus 
<elprans@gmail.com> wrote:
> > Currently, clients wishing to know when the server exits hot standby
> > have to resort to polling, which is often suboptimal.
> > 
> > This adds the new "in_hot_standby" GUC variable that is reported via
> > a> 
> > ParameterStatus message.  This allows the clients to:
> >    (a) know right away that they are connected to a server in hot
> >    
> >        standby; and
> >    
> >    (b) know immediately when a server exits hot standby.
> > 
> > This change will be most beneficial to various connection pooling
> > systems (pgpool etc.)
> 
> Why adding a good chunk of code instead of using pg_is_in_recovery(),
> which switches to false once a server exits recovery?

That requires polling the database continuously, which may not be 
possible or desirable.  

My main motivation here is to gain the ability to manage a pool of 
connections in asyncpg efficiently.  A part of the connection release 
protocol is "UNLISTEN *;", which the server in Hot Standby would fail to 
process.  Polling the database for pg_is_in_recovery() is not feasible 
in this case, unfortunately.
                            Elvis  

On 3/17/17 13:56, Elvis Pranskevichus wrote:
> Currently, clients wishing to know when the server exits hot standby
> have to resort to polling, which is often suboptimal.
> 
> This adds the new "in_hot_standby" GUC variable that is reported via a
> ParameterStatus message.

The terminology chosen here is not very clear.  What is the opposite of
"in hot standby"?  Warm standby?  Cold standby?  Not standby at all?
Promoted to primary (writable)?

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tuesday, March 21, 2017 11:50:38 PM EDT Peter Eisentraut wrote:
> On 3/17/17 13:56, Elvis Pranskevichus wrote:
> > Currently, clients wishing to know when the server exits hot standby
> > have to resort to polling, which is often suboptimal.
> > 
> > This adds the new "in_hot_standby" GUC variable that is reported via
> > a ParameterStatus message.
> 
> The terminology chosen here is not very clear.  What is the opposite
> of "in hot standby"?  Warm standby?  Cold standby?  Not standby at
> all? Promoted to primary (writable)?

The opposite means primary.  I can flip the GUC name to "is_primary", if 
that's clearer.
                              Elvis

On Wed, Mar 22, 2017 at 8:25 AM, Elvis Pranskevichus <elprans@gmail.com> wrote:
> On Tuesday, March 21, 2017 11:50:38 PM EDT Peter Eisentraut wrote:
>> On 3/17/17 13:56, Elvis Pranskevichus wrote:
>> > Currently, clients wishing to know when the server exits hot standby
>> > have to resort to polling, which is often suboptimal.
>> >
>> > This adds the new "in_hot_standby" GUC variable that is reported via
>> > a ParameterStatus message.
>>
>> The terminology chosen here is not very clear.  What is the opposite
>> of "in hot standby"?  Warm standby?  Cold standby?  Not standby at
>> all? Promoted to primary (writable)?
>
> The opposite means primary.  I can flip the GUC name to "is_primary", if
> that's clearer.

Hmm, I don't find that clearer.  "hot standby" has a very specific
meaning; "primary" isn't vague, but I would say it's less specific.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 18 March 2017 at 14:01, Elvis Pranskevichus <elprans@gmail.com> wrote:
> On Saturday, March 18, 2017 3:33:16 AM EDT Michael Paquier wrote:
>>
>> Why adding a good chunk of code instead of using pg_is_in_recovery(),
>> which switches to false once a server exits recovery?
>
> That requires polling the database continuously, which may not be
> possible or desirable.
>
> My main motivation here is to gain the ability to manage a pool of
> connections in asyncpg efficiently.  A part of the connection release
> protocol is "UNLISTEN *;", which the server in Hot Standby would fail to
> process.  Polling the database for pg_is_in_recovery() is not feasible
> in this case, unfortunately.
>

Sorry, i still don't understand the motivation for this.
At one point you're going to poll for the value of the GUC in pg_settings, no?
Or how are you going to know the current value of the GUC that makes it
different to just poll for pg_is_in_recovery()?

-- 
Jaime Casanova                      www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wednesday, March 22, 2017 2:17:27 PM EDT Jaime Casanova wrote:
> On 18 March 2017 at 14:01, Elvis Pranskevichus <elprans@gmail.com> wrote:
> > On Saturday, March 18, 2017 3:33:16 AM EDT Michael Paquier wrote:
> >> Why adding a good chunk of code instead of using
> >> pg_is_in_recovery(),
> >> which switches to false once a server exits recovery?
> > 
> > That requires polling the database continuously, which may not be
> > possible or desirable.
> > 
> > My main motivation here is to gain the ability to manage a pool of
> > connections in asyncpg efficiently.  A part of the connection
> > release
> > protocol is "UNLISTEN *;", which the server in Hot Standby would
> > fail to process.  Polling the database for pg_is_in_recovery() is
> > not feasible in this case, unfortunately.
> 
> Sorry, i still don't understand the motivation for this.
> At one point you're going to poll for the value of the GUC in
> pg_settings, no? Or how are you going to know the current value of
> the GUC that makes it different to just poll for pg_is_in_recovery()?

It is marked as GUC_REPORT and is reported by the server on 
connection and on every change:

https://www.postgresql.org/docs/current/static/protocol-flow.html#PROTOCOL-ASYNC
                           Elvis

On 3/22/17 14:09, Robert Haas wrote:
>> The opposite means primary.  I can flip the GUC name to "is_primary", if
>> that's clearer.
> Hmm, I don't find that clearer.  "hot standby" has a very specific
> meaning; "primary" isn't vague, but I would say it's less specific.

The problem I have is that there is already a GUC named "hot_standby",
which determines whether an instance is in hot (as opposed to warm?)
mode if it is a standby.  This is proposing to add a setting named
"in_hot_standby" which says nothing about the hotness, but something
about the standbyness.  Note that these are all in the same namespace.

I think we could use "in_recovery", which would be consistent with
existing naming.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Mar 22, 2017 at 4:00 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 3/22/17 14:09, Robert Haas wrote:
>>> The opposite means primary.  I can flip the GUC name to "is_primary", if
>>> that's clearer.
>> Hmm, I don't find that clearer.  "hot standby" has a very specific
>> meaning; "primary" isn't vague, but I would say it's less specific.
>
> The problem I have is that there is already a GUC named "hot_standby",
> which determines whether an instance is in hot (as opposed to warm?)
> mode if it is a standby.  This is proposing to add a setting named
> "in_hot_standby" which says nothing about the hotness, but something
> about the standbyness.  Note that these are all in the same namespace.

Good point.

> I think we could use "in_recovery", which would be consistent with
> existing naming.

True.

(Jaime's question is also on point, I think.)

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wednesday, March 22, 2017 4:28:18 PM EDT Robert Haas wrote:
> On Wed, Mar 22, 2017 at 4:00 PM, Peter Eisentraut 
> > I think we could use "in_recovery", which would be consistent with
> > existing naming.
> 
> True.

Ironically, that was the name I originally used.  I'll update the patch.
> (Jaime's question is also on point, I think.)

The main (and only) point of this patch is to avoid polling.  Since 
"in_recovery" is marked as GUC_REPORT, it will be sent to the client 
asynchronously in a ParamStatus message.  Other GUC_REPORT variables set 
a good precedent.

My argument is that Hot Standby is a major mode of operation, which 
significantly alters how connected clients work with the server, and 
this bit of knowledge is no less important than the other GUC_REPORT 
vars.
                             Elvis

On Thursday, March 23, 2017 4:50:20 AM EDT Magnus Hagander wrote:
> We should probably consider if there is some way we can implement
> these two things the same way. If we're inventing a new variable that
> gets pushed on each connection, perhaps we can use that one and avoid
> the SHOW command? Is the read-write thing really interesting in the
> aspect of the general case, or is it more about detectinv readonly
> standbys as well? Or to flip that, would sending the
> transaction_read_only parameter be enough for the usecase in this
> thread, without having to invent a new variable?

Hot standby differs from regular read-only transactions in a number of 
ways.  Most importantly, it prohibits LISTEN/NOTIFY.  Grep for 
PreventCommandDuringRecovery() if you're interested in the scope.
                          Elvis

On Thu, Mar 23, 2017 at 4:50 AM, Magnus Hagander <magnus@hagander.net> wrote:
> One thing we might want to consider around this -- in 10 we have
> target_session_attrs=read-write (since
> 721f7bd3cbccaf8c07cad2707826b83f84694832), which will issue a SHOW
> transaction_read_only on the connection.
>
> We should probably consider if there is some way we can implement these two
> things the same way. If we're inventing a new variable that gets pushed on
> each connection, perhaps we can use that one and avoid the SHOW command?

I think that would be a good idea.  It was, in fact, proposed to do
exactly that as part of the patch that added
target_session_attrs=read-write, but we ended up not doing anything
about it because the SHOW mechanism would still be needed when
connecting to pre-10 versions of PostgreSQL.  Therefore, it seemed
like a separate improvement.  But if we're adding a GUC_REPORT value
that could be used for the same or a similar purpose, I think it would
make sense to consider revising that mechanism to leverage it as well,
obviously only on releases that have the GUC.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Apr 10, 2017 at 5:47 PM, Magnus Hagander <magnus@hagander.net> wrote:
> Based on that we seem to agree here, should we add this as an open item?
> Clearly if we want to change this, we should do so before 10.

This really is a new feature, so as the focus is to stabilize things I
think that we should not make the code more complicated because...

> I also came up with another case where the current one won't work but it
> could be really useful -- if we make a replication connection (with say
> pg_receivewal) it would be good to be able to say "i want the master" (or "i
> want a standby") the same way. And that will fail today if it needs SHOW to
> work, right?
>
> So having it send that information across in the startup package when
> talking to a 10 server, but falling back to using SHOW if talking to an
> earlier server, would make a lot of sense I think.

Of this reason, as libpq needs to be compliant with past server
versions as well we are never going to save a set of version-dependent
if/else code to handle target_session_attrs properly using either a
SHOW or a new mechanism.
-- 
Michael

On 11 April 2017 at 09:05, Magnus Hagander <magnus@hagander.net> wrote:
> On Tue, Apr 11, 2017 at 3:26 AM, Michael Paquier <michael.paquier@gmail.com>
> wrote:
>>
>> On Mon, Apr 10, 2017 at 5:47 PM, Magnus Hagander <magnus@hagander.net>
>> wrote:
>> > Based on that we seem to agree here, should we add this as an open item?
>> > Clearly if we want to change this, we should do so before 10.
>>
>> This really is a new feature, so as the focus is to stabilize things I
>> think that we should not make the code more complicated because...
>
>
> The part I'm talking about is the potential adjustment of the patch that's
> already committed. That's not a new feature, that's exactly the sort of
> thing we'd want to adjust before we get to release. Because once released we
> really can't change it.

I agree if we introduce target_session_attrs it would be better to
have a complete feature in one release.

It does seem quite strange to have target_session_attrs=read-write
but not target_session_attrs=read-only

And it would be even better to have these session attrs as well notify-on-promote - sent when standby is promoted
notify-on-write- sent when an xid is assigned
 

"notify-on-promotion" being my suggested name for the feature being
discussed here. In terms of the feature as submitted, I wonder whether
having a GUC parameter like this makes sense, but I think its ok for
us to send a protocol message, maybe a NotificationResponse, but there
isn't any material difference between those two protocol messages.

Rather than the special case code in the patch, I imagine more generic
code like this...

if (sessionInterruptPending) ProcessSessionInterrupt();

I'm happy to work on the patch, if that's OK.

-- 
Simon Riggs                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Apr 11, 2017 at 4:05 AM, Magnus Hagander <magnus@hagander.net> wrote:
> On Tue, Apr 11, 2017 at 3:26 AM, Michael Paquier <michael.paquier@gmail.com> wrote:
>> On Mon, Apr 10, 2017 at 5:47 PM, Magnus Hagander <magnus@hagander.net>
>> wrote:
>> > Based on that we seem to agree here, should we add this as an open item?
>> > Clearly if we want to change this, we should do so before 10.
>>
>> This really is a new feature, so as the focus is to stabilize things I
>> think that we should not make the code more complicated because...
>
> The part I'm talking about is the potential adjustment of the patch that's
> already committed. That's not a new feature, that's exactly the sort of
> thing we'd want to adjust before we get to release. Because once released we
> really can't change it.

I don't really agree.  I think if we go and install a GUC_REPORT GUC
now, we're much less likely to flush out the bugs in the 'show
transaction_read_only' mechanism.  Also, now that I think about, the
reason why we settled on 'show transaction_read_only' against
alternate queries is because there's some ability for the DBA to make
that return 'false' rather than 'true' even when not in recovery, so
that if for example you are using logical replication rather than
physical replication, you have a way to make
target_session_attrs=read-write still do something useful.  If you add
an in_hot_standby GUC that's used instead, you lose that.

Now, we can decide what we want to do about that, but I don't see that
a change in this area *must* go into v10.  Maybe the answer is that
target_session_attrs grows additional values like 'primary' and
'standby' alongside 'read-write' (and Simon's suggested 'read-only').
Or maybe we have another idea.  But I don't really see the urgency of
whacking this around right this minute.  There's nothing broken here;
there's just more stuff people would like to have.  It can be added
next time around.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hello,

I didn't realize that my target_session_attrs naming proposal was committed.  I didn't think half way it would be
adopted,because the name is less intuitive than the original target_server_type, and is different from the PgJDBC's
targetServerType.


From: pgsql-hackers-owner@postgresql.org
> [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Simon Riggs
> I agree if we introduce target_session_attrs it would be better to have
> a complete feature in one release.
> 
> It does seem quite strange to have
>   target_session_attrs=read-write
> but not
>   target_session_attrs=read-only

I totally agree.  I'm a bit disappointed with and worried about the current situation.  I could easily imagine that
peoplearound me would say a stern opinion on the specification...
 

I think these are necessary in descending order of priority, if based on target_session_attrs:

[PG10]
1. target_session_attrs=read-only
This is mainly to connect to the standby.  People will naturally expect that this is available, because PostgreSQL
provideshot standby feature, and other DBMSs and even PgJDBC has the functionality.
 

2. Make transaction_read_only GUC_REPORT
This is to avoid the added round-trip by SHOW command.  It also benefits client apps that want to know when the server
getspromoted?  And this may simplify the libpq code.
 
I don't understand yet why we need to provide this feature for older servers by using SHOW.  Those who are already
using<= 9.6 in production completed the system or application, and their business is running.  Why would they want to
justreplace libpq and use this feature?
 

[PG 11]
3. target_session_attrs=prefer-read-only
This is mainly to prefer standbys, but allows to connect to the primary if no standby is available.  Honestly, this is
alsorequired in PG 10 because PgJDBC already provides this by "preferSlave".
 



From: pgsql-hackers-owner@postgresql.org
> [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Robert Haas
> On Tue, Apr 11, 2017 at 4:05 AM, Magnus Hagander <magnus@hagander.net>
> wrote:
> > The part I'm talking about is the potential adjustment of the patch
> > that's already committed. That's not a new feature, that's exactly the
> > sort of thing we'd want to adjust before we get to release. Because
> > once released we really can't change it.
> 
> I don't really agree.  I think if we go and install a GUC_REPORT GUC now,
> we're much less likely to flush out the bugs in the 'show
> transaction_read_only' mechanism.  

I'm sorry I couldn't get this part (maybe purely English nuance.  Are you concerned about some bugs?  We can't do
anythingif we fear of bugs.  Is it OK instead to make transaction_read_only GUC_REPORT?
 


> Also, now that I think about, the reason
> why we settled on 'show transaction_read_only' against alternate queries
> is because there's some ability for the DBA to make that return 'false'
> rather than 'true' even when not in recovery, so that if for example you
> are using logical replication rather than physical replication, you have
> a way to make target_session_attrs=read-write still do something useful.
> If you add an in_hot_standby GUC that's used instead, you lose that.

Agreed.  Again, is this satisfied by GUC_REPORTing transaction_read_only as well?


> Now, we can decide what we want to do about that, but I don't see that a
> change in this area *must* go into v10.  Maybe the answer is that
> target_session_attrs grows additional values like 'primary' and 'standby'
> alongside 'read-write' (and Simon's suggested 'read-only').
> Or maybe we have another idea.  But I don't really see the urgency of
> whacking this around right this minute.  There's nothing broken here;
> there's just more stuff people would like to have.  It can be added next
> time around.

But if completeness of the functionality is below people's expectations, it may unnecessarily compromise the reputation
ofPostgreSQL.
 

Is there any chance to incorporate a patch into PG 10?  May I add this as a PG 10 open item?


Regards
Takayuki Tsunakawa

On 13 April 2017 at 14:59, Tsunakawa, Takayuki
<tsunakawa.takay@jp.fujitsu.com> wrote:

> 2. Make transaction_read_only GUC_REPORT
> This is to avoid the added round-trip by SHOW command.  It also benefits client apps that want to know when the
servergets promoted?  And this may simplify the libpq code. 
> I don't understand yet why we need to provide this feature for older servers by using SHOW.  Those who are already
using<= 9.6 in production completed the system or application, and their business is running.  Why would they want to
justreplace libpq and use this feature? 

I think "transaction_read_only" is a bit confusing for something we're
expecting to change under us.

To me, a "read only" xact is one created with

BEGIN READ ONLY TRANSACTION;

.... which I would not expect to become read/write under me, since I
explicitly asked for read-only.

It's more like "session read only" that we're interested in IMO.

From: pgsql-hackers-owner@postgresql.org
> [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Craig Ringer
> On 13 April 2017 at 14:59, Tsunakawa, Takayuki
> <tsunakawa.takay@jp.fujitsu.com> wrote:
> 
> > 2. Make transaction_read_only GUC_REPORT This is to avoid the added
> > round-trip by SHOW command.  It also benefits client apps that want to
> know when the server gets promoted?  And this may simplify the libpq code.
> > I don't understand yet why we need to provide this feature for older servers
> by using SHOW.  Those who are already using <= 9.6 in production completed
> the system or application, and their business is running.  Why would they
> want to just replace libpq and use this feature?
> 
> I think "transaction_read_only" is a bit confusing for something we're
> expecting to change under us.
> 
> To me, a "read only" xact is one created with
> 
> BEGIN READ ONLY TRANSACTION;
> 
> .... which I would not expect to become read/write under me, since I
> explicitly asked for read-only.
> 
> It's more like "session read only" that we're interested in IMO.

Are you suggest thating we provide a GUC_REPORT read-only variable "session_read_only" and the libpq should use it?

Anyway, I added this item in the PostgreSQL 10 Open Items page under
"Design Decisions to Recheck Mid-Beta".  I'm willing to submit a patch for PG10.

Regards
Takayuki Tsunakawa

From: pgsql-hackers-owner@postgresql.org
> [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Craig Ringer
> On 13 April 2017 at 14:59, Tsunakawa, Takayuki
> <tsunakawa.takay@jp.fujitsu.com> wrote:
> 
> > 2. Make transaction_read_only GUC_REPORT This is to avoid the added
> > round-trip by SHOW command.  It also benefits client apps that want to
> know when the server gets promoted?  And this may simplify the libpq code.
> > I don't understand yet why we need to provide this feature for older servers
> by using SHOW.  Those who are already using <= 9.6 in production completed
> the system or application, and their business is running.  Why would they
> want to just replace libpq and use this feature?
> 
> I think "transaction_read_only" is a bit confusing for something we're
> expecting to change under us.
> 
> To me, a "read only" xact is one created with
> 
> BEGIN READ ONLY TRANSACTION;
> 
> .... which I would not expect to become read/write under me, since I
> explicitly asked for read-only.
> 
> It's more like "session read only" that we're interested in IMO.

I confirmed that the attached patch successfully provides:

* target_session_attrs=read-only
* If the server is >= 10, avoid the round-trip for SHOW transaction_read_only.

For this, I added a GUC_REPORT variable session_read_only which indicates the session's default read-only status.  The
characteristicsare:
 

* It cannot be changed directly by the user (postgresql.conf, SET, etc.)
* Its value is the same as default_transaction_read_only when not in recovery.
* Its value is false during recovery.

Could you include this in PG 10?  I think these are necessary as the bottom line to meet the average expectation of
users(please don't ask me what's the average; the main reasons are that PostgreSQL provides hot standby, PgJDBC enables
connectionto the standby (targetServerType=slave), and PostgreSQL emphasizes performance.)  Ideally, I wanted to add
otherfeatures of PgJDBC (e.g. targetServerType=preferSlave), but I thought this is the limit not to endanger the
qualityof the final release.
 

Regards
Takayuki Tsunakawa



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, May 24, 2017 at 3:16 AM, Tsunakawa, Takayuki
<tsunakawa.takay@jp.fujitsu.com> wrote:
> I confirmed that the attached patch successfully provides:
>
> * target_session_attrs=read-only
> * If the server is >= 10, avoid the round-trip for SHOW transaction_read_only.
>
> For this, I added a GUC_REPORT variable session_read_only which indicates the session's default read-only status.
Thecharacteristics are: 
>
> * It cannot be changed directly by the user (postgresql.conf, SET, etc.)
> * Its value is the same as default_transaction_read_only when not in recovery.
> * Its value is false during recovery.
>
> Could you include this in PG 10?  I think these are necessary as the bottom line to meet the average expectation of
users(please don't ask me what's the average; the main reasons are that PostgreSQL provides hot standby, PgJDBC enables
connectionto the standby (targetServerType=slave), and PostgreSQL emphasizes performance.)  Ideally, I wanted to add
otherfeatures of PgJDBC (e.g. targetServerType=preferSlave), but I thought this is the limit not to endanger the
qualityof the final release. 

I've already stated my position on this, which is that:

* target_session_attrs=read-only is a perfectly good new feature, but
we're past feature freeze, so it's material for v11.

* I'm not opposed to adding a GUC_REPORT GUC of some kind, but I see
no urgency about that either.  The feature works fine as it is.  The
fact that it could possibly be made to work more efficiently is not a
critical bug.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, May 24, 2017 at 3:16 AM, Tsunakawa, Takayuki
> <tsunakawa.takay@jp.fujitsu.com> wrote:
>> For this, I added a GUC_REPORT variable session_read_only which indicates the session's default read-only status.
Thecharacteristics are:
 
>> 
>> * It cannot be changed directly by the user (postgresql.conf, SET, etc.)
>> * Its value is the same as default_transaction_read_only when not in recovery.

I didn't look at exactly how you tried to do that, but GUCs whose values
depend on other GUCs generally don't work well at all.

>> * Its value is false during recovery.

[ scratches head... ]  Surely this should read as "true" during recovery?
Also, what happens if the standby server goes live mid-session?

>> Could you include this in PG 10?

I concur with Robert that this is too late for v10, and the argument to
shove it in anyway is not compelling.
        regards, tom lane

From: Robert Haas [mailto:robertmhaas@gmail.com]
> I've already stated my position on this, which is that:
> 
> * target_session_attrs=read-only is a perfectly good new feature, but we're
> past feature freeze, so it's material for v11.
> 
> * I'm not opposed to adding a GUC_REPORT GUC of some kind, but I see no
> urgency about that either.  The feature works fine as it is.  The fact that
> it could possibly be made to work more efficiently is not a critical bug.

I see.  I'll add this in the next CF for PG 11.  I'd be glad if you could review the patch in PG 11 development.  Also,
I'llupdate the PostgreSQL 10 Open Items page that way.
 

Regards
Takayuki Tsunakawa

From: pgsql-hackers-owner@postgresql.org
> [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Tom Lane
> I didn't look at exactly how you tried to do that, but GUCs whose values
> depend on other GUCs generally don't work well at all.

transaction_read_only and transaction_isolation depends on default_transaction_read_only and
default_transaction_isolationrespectively.  But I feel you are concerned about something I'm not aware of.  Could you
shareyour worries?  I haven't found a problem so far.
 


> >> * Its value is false during recovery.
> 
> [ scratches head... ]  Surely this should read as "true" during recovery?

Ouch, you are right.


> Also, what happens if the standby server goes live mid-session?

The clients will know the change of session_read_only when they do something that calls RecoveryInProgress().
Currently,RecoveryInProgress() seems to be the only place where the sessions notice the promotion, so I set
session_read_onlyto the value of default_transaction_read_only there.  I think that there is room for discussion here.
Itwould be ideal for the sessions to notice the server promotion promptly and notify the clients of the change.  I have
noidea to do that well.
 

Regards
Takayuki Tsunakawa

Hi,

On Wed, May 24, 2017 at 07:16:06AM +0000, Tsunakawa, Takayuki wrote:
> I confirmed that the attached patch successfully provides:

I was going to take a look at this, but the patch no longer applies
cleanly for me:

Hunk #1 succeeded at 1474 (offset 49 lines).
Hunk #2 succeeded at 1762 (offset 49 lines).
Hunk #3 succeeded at 1773 (offset 49 lines).
patching file doc/src/sgml/protocol.sgml
patching file src/backend/access/transam/xlog.c
Hunk #1 succeeded at 7909 (offset 5 lines).
patching file src/backend/tcop/postgres.c
Hunk #1 succeeded at 3737 (offset 15 lines).
patching file src/backend/utils/misc/check_guc
patching file src/backend/utils/misc/guc.c
Hunk #1 succeeded at 147 with fuzz 1.
Hunk #5 succeeded at 10062 (offset 11 lines).
patching file src/interfaces/libpq/fe-connect.c
Hunk #1 succeeded at 1178 (offset 112 lines).
Hunk #2 succeeded at 2973 (offset 124 lines).
Hunk #3 succeeded at 3003 (offset 124 lines).
Hunk #4 succeeded at 3067 (offset 124 lines).
Hunk #5 FAILED at 3022.
Hunk #6 succeeded at 3375 (offset 144 lines).
1 out of 6 hunks FAILED -- saving rejects to file
src/interfaces/libpq/fe-connect.c.rej
patching file src/interfaces/libpq/fe-exec.c
patching file src/interfaces/libpq/libpq-int.h
Hunk #1 succeeded at 361 (offset 1 line).
Hunk #2 succeeded at 421 (offset -1 lines).

Can you rebase it, please?


Best

Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.banck@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

On Wednesday, May 24, 2017 9:38:36 PM EDT Tsunakawa, Takayuki wrote:
> The clients will know the change of session_read_only when they do
> something that calls RecoveryInProgress().  Currently,
> RecoveryInProgress() seems to be the only place where the sessions
> notice the promotion, so I set session_read_only to the value of
> default_transaction_read_only there.  I think that there is room for
> discussion here.  It would be ideal for the sessions to notice the
> server promotion promptly and notify the clients of the change.  I
> have no idea to do that well.

My original patch did that via the new SendSignalToAllBackends() helper,
which is called whenever the postmaster exits hot stanby.

I incorporated those bits into your patch and rebased in onto master.  
Please see attached.

FWIW, I think that mixing the standby status and the default 
transaction writability is suboptimal.  They are related, yes, but not 
the same thing.  It is possible to have a master cluster in the 
read-only mode, and with this patch it would be impossible to 
distinguish from a hot-standby replica without also polling 
pg_is_in_recovery(), which defeats the purpose of having to do no
database roundtrips.  

                                 Elvis

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Sep 13, 2017 at 3:48 AM, Elvis Pranskevichus <elprans@gmail.com> wrote:
> I incorporated those bits into your patch and rebased in onto master.
> Please see attached.
>
> FWIW, I think that mixing the standby status and the default
> transaction writability is suboptimal.  They are related, yes, but not
> the same thing.  It is possible to have a master cluster in the
> read-only mode, and with this patch it would be impossible to
> distinguish from a hot-standby replica without also polling
> pg_is_in_recovery(), which defeats the purpose of having to do no
> database roundtrips.

Hi Elvis,

FYI the recovery test 001_stream_rep.pl fails with this patch applied.
You can see that if you configure with --enable-tap-tests, build and
then cd into src/test/recovery and "make check".

-- 
Thomas Munro
http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

> On 22 Sep 2017, at 18:57, Melanie Plageman <melanieplageman@gmail.com> wrote:
>
> On Tue, Sep 19, 2017 at 4:15 PM, Melanie Plageman <melanieplageman@gmail.com <mailto:melanieplageman@gmail.com>>
wrote:
> The latest patch applies cleanly and builds (I am also seeing the failing TAP tests), however, I have a concern. With
asingle server set up, when I attempt to make a connection with target_session_attrs=read-write, I get the message  
> psql: could not make a suitable connection to server "localhost:5432"
> Whereas, when I attempt to make a connection with target_session_attrs=read-only, it is successful.
>
> I might be missing something, but this seems somewhat counter-intuitive. I would expect to specify read-write as
target_session_attrsand successfully connect to a server on which read and write operations are permitted. I see this
behaviorimplemented in src/interfaces/libpq/fe-connect.c 
> Is there a reason to reject a connection to a primary server when I specify 'read-write'? Is this intentional?
>
> Hi Elvis,
>
> Making an assumption about the intended functionality mentioned above, I swapped the 'not' to the following lines of
src/interfaces/libpq/fe-connect.c~ line 3005 
>
>                 if (conn->target_session_attrs != NULL &&
>                     ((strcmp(conn->target_session_attrs, "read-write") == 0 && conn->session_read_only) ||
>                      (strcmp(conn->target_session_attrs, "read-only") == 0 && !conn->session_read_only)))
>
> I rebased and built with this change locally.
> The review below is based on the patch with that change.
>
> Also, the following comment has what looks like a copy-paste error and the first line should be deleted
> in src/backend/utils/misc/guc.c ~ line 10078
> assign_default_transaction_read_only
>
>
> +assign_default_transaction_read_only(bool newval, void *extra)
> ...
> +    /*
> -     * We clamp manually-set values to at least 1MB.  Since
> +     * Also set the session read-only parameter.  We only need
> +     * to set the correct value in processes that have database
> +     * sessions, but there's no mechanism to know that there's
>
> patch applies cleanly: yes
> installcheck: passed
> installcheck-world: passed
> feature works as expected: yes (details follow)
>
> With two servers, one configured as the primary and one configured to run in Hot Standby mode, I was able to observe
thatthe value of session_read_only changed after triggering failover once the standby server exited recovery 
>
> When attempting to connect to a primary server with target_session_attrs=read-write, I was successful and when
attemptingto connect with target_session_attrs=read-only, the connection was closed and the expected message was
produced

Based on the unaddressed questions raised in this thread, I’m marking this
patch Returned with Feedback.  Please re-submit a new version of the patch to a
future commitfest.

cheers ./daniel



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Oct 2, 2017 at 4:16 PM, Daniel Gustafsson <daniel@yesql.se> wrote:
> Based on the unaddressed questions raised in this thread, I’m marking this
> patch Returned with Feedback.  Please re-submit a new version of the patch to a
> future commitfest.

For some reason this patch has been moved to CF 2017-11, but there has
been no real updates since the last version in

https://www.postgresql.org/message-id/flat/1572601.d6Y6hSXNBC%40hammer.magicstack.net#1572601.d6Y6hSXNBC@hammer.magicstack.net
was sent. The patch fails to apply as well now. I am marking it as
returned with feedback. I am not seeing any replies to the requests
done as well.
--
Michael

On 2017-11-28 10:55:34 +0900, Michael Paquier wrote:
> On Mon, Oct 2, 2017 at 4:16 PM, Daniel Gustafsson <daniel@yesql.se> wrote:
> > Based on the unaddressed questions raised in this thread, I’m marking this
> > patch Returned with Feedback.  Please re-submit a new version of the patch to a
> > future commitfest.
> 
> For some reason this patch has been moved to CF 2017-11, but there has
> been no real updates since the last version in
>
https://www.postgresql.org/message-id/flat/1572601.d6Y6hSXNBC%40hammer.magicstack.net#1572601.d6Y6hSXNBC@hammer.magicstack.net
> was sent. The patch fails to apply as well now. I am marking it as
> returned with feedback. I am not seeing any replies to the requests
> done as well.

A CF entry for this patch has been created yesterday, without any
changes having happend since the above status update.  Given that
there's been no progress for multiple commitfests, and this is the last
CF, this doesn't seem appropriate. Therefore marked as returned with
feedback.

Greetings,

Andres Freund

On Thursday, March 1, 2018 4:25:16 AM EDT Andres Freund wrote:
> A CF entry for this patch has been created yesterday, without any
> changes having happend since the above status update.  Given that
> there's been no progress for multiple commitfests, and this is the
> last CF, this doesn't seem appropriate. Therefore marked as returned
> with feedback.

Sorry for the long silence on this.  I'm attaching a rebased and 
cleaned-up patch with the earlier review issues addressed.  I've checked 
all relevant tests and verified manually.

                             Elvis

Melanie Plageman <melanieplageman@gmail.com> writes:
> On Mon, Sep 24, 2018 at 9:28 AM Elvis Pranskevichus <elprans@gmail.com>
> wrote:
>> Sorry for the long silence on this.  I'm attaching a rebased and
>> cleaned-up patch with the earlier review issues addressed.  I've checked
>> all relevant tests and verified manually.

> After taking a look at the updated patch, it appears to address the review
> feedback which I provided. However, the patch didn't apply cleanly for me,
> so it likely needs a quick look along with a rebase.

It looks like the one apply failure is due to the patch having corrected
a comment that somebody else already corrected.  So no big deal there.

Looking through the thread, it seems like there's a pretty fundamental
design issue that hasn't been resolved, namely whether and how this ought
to interact with default_transaction_read_only.  The patch as it stands
seems to be trying to implement the definition that the transmittable
variable session_read_only is equal to "!(DefaultXactReadOnly ||
RecoveryInProgress())".  I doubt that that's a good idea.  In the first
place, it's not at all clear that such a flag is sufficient for all
use-cases.  In the second, it's somewhere between difficult and impossible
to correctly implement GUCs that are interdependent in that way; the
current patch certainly fails to do so.  (It will not correctly track
intra-session changes to DefaultXactReadOnly, for instance.)

I think that we'd be better off maintaining a strict separation between
"in hot standby" and default_transaction_read_only.  If there are use
cases in which people would like to reject connections based on either
one being true, let's handle that by marking them both GUC_REPORT, and
having libpq check both.  (So there need to be two flavors of
target-session-attrs libpq parameter, depending on whether you want
"in hot standby" or "currently read only" to be checked.)

That puts us back to having to choose an appropriate GUC name, because
"session_read_only" isn't it :-(.  Don't have any better ideas about
that than what's been discussed in the thread.

I'm also not terribly happy with the patch adding a whole new
cross-backend signaling mechanism for this; surely we don't really
need that.  Perhaps it'd be sufficient to transmit SIGHUP and embed
the check for "just exited hot standby" in the handling for that?

            regards, tom lane

On Monday, November 12, 2018 1:08:37 PM EST Tom Lane wrote:
> Looking through the thread, it seems like there's a pretty fundamental
> design issue that hasn't been resolved, namely whether and how this
> ought to interact with default_transaction_read_only.  The patch as
> it stands seems to be trying to implement the definition that the
> transmittable variable session_read_only is equal to
> "!(DefaultXactReadOnly || RecoveryInProgress())".  I doubt that
> that's a good idea.  In the first place, it's not at all clear that
> such a flag is sufficient for all use-cases.  In the second, it's
> somewhere between difficult and impossible to correctly implement
> GUCs that are interdependent in that way; the current patch certainly
> fails to do so.  (It will not correctly track intra-session changes
> to DefaultXactReadOnly, for instance.)
>
> I think that we'd be better off maintaining a strict separation
> between "in hot standby" and default_transaction_read_only.  If there
> are use cases in which people would like to reject connections based
> on either one being true, let's handle that by marking them both
> GUC_REPORT, and having libpq check both.  (So there need to be two
> flavors of target-session-attrs libpq parameter, depending on whether
> you want "in hot standby" or "currently read only" to be checked.)

I agree that the original design with the separate "in_hot_standby" GUC 
is better.  Hot standby and read-only session are distinct modes, not 
all commands that are OK in a read-only session will succeed on a hot-
standby backend.

> I'm also not terribly happy with the patch adding a whole new
> cross-backend signaling mechanism for this; surely we don't really
> need that.  Perhaps it'd be sufficient to transmit SIGHUP and embed
> the check for "just exited hot standby" in the handling for that?

That seems doable.  Is there an existing check that is a good candidate 
for "just exited hot standby"?


                               Elvis

> On Mon, Nov 12, 2018 at 8:30 PM Elvis Pranskevichus <elprans@gmail.com> wrote:
>
> On Monday, November 12, 2018 1:08:37 PM EST Tom Lane wrote:
> > Looking through the thread, it seems like there's a pretty fundamental
> > design issue that hasn't been resolved, namely whether and how this
> > ought to interact with default_transaction_read_only.  The patch as
> > it stands seems to be trying to implement the definition that the
> > transmittable variable session_read_only is equal to
> > "!(DefaultXactReadOnly || RecoveryInProgress())".  I doubt that
> > that's a good idea.  In the first place, it's not at all clear that
> > such a flag is sufficient for all use-cases.  In the second, it's
> > somewhere between difficult and impossible to correctly implement
> > GUCs that are interdependent in that way; the current patch certainly
> > fails to do so.  (It will not correctly track intra-session changes
> > to DefaultXactReadOnly, for instance.)
> >
> > I think that we'd be better off maintaining a strict separation
> > between "in hot standby" and default_transaction_read_only.  If there
> > are use cases in which people would like to reject connections based
> > on either one being true, let's handle that by marking them both
> > GUC_REPORT, and having libpq check both.  (So there need to be two
> > flavors of target-session-attrs libpq parameter, depending on whether
> > you want "in hot standby" or "currently read only" to be checked.)
>
> I agree that the original design with the separate "in_hot_standby" GUC
> is better.  Hot standby and read-only session are distinct modes, not
> all commands that are OK in a read-only session will succeed on a hot-
> standby backend.
>
> > I'm also not terribly happy with the patch adding a whole new
> > cross-backend signaling mechanism for this; surely we don't really
> > need that.  Perhaps it'd be sufficient to transmit SIGHUP and embed
> > the check for "just exited hot standby" in the handling for that?
>
> That seems doable.  Is there an existing check that is a good candidate
> for "just exited hot standby"?

Apparently due the minor conflict, mentioned above, the patch was in "Waiting
on author" state, which probably is not exactly correct. I'm moving it to the
next CF, but still, it would be nice if you post an updated version without
conflicts.

On Fri, Nov 30, 2018 at 05:35:21PM +0100, Dmitry Dolgov wrote:
> Apparently due the minor conflict, mentioned above, the patch was in "Waiting
> on author" state, which probably is not exactly correct. I'm moving it to the
> next CF, but still, it would be nice if you post an updated version without
> conflicts.

And nothing has happened since, so I have marked the entry as returned
with feedback.
--
Michael