[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