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

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

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

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

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

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

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

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

--
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, 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

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

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

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

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

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

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

--
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, 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

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

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

--
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, Mar 22, 2017 at 9: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.

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

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? 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?

(I haven't thought it through all the way, but figured I should mention the
thought as I'm working through my email backlog.)

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

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

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

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

--
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, Apr 5, 2017 at 6:22 PM, Robert Haas <robertmhaas@gmail.com> wrote:

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.

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.

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.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

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

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

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 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.

We'd have to cache the status recived yes. I don't see how that makes it a
"set of" if/else code when there is only one if/else now, though? Though
admittedly I haven't looked at the actual code for it.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

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

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

On Tue, Apr 11, 2017 at 2:38 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

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

Well, one of those could come automatically with a GUC_REPORT variable of
the correct type, no? So if we were to use the transaction_read_only one,
you'd get a notification on promotion because your transaction became
read/write, wouldn't it?

"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.

I think going through all those steps is moving the goalposts a bit too far
for the 10 release.

But if adjustment to the already applied patch is needed to make sure we
can improve on it to get to this point in 11, that's more on topic I think.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

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

--
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, Apr 12, 2017 at 2:36 PM, Robert Haas <robertmhaas@gmail.com> wrote:

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.

Fair enough, sounds reasonable. I wasn't engaged in the original thread, so
you clearly have thought about this more than I have. I just wanted to make
sure we're not creating something that's going to cause a head-ache for
such a feature in the future.

(And this is why I was specifically asking you if you wanted it on the open
items list or not!)

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

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 people around 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 provides hot 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 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?

[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 also required 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 anything if 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 of PostgreSQL.

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

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 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.

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

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

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

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 characteristics 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 connection to the standby (targetServerType=slave), and PostgreSQL emphasizes performance.) Ideally, I wanted to add other features of PgJDBC (e.g. targetServerType=preferSlave), but I thought this is the limit not to endanger the quality of the final release.

Regards
Takayuki Tsunakawa

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. The characteristics 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 connection to the standby (targetServerType=slave), and PostgreSQL emphasizes performance.) Ideally, I wanted to add other features of PgJDBC (e.g. targetServerType=preferSlave), but I thought this is the limit not to endanger the quality of 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

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

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. The characteristics 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

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

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'll update the PostgreSQL 10 Open Items page that way.

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

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_isolation respectively. But I feel you are concerned about something I'm not aware of. Could you share your 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_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.

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

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

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

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

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 Tue, Sep 12, 2017 at 6:11 PM, Thomas Munro <thomas.munro@enterprisedb.com

wrote:

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".

The latest patch applies cleanly and builds (I am also seeing the failing

TAP tests), however, I have a concern. With a single 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_attrs and successfully
connect to a server on which read and write operations are permitted. I see
this behavior implemented 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?

On Tue, Sep 19, 2017 at 4:15 PM, Melanie Plageman <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 a single 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_attrs and successfully
connect to a server on which read and write operations are permitted. I see
this behavior implemented 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 that the 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 attempting to
connect with target_session_attrs=read-only, the connection was closed and
the expected message was produced

Thanks!

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 a single 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_attrs and successfully connect to a server on which read and write operations are permitted. I see this behavior implemented 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 that the 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 attempting to 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
/messages/by-id/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
/messages/by-id/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

On Mon, Sep 24, 2018 at 9:28 AM Elvis Pranskevichus <elprans@gmail.com>
wrote:

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

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.

After looking at the patch [1]https://commitfest.postgresql.org/20/1677/ to add a "prefer-read" option to
target-session-attrs which is also up for consideration this commitfest, I
was
wondering, assuming it is desirable to have both "prefer-read" and
"read-only"
modes, does it make sense for the "prefer-read" option to also be
implemented
with a ParameterStatus message instead of using polling?

[1]: https://commitfest.postgresql.org/20/1677/

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