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

[Daniel Gustafsson]

> On 22 Sep 2017, at 18:57, Melanie Plageman  wrote:
> 
> On Tue, Sep 19, 2017 at 4:15 PM, Melanie Plageman  > 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

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

[Melanie Plageman]

On Tue, Sep 19, 2017 at 4:15 PM, Melanie Plageman  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!

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

[Melanie Plageman]

On Tue, Sep 12, 2017 at 6:11 PM, Thomas Munro  wrote:

> On Wed, Sep 13, 2017 at 3:48 AM, Elvis Pranskevichus 
> 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?

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

[Thomas Munro]

On Wed, Sep 13, 2017 at 3:48 AM, Elvis Pranskevichus  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

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

[Elvis Pranskevichus]

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
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 096a8be605..4d74740699 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1474,10 +1474,14 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname

 If this parameter is set to read-write, only a
 connection in which read-write transactions are accepted by default
+is considered acceptable.
+If this parameter is set to read-only, only a
+connection in which read-write transactions are rejected by default
 is considered acceptable.  The query
 SHOW transaction_read_only will be sent upon any
-successful connection; if it returns on, the connection
-will be closed.  If multiple hosts were specified in the connection
+successful connection if the server is prior to version 10.
+If the session attribute does not match the requested one, the connection will be closed.
+If multiple hosts were specified in the connection
 string, any remaining servers will be tried just as if the connection
 attempt had failed.  The default value of this parameter,
 any, regards all connections as acceptable.
@@ -1758,6 +1762,7 @@ const char *PQparameterStatus(const PGconn *conn, const char *paramName);
application_name,
is_superuser,
session_authorization,
+   session_read_only,
DateStyle,
IntervalStyle,
TimeZone,
@@ -1768,7 +1773,8 @@ const char *PQparameterStatus(const PGconn *conn, const char *paramName);
standard_conforming_strings was not reported by releases
before 8.1;
IntervalStyle was not reported by releases before 8.4;
-   application_name was not reported by releases before 9.0.)
+   application_name was not reported by releases before 9.0;
+   session_read_only was not reported by releases before 10.0.)
Note that
server_version,
server_encoding and
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 76d1c13cc4..e2a1cd1c03 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -1255,6 +1255,7 @@ SELCT 1/0;
 application_name,
 is_superuser,
 session_authorization,
+session_read_only,
 DateStyle,
 IntervalStyle,
 TimeZone,
@@ -1265,7 +1266,8 @@ SELCT 1/0;
 standard_conforming_strings was not reported by releases
 before 8.1;
 IntervalStyle was not reported by releases before 8.4;
-application_name was not reported by releases before 9.0.)
+application_name was not reported by releases before 9.0;
+session_read_only was not reported by releases before 10.0.)
 Note that
 server_version,
 server_encoding and
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 442341a707..e862d7e993 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7715,8 +7715,10 @@ StartupXLOG(void)
 	 * Shutdown the recovery environment. This must occur after
 	 * RecoverPreparedTransactions(), see notes for lock_twophase_recover()
 	 */
-	if (standbyState != STANDBY_DISABLED)
+	if (standbyState != STANDBY_DISABLED) {
 		ShutdownRecoveryTransactionEnvironment();
+		SendHotStandbyExitSignal();
+	}
 
 	/* Shut down xlogreader */
 	if (readFile >= 0)
@@ -7921,6 +7923,11 @@ RecoveryInProgress(void)
 			 */
 			pg_memory_barrier();
 			InitXLOGAccess();
+
+			/* Update session read-only status. */
+			SetConfigOption("session_read_only",
+			DefaultXactReadOnly ? "on" : "off",
+			PGC_INTERNAL, PGC_S_OVERRIDE);
 		}
 
 		/*
diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
index bacc08eb84..9b1646cf9b 

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

[Michael Banck]

Hi,

On Wed, May 24, 2017 at 07:16:06AM +, 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.ba...@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

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

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@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

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

[Tsunakawa, Takayuki]

From: Robert Haas [mailto:robertmh...@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

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

[Tom Lane]

Robert Haas  writes:
> On Wed, May 24, 2017 at 3:16 AM, Tsunakawa, Takayuki
>  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

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

[Robert Haas]

On Wed, May 24, 2017 at 3:16 AM, Tsunakawa, Takayuki
 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

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

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Craig Ringer
> On 13 April 2017 at 14:59, Tsunakawa, Takayuki
>  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




libpq-fast-connect-read-only.patch
Description: libpq-fast-connect-read-only.patch

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

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

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Craig Ringer
> On 13 April 2017 at 14:59, Tsunakawa, Takayuki
>  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

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

[Craig Ringer]

On 13 April 2017 at 14:59, Tsunakawa, Takayuki
 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

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

[Tsunakawa, Takayuki]

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-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@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-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Robert Haas
> On Tue, Apr 11, 2017 at 4:05 AM, Magnus Hagander 
> 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

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

[Magnus Hagander]

On Wed, Apr 12, 2017 at 2:36 PM, Robert Haas  wrote:

> On Tue, Apr 11, 2017 at 4:05 AM, Magnus Hagander 
> wrote:
> > On Tue, Apr 11, 2017 at 3:26 AM, Michael Paquier <
> michael.paqu...@gmail.com> wrote:
> >> On Mon, Apr 10, 2017 at 5:47 PM, Magnus Hagander 
> >> 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/ 
 Work: https://www.redpill-linpro.com/ 

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

[Robert Haas]

On Tue, Apr 11, 2017 at 4:05 AM, Magnus Hagander  wrote:
> On Tue, Apr 11, 2017 at 3:26 AM, Michael Paquier  
> wrote:
>> On Mon, Apr 10, 2017 at 5:47 PM, Magnus Hagander 
>> 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

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

[Magnus Hagander]

On Tue, Apr 11, 2017 at 2:38 PM, Simon Riggs  wrote:

> On 11 April 2017 at 09:05, Magnus Hagander  wrote:
> > On Tue, Apr 11, 2017 at 3:26 AM, Michael Paquier <
> michael.paqu...@gmail.com>
> > wrote:
> >>
> >> On Mon, Apr 10, 2017 at 5:47 PM, Magnus Hagander 
> >> 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/ 
 Work: https://www.redpill-linpro.com/ 

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

[Simon Riggs]

On 11 April 2017 at 09:05, Magnus Hagander  wrote:
> On Tue, Apr 11, 2017 at 3:26 AM, Michael Paquier 
> wrote:
>>
>> On Mon, Apr 10, 2017 at 5:47 PM, Magnus Hagander 
>> 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 Riggshttp://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

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

[Magnus Hagander]

On Tue, Apr 11, 2017 at 3:26 AM, Michael Paquier 
wrote:

> On Mon, Apr 10, 2017 at 5:47 PM, Magnus Hagander 
> 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/ 
 Work: https://www.redpill-linpro.com/ 

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

[Michael Paquier]

On Mon, Apr 10, 2017 at 5:47 PM, Magnus Hagander  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

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

[Magnus Hagander]

On Wed, Apr 5, 2017 at 6:22 PM, Robert Haas  wrote:

> On Thu, Mar 23, 2017 at 4:50 AM, Magnus Hagander 
> 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/ 
 Work: https://www.redpill-linpro.com/ 

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

[Robert Haas]

On Thu, Mar 23, 2017 at 4:50 AM, Magnus Hagander  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

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

[Elvis Pranskevichus]

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

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

[Magnus Hagander]

On Wed, Mar 22, 2017 at 9:00 PM, Peter Eisentraut <
peter.eisentr...@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/

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

[Elvis Pranskevichus]

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

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

[Robert Haas]

On Wed, Mar 22, 2017 at 4:00 PM, Peter Eisentraut
 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

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

[Peter Eisentraut]

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

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

[Elvis Pranskevichus]

On Wednesday, March 22, 2017 2:17:27 PM EDT Jaime Casanova wrote:
> On 18 March 2017 at 14:01, Elvis Pranskevichus  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

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

[Jaime Casanova]

On 18 March 2017 at 14:01, Elvis Pranskevichus  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

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

[Robert Haas]

On Wed, Mar 22, 2017 at 8:25 AM, Elvis Pranskevichus  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

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

[Elvis Pranskevichus]

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

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

[Peter Eisentraut]

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

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

[Elvis Pranskevichus]

On Saturday, March 18, 2017 3:33:16 AM EDT Michael Paquier wrote:
> On Sat, Mar 18, 2017 at 2:56 AM, 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.  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

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

[Michael Paquier]

On Sat, Mar 18, 2017 at 2:56 AM, 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.  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

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

[Elvis Pranskevichus]

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

>From bdf9a409005f0ea209c640935d9827369725e241 Mon Sep 17 00:00:00 2001
From: Elvis Pranskevichus 
Date: Fri, 17 Mar 2017 13:25:08 -0400
Subject: [PATCH v1] Add and report the new "in_hot_standby" GUC
 pseudo-variable.

Currently, clients wishing to know when the server exits hot standby
have to resort to polling, which is 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.
---
 doc/src/sgml/high-availability.sgml  |  4 +--
 doc/src/sgml/libpq.sgml  |  1 +
 doc/src/sgml/protocol.sgml   |  1 +
 doc/src/sgml/ref/show.sgml   |  9 +++
 src/backend/access/transam/xlog.c|  4 ++-
 src/backend/commands/async.c | 48 
 src/backend/storage/ipc/procarray.c  | 30 ++
 src/backend/storage/ipc/procsignal.c |  3 +++
 src/backend/storage/ipc/standby.c|  9 +++
 src/backend/tcop/postgres.c  |  6 -
 src/backend/utils/init/postinit.c|  6 +
 src/backend/utils/misc/check_guc | 10 
 src/backend/utils/misc/guc.c | 15 +++
 src/include/commands/async.h |  7 ++
 src/include/storage/procarray.h  |  2 ++
 src/include/storage/procsignal.h |  2 ++
 src/include/storage/standby.h|  1 +
 17 files changed, 149 insertions(+), 9 deletions(-)

diff --git a/doc/src/sgml/high-availability.sgml b/doc/src/sgml/high-availability.sgml
index cc84b911b0..44795c5bcc 100644
--- a/doc/src/sgml/high-availability.sgml
+++ b/doc/src/sgml/high-availability.sgml
@@ -1831,8 +1831,8 @@ if (!triggered)

 

-Users will be able to tell whether their session is read-only by
-issuing SHOW transaction_read_only.  In addition, a set of
+Users will be able to tell whether their session is in hot standby mode by
+issuing SHOW in_hot_standby.  In addition, a set of
 functions () allow users to
 access information about the standby server. These allow you to write
 programs that are aware of the current state of the database. These
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 4bc5bf3192..367ec4460d 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1706,6 +1706,7 @@ const char *PQparameterStatus(const PGconn *conn, const char *paramName);
server_encoding,
client_encoding,
application_name,
+   in_hot_standby,
is_superuser,
session_authorization,
DateStyle,
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 7c82b48845..0fafaf6788 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -1123,6 +1123,7 @@
 server_encoding,
 client_encoding,
 application_name,
+in_hot_standby,
 is_superuser,
 session_authorization,
 DateStyle,
diff --git a/doc/src/sgml/ref/show.sgml b/doc/src/sgml/ref/show.sgml
index 46bb239baf..cf21bd961a 100644
--- a/doc/src/sgml/ref/show.sgml
+++ b/doc/src/sgml/ref/show.sgml
@@ -78,6 +78,15 @@ SHOW ALL

 

+IN_HOT_STANDBY
+
+ 
+  True if the server is in Hot Standby mode.
+ 
+
+   
+
+   
 LC_COLLATE
 
  
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index cdb3a8ac1d..acca53b12f 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7654,8 +7654,10 @@ StartupXLOG(void)
 	 * Shutdown the recovery environment. This must occur after
 	 * RecoverPreparedTransactions(), see notes for lock_twophase_recover()
 	 */
-	if (standbyState != STANDBY_DISABLED)
+	if (standbyState != STANDBY_DISABLED) {
 		ShutdownRecoveryTransactionEnvironment();
+		SendHotStandbyExitSignal();
+	}
 
 	/* Shut down xlogreader */
 	if (readFile >= 0)
diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
index e32d7a1d4e..8bc365489a 100644
--- a/src/backend/commands/async.c
+++ b/src/backend/commands/async.c
@@ -355,6 +355,8 @@ static List *upperPendingNotifies = NIL;		/* list of upper-xact lists */
  */
 volatile sig_atomic_t notifyInterruptPending = false;
 
+volatile sig_atomic_t