Re: Libpq support to connect to standby server as priority

[Tom Lane]

I wrote:
> vignesh C  writes:
>> Conchuela is failing because of:
>> ok 17 - connect to node standby_1 if mode "standby" and standby_1,primary
>> listed
>> ack Broken pipe: write( 13, 'SHOW port;' ) at
>> /usr/local/lib/perl5/site_perl/IPC/Run/IO.pm line 549.

> It didn't fail on the next run, so this might just be a phase-of-the-moon
> glitch.  Conchuela is a bit prone to that sort of thing, in my experience.
> We'll have to wait and see if it's at all repeatable.

Conchuela hasn't shown it again, but it turns out to be repeatable
on my old warhorse gaur.  After a bit of study I see the problem:
we're asking Perl to write to the stdin of a psql process that
may not be there to receive the data.  We've dodged that issue
in other tests by passing "undef" as the stdin to sub psql, so
that's what I did here.

regards, tom lane

Re: Libpq support to connect to standby server as priority

[Tom Lane]

vignesh C  writes:
> Conchuela is failing because of:
> ok 17 - connect to node standby_1 if mode "standby" and standby_1,primary
> listed
> ack Broken pipe: write( 13, 'SHOW port;' ) at
> /usr/local/lib/perl5/site_perl/IPC/Run/IO.pm line 549.

It didn't fail on the next run, so this might just be a phase-of-the-moon
glitch.  Conchuela is a bit prone to that sort of thing, in my experience.
We'll have to wait and see if it's at all repeatable.

regards, tom lane

Re: Libpq support to connect to standby server as priority

[Greg Nancarrow]

On Wed, Mar 3, 2021 at 2:49 PM vignesh C  wrote:
>
>
>
> On Wed, Mar 3, 2021 at 7:37 AM Tom Lane  wrote:
> >
> > Greg Nancarrow  writes:
> > > I've marked this as "Ready for Committer".
> >
> > I've pushed this after whacking it around a fair amount.  A lot of
> > that was cosmetic, but one thing that wasn't is that I got rid of the
> > proposed "which_primary_host" variable.  I thought the logic around
> > that was way too messy and probably buggy.  Even if it worked exactly
> > as intended, I'm dubious that the design intention was good.  I think
> > it makes more sense just to go through the whole server list again
> > without the restriction to standby servers.  In particular, that will
> > give saner results if the servers' status isn't holding still.
> >
>
> Buildfarm machine crake and conchuela have failed after this commit.
> I had checked the failures, crake is failing because of:
> Mar 02 21:22:56 ./src/test/recovery/t/001_stream_rep.pl: Variable
declared in conditional statement at line 88, column 2.  Declare variables
outside of the condition.  ([Variables::ProhibitConditionalDeclarations]
Severity: 5)
> I have analyzed and posted a patch at [1] for this. That might fix this
problem.
>
> Conchuela is failing because of:
> ok 17 - connect to node standby_1 if mode "standby" and standby_1,primary
listed
> ack Broken pipe: write( 13, 'SHOW port;' ) at
/usr/local/lib/perl5/site_perl/IPC/Run/IO.pm line 549.
> ### Stopping node "primary" using mode immediate
> # Running: pg_ctl -D
/home/pgbf/buildroot/HEAD/pgsql.build/src/test/recovery/tmp_check/t_001_stream_rep_primary_data/pgdata
-m immediate stop
> waiting for server to shut down... done
>
> I could not find the exact reason for this failure, I'm checking further
on why it is failing.
> Thoughts?
>
> [1] -
https://www.postgresql.org/message-id/CALDaNm3L%3DROeb%3D4rKf0XMN0CqrEnn6T%3D-44m4fsDAhcw-%40mail.gmail.com
> OUCVA
>


At least the first problem seems to possibly be because of:

https://www.effectiveperlprogramming.com/2019/07/no-more-false-postfix-lexical-declarations-in-v5-30/

The buildfarm machine crake is using Perl 5.30.3.

Regards,
Greg Nancarrow
Fujitsu Australia

Re: Libpq support to connect to standby server as priority

[vignesh C]

On Wed, Mar 3, 2021 at 7:37 AM Tom Lane  wrote:
>
> Greg Nancarrow  writes:
> > I've marked this as "Ready for Committer".
>
> I've pushed this after whacking it around a fair amount.  A lot of
> that was cosmetic, but one thing that wasn't is that I got rid of the
> proposed "which_primary_host" variable.  I thought the logic around
> that was way too messy and probably buggy.  Even if it worked exactly
> as intended, I'm dubious that the design intention was good.  I think
> it makes more sense just to go through the whole server list again
> without the restriction to standby servers.  In particular, that will
> give saner results if the servers' status isn't holding still.
>

Buildfarm machine crake and conchuela have failed after this commit.
I had checked the failures, crake is failing because of:
Mar 02 21:22:56 ./src/test/recovery/t/001_stream_rep.pl: Variable declared
in conditional statement at line 88, column 2.  Declare variables outside
of the condition.  ([Variables::ProhibitConditionalDeclarations] Severity:
5)
I have analyzed and posted a patch at [1] for this. That might fix this
problem.

Conchuela is failing because of:
ok 17 - connect to node standby_1 if mode "standby" and standby_1,primary
listed
ack Broken pipe: write( 13, 'SHOW port;' ) at
/usr/local/lib/perl5/site_perl/IPC/Run/IO.pm line 549.
### Stopping node "primary" using mode immediate
# Running: pg_ctl -D
/home/pgbf/buildroot/HEAD/pgsql.build/src/test/recovery/tmp_check/t_001_stream_rep_primary_data/pgdata
-m immediate stop
waiting for server to shut down... done

I could not find the exact reason for this failure, I'm checking further on
why it is failing.
Thoughts?

[1] -
https://www.postgresql.org/message-id/CALDaNm3L%3DROeb%3D4rKf0XMN0CqrEnn6T%3D-44m4fsDAhcw-%40mail.gmail.com

OUCVA


Regards,
Vignesh

Re: Libpq support to connect to standby server as priority

[Tom Lane]

Greg Nancarrow  writes:
> I've marked this as "Ready for Committer".

I've pushed this after whacking it around a fair amount.  A lot of
that was cosmetic, but one thing that wasn't is that I got rid of the
proposed "which_primary_host" variable.  I thought the logic around
that was way too messy and probably buggy.  Even if it worked exactly
as intended, I'm dubious that the design intention was good.  I think
it makes more sense just to go through the whole server list again
without the restriction to standby servers.  In particular, that will
give saner results if the servers' status isn't holding still.

regards, tom lane

Re: Libpq support to connect to standby server as priority

[Greg Nancarrow]

On Fri, Feb 12, 2021 at 2:42 PM vignesh C  wrote:
> > Thanks, just one minor thing I missed in doc/src/sgml/libpq.sgml.
> >
> > +The support of read-write transactions is determined by the
> > value of the
> > +default_transaction_read_only and
> > +in_hot_standby configuration parameters, that is
> > +reported by the server (if supported) upon successful connection.  
> > If
> >
> >
> > should be:
> >
> > +The support of read-write transactions is determined by the
> > values of the
> > +default_transaction_read_only and
> > +in_hot_standby configuration parameters, that 
> > are
> > +reported by the server (if supported) upon successful connection.  
> > If
> >
> >
> > (i.e. "value" -> "values" and "is" -> "are")
>
> Thanks for the comments, this is handled in the v23 patch attached.
> Thoughts?
>

I've marked this as "Ready for Committer".

(and also added you to the author list)


Regards,
Greg Nancarrow
Fujitsu Australia

Re: Libpq support to connect to standby server as priority

[vignesh C]

On Fri, Feb 12, 2021 at 7:07 AM Greg Nancarrow  wrote:
>
> On Wed, Feb 10, 2021 at 5:09 PM vignesh C  wrote:
> >
> > Modified.
> > These comments are handled in v22 patch posted in my earlier mail.
> >
>
> Thanks, just one minor thing I missed in doc/src/sgml/libpq.sgml.
>
> +The support of read-write transactions is determined by the
> value of the
> +default_transaction_read_only and
> +in_hot_standby configuration parameters, that is
> +reported by the server (if supported) upon successful connection.  If
>
>
> should be:
>
> +The support of read-write transactions is determined by the
> values of the
> +default_transaction_read_only and
> +in_hot_standby configuration parameters, that are
> +reported by the server (if supported) upon successful connection.  If
>
>
> (i.e. "value" -> "values" and "is" -> "are")

Thanks for the comments, this is handled in the v23 patch attached.
Thoughts?

Regards,
Vignesh
From 45d5680adae88a5c6f9d81a5077601f036e487c5 Mon Sep 17 00:00:00 2001
From: Vignesh C 
Date: Mon, 8 Feb 2021 11:23:31 +0530
Subject: [PATCH v23] Enhance the libpq "target_session_attrs" connection
 parameter.

Enhance the connection parameter "target_session_attrs" to support new values:
read-only/primary/standby/prefer-standby.
Add a new "read-only" target_session_attrs option value, to support connecting
to a read-only server if available from the list of hosts (otherwise the
connection attempt fails).
Add a new "primary" target_session_attrs option value, to support connecting to
a server which is not in hot-standby mode, if available from the list of hosts
(otherwise the connection attempt fails).
Add a new "standby" target_session_attrs option value, to support connecting to
a server which is in hot-standby mode, if available from the list of hosts
(otherwise the connection attempt fails).
Add a new "prefer-standby" target_session_attrs option value, to support
connecting to a server which is in hot-standby mode, if available from the list
of hosts (otherwise connect to a server which is not in hot-standby mode).

Discussion: https://www.postgresql.org/message-id/flat/caf3+xm+8-ztokav9ghij3wfgentq97qcjxqt+rbfq6f7onz...@mail.gmail.com
---
 doc/src/sgml/high-availability.sgml   |  16 +-
 doc/src/sgml/libpq.sgml   |  73 +-
 doc/src/sgml/protocol.sgml|   5 +-
 src/backend/utils/misc/guc.c  |   3 +-
 src/interfaces/libpq/fe-connect.c | 432 ++
 src/interfaces/libpq/fe-exec.c|  18 +-
 src/interfaces/libpq/libpq-fe.h   |   3 +-
 src/interfaces/libpq/libpq-int.h  |  52 +++-
 src/test/recovery/t/001_stream_rep.pl |  79 ++-
 src/tools/pgindent/typedefs.list  |   2 +
 10 files changed, 602 insertions(+), 81 deletions(-)

diff --git a/doc/src/sgml/high-availability.sgml b/doc/src/sgml/high-availability.sgml
index f49f5c0..2bbd52c 100644
--- a/doc/src/sgml/high-availability.sgml
+++ b/doc/src/sgml/high-availability.sgml
@@ -1700,14 +1700,14 @@ synchronous_standby_names = 'ANY 2 (s1, s2, s3)'

 

-During hot standby, the parameter transaction_read_only is always
-true and may not be changed.  But as long as no attempt is made to modify
-the database, connections during hot standby will act much like any other
-database connection.  If failover or switchover occurs, the database will
-switch to normal processing mode.  Sessions will remain connected while the
-server changes mode.  Once hot standby finishes, it will be possible to
-initiate read-write transactions (even from a session begun during
-hot standby).
+During hot standby, the parameter in_hot_standby and
+transaction_read_only are always true and may not be
+changed.  But as long as no attempt is made to modify the database,
+connections during hot standby will act much like any other database
+connection.  If failover or switchover occurs, the database will switch to
+normal processing mode.  Sessions will remain connected while the server
+changes mode.  Once hot standby finishes, it will be possible to initiate
+read-write transactions (even from a session begun during hot standby).

 

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index b7a8245..08a1b8e 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1836,18 +1836,66 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
   target_session_attrs
   

-If this parameter is set to read-write, only a
-connection in which read-write transactions are accepted 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
-string, any remaining servers will be tried just as if the connection
-

Re: Libpq support to connect to standby server as priority

[Greg Nancarrow]

On Wed, Feb 10, 2021 at 5:09 PM vignesh C  wrote:
>
> Modified.
> These comments are handled in v22 patch posted in my earlier mail.
>

Thanks, just one minor thing I missed in doc/src/sgml/libpq.sgml.

+The support of read-write transactions is determined by the
value of the
+default_transaction_read_only and
+in_hot_standby configuration parameters, that is
+reported by the server (if supported) upon successful connection.  If


should be:

+The support of read-write transactions is determined by the
values of the
+default_transaction_read_only and
+in_hot_standby configuration parameters, that are
+reported by the server (if supported) upon successful connection.  If


(i.e. "value" -> "values" and "is" -> "are")


Regards,
Greg Nancarrow
Fujitsu Australia

Re: Libpq support to connect to standby server as priority

[vignesh C]

Thanks for the comments Greg, please find my comments inline below.

On Tue, Feb 9, 2021 at 2:27 PM Greg Nancarrow  wrote:
>
> On Mon, Feb 8, 2021 at 8:17 PM vignesh C  wrote:
> >
> > >
> > > I think what we want to do is mark default_transaction_read_only as
> > > GUC_REPORT, instead.  That will give a reliable report of what the
> > > state of its GUC is, and you can combine it with is_hot_standby
> > > to decide whether the session should be considered read-only.
> > > If you don't get those two GUC values during connection, then you
> > > can fall back on "SHOW transaction_read_only".
> > >
> >
> > I have made a patch for the above with the changes suggested and
> > rebased it with the head code.
> > Attached v21 patch which has the changes for the same.
> > Thoughts?
>
> Further to my other doc change feedback, I can only spot the following
> minor things (otherwise the changes that you have made seek OK to me).
>
> 1) doc/src/sgml/protocol.sgml
>
>default_transaction_read_only  and
>in_hot_standby were not reported by releases before
>14.)
>
> should be:
>
>default_transaction_read_only  and
>in_hot_standby were not reported by releases before
>14.0)
>

Modified.

> 2) doc/src/sgml/high-availability,sgml
>
>
> During hot standby, the parameter in_hot_standby and
> default_transaction_read_only are always true and may
> not be changed.
>
> should be:
>
>
> During hot standby, the parameters in_hot_standby and
> transaction_read_only are always true and may
> not be changed.
>
>
> [I believe that there's only checks on attempts to change
> "transaction_read_only" when in hot_standby, not
> "default_transaction_read_only"; see  check_transaction_read_only()]
>

Modified.

> 3) src/interfaces/libpq/fe-connect.c
>
> In rejectCheckedReadOrWriteConnection() and
> rejectCheckedStandbyConnection(), now that host and port info are
> emitted separately and are not included in each error message string
> (as parameters in a format string), I think those functions should use
> appendPQExpBufferStr() instead of appendPQExpBuffer(), as it's more
> efficient if there is just a single string argument.
>

Modified.
These comments are handled in v22 patch posted in my earlier mail.

Regards,
VIgnesh

Re: Libpq support to connect to standby server as priority

[vignesh C]

On Tue, Feb 9, 2021 at 5:47 AM Greg Nancarrow  wrote:
>
> On Mon, Feb 8, 2021 at 8:17 PM vignesh C  wrote:
> >
> > >
> > > I think what we want to do is mark default_transaction_read_only as
> > > GUC_REPORT, instead.  That will give a reliable report of what the
> > > state of its GUC is, and you can combine it with is_hot_standby
> > > to decide whether the session should be considered read-only.
> > > If you don't get those two GUC values during connection, then you
> > > can fall back on "SHOW transaction_read_only".
> > >
> >
> > I have made a patch for the above with the changes suggested and
> > rebased it with the head code.
> > Attached v21 patch which has the changes for the same.
> > Thoughts?
> >
>
> I'm still looking at the patch code, but I noticed that the
> documentation update describing how support of read-write transactions
> is determined isn't quite right and it isn't clear how the parameters
> work.
> I'd suggest something like the following (you'd need to fix the line
> lengths and line-wrapping appropriately) - please check it for
> correctness:
>
>
> The support of read-write transactions is determined by the value of 
> the
> default_transaction_read_only and
> in_hot_standby configuration parameters,
> that, if supported,
> are reported by the server upon successful connection. If the
> value of either
> of these parameters is on, it means the
> server doesn't support
> read-write transactions. If either/both of these parameters
> are not reported,
> then the support of read-write transactions is determined by
> an explicit query,
> by sending SHOW transaction_read_only after
> successful
> connection; if it returns on, it means the
> server doesn't
> support read-write transactions. The standby mode state is
> determined by either
> the value of the in_hot_standby configuration
> parameter, that is reported by the server (if supported) upon
> successful connection, or is otherwise explicitly queried by sending
> SELECT pg_is_in_recovery() after successful
> connection; if it returns t, it means the server is
> in hot standby mode.
>

Thanks Greg for the comments, Please find the attached v22 patch
having the fix for the same.
Thoughts?

Regards,
Vignesh
From db9b894d8a8c51de80e6b3b7b8c660dda5374ff9 Mon Sep 17 00:00:00 2001
From: Vignesh C 
Date: Mon, 8 Feb 2021 11:23:31 +0530
Subject: [PATCH v22] Enhance the libpq "target_session_attrs" connection
 parameter.

Enhance the connection parameter "target_session_attrs" to support new values:
read-only/primary/standby/prefer-standby.
Add a new "read-only" target_session_attrs option value, to support connecting
to a read-only server if available from the list of hosts (otherwise the
connection attempt fails).
Add a new "primary" target_session_attrs option value, to support connecting to
a server which is not in hot-standby mode, if available from the list of hosts
(otherwise the connection attempt fails).
Add a new "standby" target_session_attrs option value, to support connecting to
a server which is in hot-standby mode, if available from the list of hosts
(otherwise the connection attempt fails).
Add a new "prefer-standby" target_session_attrs option value, to support
connecting to a server which is in hot-standby mode, if available from the list
of hosts (otherwise connect to a server which is not in hot-standby mode).

Discussion: https://www.postgresql.org/message-id/flat/caf3+xm+8-ztokav9ghij3wfgentq97qcjxqt+rbfq6f7onz...@mail.gmail.com
---
 doc/src/sgml/high-availability.sgml   |  16 +-
 doc/src/sgml/libpq.sgml   |  73 +-
 doc/src/sgml/protocol.sgml|   5 +-
 src/backend/utils/misc/guc.c  |   3 +-
 src/interfaces/libpq/fe-connect.c | 432 ++
 src/interfaces/libpq/fe-exec.c|  18 +-
 src/interfaces/libpq/libpq-fe.h   |   3 +-
 src/interfaces/libpq/libpq-int.h  |  52 +++-
 src/test/recovery/t/001_stream_rep.pl |  79 ++-
 src/tools/pgindent/typedefs.list  |   2 +
 10 files changed, 602 insertions(+), 81 deletions(-)

diff --git a/doc/src/sgml/high-availability.sgml b/doc/src/sgml/high-availability.sgml
index f49f5c0..2bbd52c 100644
--- a/doc/src/sgml/high-availability.sgml
+++ b/doc/src/sgml/high-availability.sgml
@@ -1700,14 +1700,14 @@ synchronous_standby_names = 'ANY 2 (s1, s2, s3)'

 

-During hot standby, the parameter transaction_read_only is always
-true and may not be changed.  But as long as no attempt is made to modify
-the database, connections during hot standby will act much like any other
-database connection.  If failover or switchover occurs, the database will
-switch to normal processing mode.  Sessions will remain connected while the
-server changes mode.  Once hot standby finishes, it will be possible to
-initiate read-write transactions 

Re: Libpq support to connect to standby server as priority

[Greg Nancarrow]

On Mon, Feb 8, 2021 at 8:17 PM vignesh C  wrote:
>
> >
> > I think what we want to do is mark default_transaction_read_only as
> > GUC_REPORT, instead.  That will give a reliable report of what the
> > state of its GUC is, and you can combine it with is_hot_standby
> > to decide whether the session should be considered read-only.
> > If you don't get those two GUC values during connection, then you
> > can fall back on "SHOW transaction_read_only".
> >
>
> I have made a patch for the above with the changes suggested and
> rebased it with the head code.
> Attached v21 patch which has the changes for the same.
> Thoughts?

Further to my other doc change feedback, I can only spot the following
minor things (otherwise the changes that you have made seek OK to me).

1) doc/src/sgml/protocol.sgml

   default_transaction_read_only  and
   in_hot_standby were not reported by releases before
   14.)

should be:

   default_transaction_read_only  and
   in_hot_standby were not reported by releases before
   14.0)

2) doc/src/sgml/high-availability,sgml

   
During hot standby, the parameter in_hot_standby and
default_transaction_read_only are always true and may
not be changed.

should be:

   
During hot standby, the parameters in_hot_standby and
transaction_read_only are always true and may
not be changed.


[I believe that there's only checks on attempts to change
"transaction_read_only" when in hot_standby, not
"default_transaction_read_only"; see  check_transaction_read_only()]


3) src/interfaces/libpq/fe-connect.c

In rejectCheckedReadOrWriteConnection() and
rejectCheckedStandbyConnection(), now that host and port info are
emitted separately and are not included in each error message string
(as parameters in a format string), I think those functions should use
appendPQExpBufferStr() instead of appendPQExpBuffer(), as it's more
efficient if there is just a single string argument.


Regards,
Greg Nancarrow
Fujitsu Australia

Re: Libpq support to connect to standby server as priority

[Greg Nancarrow]

On Mon, Feb 8, 2021 at 8:17 PM vignesh C  wrote:
>
> >
> > I think what we want to do is mark default_transaction_read_only as
> > GUC_REPORT, instead.  That will give a reliable report of what the
> > state of its GUC is, and you can combine it with is_hot_standby
> > to decide whether the session should be considered read-only.
> > If you don't get those two GUC values during connection, then you
> > can fall back on "SHOW transaction_read_only".
> >
>
> I have made a patch for the above with the changes suggested and
> rebased it with the head code.
> Attached v21 patch which has the changes for the same.
> Thoughts?
>

I'm still looking at the patch code, but I noticed that the
documentation update describing how support of read-write transactions
is determined isn't quite right and it isn't clear how the parameters
work.
I'd suggest something like the following (you'd need to fix the line
lengths and line-wrapping appropriately) - please check it for
correctness:

   
The support of read-write transactions is determined by the value of the
default_transaction_read_only and
in_hot_standby configuration parameters,
that, if supported,
are reported by the server upon successful connection. If the
value of either
of these parameters is on, it means the
server doesn't support
read-write transactions. If either/both of these parameters
are not reported,
then the support of read-write transactions is determined by
an explicit query,
by sending SHOW transaction_read_only after
successful
connection; if it returns on, it means the
server doesn't
support read-write transactions. The standby mode state is
determined by either
the value of the in_hot_standby configuration
parameter, that is reported by the server (if supported) upon
successful connection, or is otherwise explicitly queried by sending
SELECT pg_is_in_recovery() after successful
connection; if it returns t, it means the server is
in hot standby mode.
   


Regards,
Greg Nancarrow
Fujitsu Australia

Re: Libpq support to connect to standby server as priority

[vignesh C]

On Wed, Jan 6, 2021 at 3:05 AM Tom Lane  wrote:
>
> Greg Nancarrow  writes:
> > Posting an updated set of patches.
>
> I've reviewed and pushed most of v20-0001, with the following changes:
>
> * I realized that we had more moving parts than necessary for
> in_hot_standby.  We don't really need two static variables, one is
> sufficient --- and we shouldn't make the SHOW hook have side-effects,
> that's just dangerous.
>
> * The documentation patches were missing an addition to config.sgml,
> as well as failing to list the new GUC in the two places where we
> document all GUC_REPORT variables.
>
> What I did *not* push was the change to mark transaction_read_only
> as GUC_REPORT.  I find that idea highly dubious, for a couple of
> reasons:
>
> * It'll create useless ParameterStatus traffic during normal operations
> of an application using "START TRANSACTION READ ONLY" or the like.
>
> * I do not think it will actually work for the desired purpose of
> finding out the read-only state during session connection.  AFAICS,
> we don't set XactReadOnly to a meaningful value except when starting
> a transaction.  Yeah, we'll run a transaction during login because
> we have to examine the system catalogs ... but do we start a new
> one after absorbing the effects of, say, ALTER USER SET
> default_transaction_read_only?  I doubt it, and even if it works
> today it'd be fragile, because someday somebody will try to collapse
> any multiple transactions during startup into one transaction.
>
> I think what we want to do is mark default_transaction_read_only as
> GUC_REPORT, instead.  That will give a reliable report of what the
> state of its GUC is, and you can combine it with is_hot_standby
> to decide whether the session should be considered read-only.
> If you don't get those two GUC values during connection, then you
> can fall back on "SHOW transaction_read_only".
>

I have made a patch for the above with the changes suggested and
rebased it with the head code.
Attached v21 patch which has the changes for the same.
Thoughts?

Regards,
Vignesh
From 9d85abfe1e4b43d67ee746891830abe53077c0e7 Mon Sep 17 00:00:00 2001
From: Vignesh C 
Date: Mon, 8 Feb 2021 11:23:31 +0530
Subject: [PATCH v21] Enhance the libpq "target_session_attrs" connection 
 parameter.

Enhance the connection parameter "target_session_attrs" to support new values:
read-only/primary/standby/prefer-standby.
Add a new "read-only" target_session_attrs option value, to support connecting
to a read-only server if available from the list of hosts (otherwise the
connection attempt fails).
Add a new "primary" target_session_attrs option value, to support connecting to
a server which is not in hot-standby mode, if available from the list of hosts
(otherwise the connection attempt fails).
Add a new "standby" target_session_attrs option value, to support connecting to
a server which is in hot-standby mode, if available from the list of hosts
(otherwise the connection attempt fails).
Add a new "prefer-standby" target_session_attrs option value, to support
connecting to a server which is in hot-standby mode, if available from the list
of hosts (otherwise connect to a server which is not in hot-standby mode).

Discussion: https://www.postgresql.org/message-id/flat/CAF3+xM+8-ztOkaV9gHiJ3wfgENTq97QcjXQt+rbFQ6F7oNzt9A@mail.gmail.com
---
 doc/src/sgml/high-availability.sgml   |  16 +-
 doc/src/sgml/libpq.sgml   |  70 +-
 doc/src/sgml/protocol.sgml|   5 +-
 src/backend/utils/misc/guc.c  |   3 +-
 src/interfaces/libpq/fe-connect.c | 432 ++
 src/interfaces/libpq/fe-exec.c|  18 +-
 src/interfaces/libpq/libpq-fe.h   |   3 +-
 src/interfaces/libpq/libpq-int.h  |  52 +++-
 src/test/recovery/t/001_stream_rep.pl |  79 ++-
 src/tools/pgindent/typedefs.list  |   2 +
 10 files changed, 599 insertions(+), 81 deletions(-)

diff --git a/doc/src/sgml/high-availability.sgml b/doc/src/sgml/high-availability.sgml
index f49f5c0..a454e93 100644
--- a/doc/src/sgml/high-availability.sgml
+++ b/doc/src/sgml/high-availability.sgml
@@ -1700,14 +1700,14 @@ synchronous_standby_names = 'ANY 2 (s1, s2, s3)'

 

-During hot standby, the parameter transaction_read_only is always
-true and may not be changed.  But as long as no attempt is made to modify
-the database, connections during hot standby will act much like any other
-database connection.  If failover or switchover occurs, the database will
-switch to normal processing mode.  Sessions will remain connected while the
-server changes mode.  Once hot standby finishes, it will be possible to
-initiate read-write transactions (even from a session begun during
-hot standby).
+During hot standby, the parameter in_hot_standby and
+default_transaction_read_only are always true and may
+not be changed.  But as long as no attempt is made to modify the database,
+connections during hot standby will act 

Re: Libpq support to connect to standby server as priority

[Tom Lane]

Greg Nancarrow  writes:
> Posting an updated set of patches.

I've reviewed and pushed most of v20-0001, with the following changes:

* I realized that we had more moving parts than necessary for
in_hot_standby.  We don't really need two static variables, one is
sufficient --- and we shouldn't make the SHOW hook have side-effects,
that's just dangerous.

* The documentation patches were missing an addition to config.sgml,
as well as failing to list the new GUC in the two places where we
document all GUC_REPORT variables.

What I did *not* push was the change to mark transaction_read_only
as GUC_REPORT.  I find that idea highly dubious, for a couple of
reasons:

* It'll create useless ParameterStatus traffic during normal operations
of an application using "START TRANSACTION READ ONLY" or the like.

* I do not think it will actually work for the desired purpose of
finding out the read-only state during session connection.  AFAICS,
we don't set XactReadOnly to a meaningful value except when starting
a transaction.  Yeah, we'll run a transaction during login because
we have to examine the system catalogs ... but do we start a new
one after absorbing the effects of, say, ALTER USER SET
default_transaction_read_only?  I doubt it, and even if it works
today it'd be fragile, because someday somebody will try to collapse
any multiple transactions during startup into one transaction.

I think what we want to do is mark default_transaction_read_only as
GUC_REPORT, instead.  That will give a reliable report of what the
state of its GUC is, and you can combine it with is_hot_standby
to decide whether the session should be considered read-only.
If you don't get those two GUC values during connection, then you
can fall back on "SHOW transaction_read_only".

Setting this back to waiting on author.

regards, tom lane

Re: Libpq support to connect to standby server as priority

[Greg Nancarrow]

On Thu, Nov 26, 2020 at 11:07 AM Greg Nancarrow  wrote:
>
> On Thu, Nov 26, 2020 at 3:43 AM Tom Lane  wrote:
> >
> > Thanks for looking!  Pushed.
> >
> > At this point the cfbot is going to start complaining that the last-posted
> > patch in this thread no longer applies.  Unless you have a new patch set
> > nearly ready to post, I think we should close the CF entry as RWF, and
> > then you can open a new one when you're ready.
> >
>
> Actually, the cfbot shouldn't be complaining, as my last-posted patch
> still seems to apply cleanly on the latest code (with your pushed
> patch), and all tests pass.
> Hopefully it's OK to let it roll over to the next CF with the WOA status.
> I am actively working on processing the feedback and updating the
> patch, and hope to post an update next week sometime.
>

Posting an updated set of patches.

Regards,
Greg Nancarrow
Fujitsu Australia


v20-0001-in_hot_standby-and-transaction_read_only-reportable-GUCs.patch
Description: Binary data


v20-0002-Enhance-libpq-target_session_attrs.patch
Description: Binary data

Re: Libpq support to connect to standby server as priority

[Greg Nancarrow]

On Thu, Nov 26, 2020 at 3:43 AM Tom Lane  wrote:
>
> Thanks for looking!  Pushed.
>
> At this point the cfbot is going to start complaining that the last-posted
> patch in this thread no longer applies.  Unless you have a new patch set
> nearly ready to post, I think we should close the CF entry as RWF, and
> then you can open a new one when you're ready.
>

Actually, the cfbot shouldn't be complaining, as my last-posted patch
still seems to apply cleanly on the latest code (with your pushed
patch), and all tests pass.
Hopefully it's OK to let it roll over to the next CF with the WOA status.
I am actively working on processing the feedback and updating the
patch, and hope to post an update next week sometime.

Regards,
Greg Nancarrow
Fujitsu Australia

Re: Libpq support to connect to standby server as priority

[Tom Lane]

Greg Nancarrow  writes:
> On Wed, Nov 25, 2020 at 12:07 PM Tom Lane  wrote:
>> Here's a v2 that does it like that.

> Looks OK to me.

Thanks for looking!  Pushed.

At this point the cfbot is going to start complaining that the last-posted
patch in this thread no longer applies.  Unless you have a new patch set
nearly ready to post, I think we should close the CF entry as RWF, and
then you can open a new one when you're ready.

regards, tom lane

Re: Libpq support to connect to standby server as priority

[Greg Nancarrow]

On Wed, Nov 25, 2020 at 12:07 PM Tom Lane  wrote:
>
>
> Here's a v2 that does it like that.
>

Looks OK to me.



Regards,
Greg Nancarrow
Fujitsu Australia

Re: Libpq support to connect to standby server as priority

[Tom Lane]

I wrote:
> Alvaro Herrera  writes:
>> Agreed.  If this is just a few hundred bytes of server-side local memory
>> per backend, it seems definitely worth it.

> Yeah, given the current set of GUC_REPORT variables, it's hard to see
> the storage for their last-reported values amounting to much.  The need
> for an extra pointer field in each GUC variable record might eat more
> space than the actually-live values :-(

Here's a v2 that does it like that.

regards, tom lane

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 7c5f7c775b..34ed0e7558 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -4229,6 +4229,9 @@ PostgresMain(int argc, char *argv[],
 pgstat_report_activity(STATE_IDLE, NULL);
 			}
 
+			/* Report any recently-changed GUC options */
+			ReportChangedGUCOptions();
+
 			ReadyForQuery(whereToSendOutput);
 			send_ready_for_query = false;
 		}
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index bb34630e8e..245a3472bc 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -4822,6 +4822,8 @@ static bool guc_dirty;			/* true if need to do commit/abort work */
 
 static bool reporting_enabled;	/* true to enable GUC_REPORT */
 
+static bool report_needed;		/* true if any GUC_REPORT reports are needed */
+
 static int	GUCNestLevel = 0;	/* 1 when in main transaction */
 
 
@@ -5452,6 +5454,7 @@ InitializeOneGUCOption(struct config_generic *gconf)
 	gconf->reset_scontext = PGC_INTERNAL;
 	gconf->stack = NULL;
 	gconf->extra = NULL;
+	gconf->last_reported = NULL;
 	gconf->sourcefile = NULL;
 	gconf->sourceline = 0;
 
@@ -5828,7 +5831,10 @@ ResetAllOptions(void)
 		gconf->scontext = gconf->reset_scontext;
 
 		if (gconf->flags & GUC_REPORT)
-			ReportGUCOption(gconf);
+		{
+			gconf->status |= GUC_NEEDS_REPORT;
+			report_needed = true;
+		}
 	}
 }
 
@@ -6215,7 +6221,10 @@ AtEOXact_GUC(bool isCommit, int nestLevel)
 
 			/* Report new value if we changed it */
 			if (changed && (gconf->flags & GUC_REPORT))
-ReportGUCOption(gconf);
+			{
+gconf->status |= GUC_NEEDS_REPORT;
+report_needed = true;
+			}
 		}		/* end of stack-popping loop */
 
 		if (stack != NULL)
@@ -6257,17 +6266,60 @@ BeginReportingGUCOptions(void)
 		if (conf->flags & GUC_REPORT)
 			ReportGUCOption(conf);
 	}
+
+	report_needed = false;
+}
+
+/*
+ * ReportChangedGUCOptions: report recently-changed GUC_REPORT variables
+ *
+ * This is called just before we wait for a new client query.
+ *
+ * By handling things this way, we ensure that a ParameterStatus message
+ * is sent at most once per variable per query, even if the variable
+ * changed multiple times within the query.  That's quite possible when
+ * using features such as function SET clauses.  Function SET clauses
+ * also tend to cause values to change intraquery but eventually revert
+ * to their prevailing values; ReportGUCOption is responsible for avoiding
+ * redundant reports in such cases.
+ */
+void
+ReportChangedGUCOptions(void)
+{
+	/* Quick exit if not (yet) enabled */
+	if (!reporting_enabled)
+		return;
+
+	/* Quick exit if no values have been changed */
+	if (!report_needed)
+		return;
+
+	/* Transmit new values of interesting variables */
+	for (int i = 0; i < num_guc_variables; i++)
+	{
+		struct config_generic *conf = guc_variables[i];
+
+		if ((conf->flags & GUC_REPORT) && (conf->status & GUC_NEEDS_REPORT))
+			ReportGUCOption(conf);
+	}
+
+	report_needed = false;
 }
 
 /*
  * ReportGUCOption: if appropriate, transmit option value to frontend
+ *
+ * We need not transmit the value if it's the same as what we last
+ * transmitted.  However, clear the NEEDS_REPORT flag in any case.
  */
 static void
 ReportGUCOption(struct config_generic *record)
 {
-	if (reporting_enabled && (record->flags & GUC_REPORT))
+	char	   *val = _ShowOption(record, false);
+
+	if (record->last_reported == NULL ||
+		strcmp(val, record->last_reported) != 0)
 	{
-		char	   *val = _ShowOption(record, false);
 		StringInfoData msgbuf;
 
 		pq_beginmessage(, 'S');
@@ -6275,8 +6327,19 @@ ReportGUCOption(struct config_generic *record)
 		pq_sendstring(, val);
 		pq_endmessage();
 
-		pfree(val);
+		/*
+		 * We need a long-lifespan copy.  If strdup() fails due to OOM, we'll
+		 * set last_reported to NULL and thereby possibly make a duplicate
+		 * report later.
+		 */
+		if (record->last_reported)
+			free(record->last_reported);
+		record->last_reported = strdup(val);
 	}
+
+	pfree(val);
+
+	record->status &= ~GUC_NEEDS_REPORT;
 }
 
 /*
@@ -7695,7 +7758,10 @@ set_config_option(const char *name, const char *value,
 	}
 
 	if (changeVal && (record->flags & GUC_REPORT))
-		ReportGUCOption(record);
+	{
+		record->status |= GUC_NEEDS_REPORT;
+		report_needed = true;
+	}
 
 	return changeVal ? 1 : -1;
 }
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 073c8f3e06..6a20a3bcec 100644
--- a/src/include/utils/guc.h
+++ 

Re: Libpq support to connect to standby server as priority

[Tom Lane]

Alvaro Herrera  writes:
> On 2020-Nov-24, Tom Lane wrote:
>>> As it stands, 0001 reduces the ParameterStatus message traffic to
>>> at most one per GUC per query, but it doesn't attempt to eliminate
>>> duplicate ParameterStatus messages altogether.  We could do that
>>> as a pretty simple adjustment if we're willing to expend the storage
>>> to remember the last value sent to the client.  It might be worth
>>> doing, since for example the function-SET-clause case would typically
>>> lead to no net change in the GUC's value by the end of the query.

>> On reflection this seems worth doing, since excess client traffic
>> is far from free.

> Agreed.  If this is just a few hundred bytes of server-side local memory
> per backend, it seems definitely worth it.

Yeah, given the current set of GUC_REPORT variables, it's hard to see
the storage for their last-reported values amounting to much.  The need
for an extra pointer field in each GUC variable record might eat more
space than the actually-live values :-(

regards, tom lane

Re: Libpq support to connect to standby server as priority

[Alvaro Herrera]

On 2020-Nov-24, Tom Lane wrote:

> I'm inclined to go ahead and commit the 0001 patch I posted at [1]
> (ie, change the implementation of GUC_REPORT to avoid intra-query
> reports), since that addresses a performance problem that's
> independent of the goal here.  The rest of this seems to still
> be in Greg's court.

Sounded a good idea to me.

> Has anyone got an opinion about the further improvement I suggested:
> 
> >> As it stands, 0001 reduces the ParameterStatus message traffic to
> >> at most one per GUC per query, but it doesn't attempt to eliminate
> >> duplicate ParameterStatus messages altogether.  We could do that
> >> as a pretty simple adjustment if we're willing to expend the storage
> >> to remember the last value sent to the client.  It might be worth
> >> doing, since for example the function-SET-clause case would typically
> >> lead to no net change in the GUC's value by the end of the query.
> 
> On reflection this seems worth doing, since excess client traffic
> is far from free.

Agreed.  If this is just a few hundred bytes of server-side local memory
per backend, it seems definitely worth it.

Re: Libpq support to connect to standby server as priority

[Tom Lane]

Anastasia Lubennikova  writes:
> Hi, this entry is "Waiting on Author" and the thread was inactive for a 
> while. As far as I see, the patch needs some further work.
> Are you going to continue working on it, or should I mark it as 
> "returned with feedback" until a better time?

I'm inclined to go ahead and commit the 0001 patch I posted at [1]
(ie, change the implementation of GUC_REPORT to avoid intra-query
reports), since that addresses a performance problem that's
independent of the goal here.  The rest of this seems to still
be in Greg's court.

Has anyone got an opinion about the further improvement I suggested:

>> As it stands, 0001 reduces the ParameterStatus message traffic to
>> at most one per GUC per query, but it doesn't attempt to eliminate
>> duplicate ParameterStatus messages altogether.  We could do that
>> as a pretty simple adjustment if we're willing to expend the storage
>> to remember the last value sent to the client.  It might be worth
>> doing, since for example the function-SET-clause case would typically
>> lead to no net change in the GUC's value by the end of the query.

On reflection this seems worth doing, since excess client traffic
is far from free.

regards, tom lane

[1] https://www.postgresql.org/message-id/5708.1601145259%40sss.pgh.pa.us

Re: Libpq support to connect to standby server as priority

[Anastasia Lubennikova]

On 30.09.2020 10:57, Greg Nancarrow wrote:

Thanks for your thoughts, patches and all the pointers.
I'll be looking at all of them.
(And yes, the comma instead of bitwise OR is of course an error,
somehow made and gone unnoticed; the next field in the struct is an
enum, so accepts any int value).

Regards,
Greg Nancarrow
Fujitsu Australia


CFM reminder.

Hi, this entry is "Waiting on Author" and the thread was inactive for a 
while. As far as I see, the patch needs some further work.
Are you going to continue working on it, or should I mark it as 
"returned with feedback" until a better time?


--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: Libpq support to connect to standby server as priority

[Greg Nancarrow]

On Sun, Sep 27, 2020 at 4:34 AM Tom Lane  wrote:
>
>
> Thoughts?
>

Thanks for your thoughts, patches and all the pointers.
I'll be looking at all of them.
(And yes, the comma instead of bitwise OR is of course an error,
somehow made and gone unnoticed; the next field in the struct is an
enum, so accepts any int value).

Regards,
Greg Nancarrow
Fujitsu Australia

Re: Libpq support to connect to standby server as priority

[Tom Lane]

I wrote:
> BTW, I think it would be worth splitting this into separate server-side
> and libpq patches.  It looked to me like the server side is pretty
> nearly committable, modulo bikeshedding about the new GUC name.

Actually ... I looked that over again and got a bit more queasy about
all the new signaling logic it is adding.  Signals are inherently
bug-prone stuff, plus it's not very clear what sort of guarantees
we'd have about either the reliability or the timeliness of client
notifications about exiting hot-standby mode.

I also wonder what consideration has been given to the performance
implications of marking transaction_read_only as GUC_REPORT, thus
causing client traffic to occur every time it's changed.  Most of
the current GUC_REPORT variables don't change too often in typical
sessions, but I'm less convinced about that for transaction_read_only.

So I thought about alternative ways to implement this, and realized
that it would not be hard to make guc.c handle it all by itself, if
we use a custom show-hook for the in_hot_standby GUC that calls
RecoveryInProgress() instead of examining purely static state.
Now, by itself that idea only takes care of the session-start-time
report, because there'd never be any GUC action causing a new
report to occur.  But we can improve the situation if we get rid
of the current design whereby ReportGUCOption() is called immediately
when any GUC value changes, and instead batch up the reports to
occur when we're about to go idle waiting for a new client query.

Not incidentally, this responds to a concern Robert mentioned awhile
back about the performance of GUC reporting [1].  You can already get
the server to spam the client excessively if any GUC_REPORT variables
are changed by, for example, functions' SET clauses, because that could
lead to the active value changing many times within a query.  We've
gotten away with that so far, but it'd be a problem if any more-often-
changed variables get marked GUC_REPORT.  (I actually have a vague
memory of other complaints about that, but I couldn't find any in a
desultory search of the archives.)

So I present 0001 attached which changes the GUC_REPORT code to work
that way, and then 0002 is a pretty small hack to add a reportable
in_hot_standby GUC by having the end-of-query function check (when
needed) to see if the active value changed.

As it stands, 0001 reduces the ParameterStatus message traffic to
at most one per GUC per query, but it doesn't attempt to eliminate
duplicate ParameterStatus messages altogether.  We could do that
as a pretty simple adjustment if we're willing to expend the storage
to remember the last value sent to the client.  It might be worth
doing, since for example the function-SET-clause case would typically
lead to no net change in the GUC's value by the end of the query.

An objection that could be raised to this approach for in_hot_standby
is that it will only report in_hot_standby becoming false at the end
of a query, whereas the v19 patch at least attempts to deliver an
async ParameterStatus message when the client is idle (and, I think,
indeed may fail to provide *any* message if the transition occurs
when it isn't idle).  I don't find that too compelling though;
libpq-based clients, at least, don't have any very practical way to
deal with async ParameterStatus messages anyway.

(Note that I did not touch the docs here, so that while 0001 might
be committable as-is, 0002 is certainly just WIP.)

BTW, as far as the transaction_read_only side of things goes, IMO
it would make a lot more sense to mark default_transaction_read_only
as GUC_REPORT, since that changes a lot less frequently.  We'd then
have to expend some work to report that value honestly, since right
now the hot-standby code cheats by ignoring the GUC's value during
hot standby.  But I think a technique much like 0002's would work
for that.

Thoughts?

regards, tom lane

[1] 
https://www.postgresql.org/message-id/CA%2BTgmoaDoVtMnfKNFm-iyyCSp%3DFPiHkfU1AXuEHJqmcLTAX6kQ%40mail.gmail.com

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 411cfadbff..b67cc2f375 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -4233,6 +4233,9 @@ PostgresMain(int argc, char *argv[],
 pgstat_report_activity(STATE_IDLE, NULL);
 			}
 
+			/* Report any recently-changed GUC options */
+			ReportChangedGUCOptions();
+
 			ReadyForQuery(whereToSendOutput);
 			send_ready_for_query = false;
 		}
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 596bcb7b84..ddfc7ea05d 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -4822,6 +4822,8 @@ static bool guc_dirty;			/* true if need to do commit/abort work */
 
 static bool reporting_enabled;	/* true to enable GUC_REPORT */
 
+static bool report_needed;		/* true if any GUC_REPORT reports are needed */
+
 static int	GUCNestLevel = 0;	/* 1 when in main transaction */
 
 

Re: Libpq support to connect to standby server as priority

[Tom Lane]

Greg Nancarrow  writes:
> [ v19-0001-Enhance-libpq-target_session_attrs-and-add-target_se.patch ]

I started to look through this, and I find that I'm really pretty
disappointed in the direction the patch has gone of late.  I think
there is no defensible reason for the choices that have been made
to have different behavior for v14-and-up servers than for older
servers.  It's not necessary and it complicates life for users.
We can use pg_is_in_recovery() on every server version that has
hot standby at all, so there is no reason not to treat the
GUC_REPORT indicator as an optimization that lets us skip a
separate inquiry transaction, rather than something we have to
have to make the feature work correctly.

So I think what we ought to have is the existing read-write
vs read-only distinction, implemented as it is now by checking
"SHOW transaction_read_only" if the server fails to send that
as a GUC_REPORT value; and orthogonally to that, a primary/standby
distinction implemented by checking pg_is_in_recovery(), again
with a fast path if we got a ParameterStatus report.

I do not like the addition of target_server_type.  It seems
unnecessary and confusing, particularly because you've had to
make a completely arbitrary decision about how it interacts with
target_session_attrs when both are specified.  I think the
justification that "it's more like JDBC" is risible.  Any user of
this will be writing C not Java.
 
A couple of other thoughts:

* Could we call the GUC "in_hot_standby" rather than "in_recovery"?
I think "recovery" is a poorly chosen legacy term that we ought to
avoid exposing to users more than we already have.  We're stuck
with pg_is_in_recovery() I suppose, but let's not double down on
bad decisions.

* I don't think you really need a hard-wired test on v14-or-not
in the libpq code.  The internal state about read-only and
hot-standby ought to be "yes", "no", or "unknown", starting in
the latter state.  Receipt of ParameterStatus changes it from
"unknown" to one of the other two states.  If we need to know
the value, and it's still "unknown", then we send a probe query.
We still need hard-coded version checks to know if the probe
query is safe, but those version breaks are far enough back to
be pretty well set in stone.  (In the back of my mind here is
that people might well choose to back-port the GUC_REPORT marking
of transaction_read_only, and maybe even the other GUC if they
were feeling ambitious.  So not having a hard-coded version
assumption where we don't particularly need it seems a good thing.)

* This can't be right can it?  Too many commas.

@@ -1618,7 +1619,7 @@ static struct config_bool ConfigureNamesBool[] =
 {"transaction_read_only", PGC_USERSET, CLIENT_CONN_STATEMENT,
 gettext_noop("Sets the current transaction's read-only status."),
 NULL,
-GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
+GUC_REPORT, GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | 
GUC_DISALLOW_IN_FILE
 },
 ,
 false,

(The compiler will fail to bitch about that unfortunately, since
there are more struct fields that we leave uninitialized normally.)

BTW, I think it would be worth splitting this into separate server-side
and libpq patches.  It looked to me like the server side is pretty
nearly committable, modulo bikeshedding about the new GUC name.  We could
get that out of the way and then have a much smaller libpq patch to argue
about.

regards, tom lane

Re: Libpq support to connect to standby server as priority

[Peter Smith]

Hi Greg,

> Thanks for the further review, an updated patch is attached. Please
> see my responses to your comments below:
>

Thanks for addressing all of my previous review comments in your new v19 patch.

Everything looks good to me now, so I am marking this as "ready for committer".

Kind Regards,
Peter Smith.
Fujitsu Australia

Re: Libpq support to connect to standby server as priority

[Greg Nancarrow]

Hi Peter,

Thanks for the further review, an updated patch is attached. Please
see my responses to your comments below:


On Thu, Aug 20, 2020 at 11:36 AM Peter Smith  wrote:
>

>
> COMMENT (help text)
>
> The help text is probably accurate but it does seem a bit confusing still.
>
> ...
>
> IMO if there was some up-front paragraphs to say how different
> versions calculate the r/w support and recovery mode, then all the
> different parameter values can be expressed in a much simpler way and
> have less repetition (e.g they can all look like the "read-only" one
> does now).
>

GN RESPONSE:
I have updated the documentation, taking this view into account.


> 
>
> COMMENT fe-connect.c (sizeof)
>
> - "Target-Session-Attrs", "", 11, /* sizeof("read-write") = 11 */
> + "Target-Session-Attrs", "", 15, /* sizeof("prefer-standby") = 15 */
>
> You said changing this 15 to 17 is debatable. So I will debate it.
>
> IIUC the dispsize is defined as /* Field size in characters for dialog */
> I imagine this could be used as potential max character length of a
> text input field.
>
> Therefore, so long as "prefer-secondary" remains a valid value for
> target_session_attrs then I think dispsize ought to be 17 (not 15) to
> accommodate it.
> Otherwise setting to 15 may be preventing dialog entry of this
> perfectly valid (albeit uncommon) value.
>

GN RESPONSE:
My initial reasoning was that even though "prefer-secondary" is a
valid value, a GUI for target_session_attrs probably wouldn't present
that option, it would present "prefer-standby" instead (I was
imagining a drop-down menu, and it certainly wouldn't present both
"prefer-standby" and "prefer-secondary", as they are synonyms). If the
GUI did want to present the PGJDBC-compatible option values, it should
be looking at the dispsize for "Target-Server-Type" (which is 17, for
"prefer-secondary").
However, I guess there could be a number of ways to specify the option
value, even explicitly typing it into a textbox in the "database
connection dialog" that uses this information.
So in that case, I've updated the code, as you suggested, to use
dispsize=17 (for "prefer-secondary") in this case.


> 
>
> COMMENT (typo)
>
> + /*
> + * Type of server to connect to. Possible values: "any", "primary",
> + * "prefer-secondary", "secondary" This overrides any connection type
> + * specified by target_session_attrs. This option supports a subset of the
>
> Missing period before "This overrides"
>

GN RESPONSE: Fixed.

> 
>
> COMMENT (comment)
>
> + /*
> + * Type of server to connect to. Possible values: "any", "primary",
> + * "prefer-secondary", "secondary" This overrides any connection type
> + * specified by target_session_attrs. This option supports a subset of the
> + * target_session_attrs option values, and its purpose is to closely
> + * reflect the similar PGJDBC targetServerType option. Note also that this
> + * option only accepts single option values, whereas in future,
> + * target_session_attrs may accept multiple session attribute values.
> + */
> + char*target_server_type;
>
> Perhaps the part saying "... in future, target_session_attrs may
> accept multiple session attribute values." more rightly belongs as a
> comment for the *target_session_attrs field.
>

GN RESPONSE:
I can't really compare and contrast the two parameters without
mentioning "target_session_attrs" here.
"target_session_attrs" implies the possibility of multiple attributes.
If the difference between the attributes is provided in separate bits
of information for each attribute, the reader may not pick up on this
subtle difference between them.

> 
>
> COMMENT (comments)
>
> @@ -436,6 +486,8 @@ struct pg_conn
>   pgParameterStatus *pstatus; /* ParameterStatus data */
>   int client_encoding; /* encoding id */
>   bool std_strings; /* standard_conforming_strings */
> + bool transaction_read_only; /* transaction_read_only */
> + bool in_recovery; /* in_recovery */
>
> Just repeating the field name does not make for a very useful comment.
> Can it be improved?
>

GN RESPONSE: Yes, improved.


>
> COMMENT (blank line removal?)
>
> @@ -540,7 +592,6 @@ struct pg_cancel
>   int be_key; /* key of backend --- needed for cancels */
>  };
>
> -
>
> Removal of this blank line is cleanup in some place unrelated to this patch.
>

GN RESPONSE:
Blank line put back - but this appears to be because pg_indent was NOT
previously run on this code prior to me running it.

>
> COMMENT (typo in test comment)
>
> +# Connect to standby1 in "prefer-ssecondary" mode with standby1,primary list.
> +test_target_session_attrs($node_standby_1, $node_primary, $node_standby_1,
> + "prefer-secondary", 0);
> +
>
> Typo: "prefer-ssecondary"
>

GN RESPONSE: Fixed.


>
> COMMENT (fe-connect.c - suggest if/else instead of if/if)
>
> + /*
> + * For servers before 7.4 (which don't support read-only), if
> + * the requested type of connection is prefer-standby, then
> + * record this host index and try other specified 

Re: Libpq support to connect to standby server as priority

[Peter Smith]

On Thu, Aug 20, 2020 at 10:26 AM Greg Nancarrow  wrote:

> I have updated the patch (attached) based on your comments, with
> adjustments made for additional changes based on feedback (which I
> tend to agree with) from Robert Haas and Tsunakawa san, who suggested
> read-write/read-only should be functionally different to
> primary/standby, and not just have "read-write" a synonym for
> "primary".
> I also thought it appropriate to remove "read-write", "standby" and
> "prefer-standby" from accepted values for "target_server_type"
> (instead just support "secondary" and "prefer-secondary") to match the
> similar targetServerType PGJDBC option.
> So currently have as supported option values:
>
> target_session_attrs:
> any/read-write/read-only/primary/standby(/secondary)/prefer-standby(/prefer-secondary)
> target_server_type: any/primary/secondary/prefer-secondary
>

+1 to your changes for the option values of these 2 variables.

Thanks for addressing my previous review comments in the v18 patch.

I have re-reviewed v18. Below are some additional (mostly minor)
things I noticed.



COMMENT (help text)

The help text is probably accurate but it does seem a bit confusing still.

Example1:

+   
+If this parameter is set to read-write,
only a connection in which
+read-write transactions are accepted by default is considered
acceptable. To determine
+whether the server supports read-write transactions, then if
the server is version 14
+or greater, the support of read-write transactions is
determined by the value of the
+transaction_read_only configuration
parameter that is reported by
+the server upon successful connection. Otherwise if the
server is prior to version 14,
+the query SHOW transaction_read_only will
be sent upon any successful
+connection; if it returns on, it means the
server doesn't support
+read-write transactions.
+   

That fragment "To determine whether the server supports read-write
transactions, then" seems redundant.

Example2:

The Parameter Value descriptions seem inconsistently worded. e.g.
* "read-write" gives details about how "SHOW transaction_read_only"
can be called to decide r/w server.
* but then "read-only" doesn't mention about it
* but then "primary" does
* but then "standby" doesn't

IMO if there was some up-front paragraphs to say how different
versions calculate the r/w support and recovery mode, then all the
different parameter values can be expressed in a much simpler way and
have less repetition (e.g they can all look like the "read-only" one
does now).

e.g. I mean something similar to this (which is same wording as yours,
just rearranged a bit):
--
SERVER STATES

If the server is version 14 or greater, the support of read-write
transactions is determined by the value of the transaction_read_only
configuration parameter that is reported by the server upon successful
connection. Otherwise if the server is prior to version 14, the query
SHOW transaction_read_only will be sent upon any successful
connection; if it returns on, it means the server doesn't support
read-write transaction

The recovery mode state is determined by the value of the in_recovery
configuration parameter that is reported by the server upon successful
connection

PARAMETER VALUES

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-only transactions are accepted by default is considered
acceptable.

If this parameter is set to primary, then if the server is version 14
or greater, only a connection in which the server is not in recovery
mode is considered acceptable. Otherwise, if the server is prior to
version 14, only a connection in which read-write transactions are
accepted by default is considered acceptable.

If this parameter is set to standby, then if the server is version 14
or greater, only a connection in which the server is in recovery mode
is considered acceptable. Otherwise, if the server is prior to version
14, only a connection for which the server only supports read-only
transactions is considered acceptable.

If this parameter is set to prefer-standby, then if the server is
version 14 or greater, a connection in which the server is in recovery
mode is preferred. Otherwise, if the server is prior to version 14, a
connection for which the server only supports read-only transactions
is preferred. If no such connections can be found, then a connection
in which the server is not in recovery mode (server is version 14 or
greater) or a connection for which the server supports read-write
transactions (server is prior to version 14) will be considered
--




COMMENT fe-connect.c (sizeof)

- "Target-Session-Attrs", "", 11, /* sizeof("read-write") = 11 */
+ "Target-Session-Attrs", "", 15, /* sizeof("prefer-standby") = 15 */

You said changing 

Re: Libpq support to connect to standby server as priority

[Greg Nancarrow]

Hi Peter,

I have updated the patch (attached) based on your comments, with
adjustments made for additional changes based on feedback (which I
tend to agree with) from Robert Haas and Tsunakawa san, who suggested
read-write/read-only should be functionally different to
primary/standby, and not just have "read-write" a synonym for
"primary".
I also thought it appropriate to remove "read-write", "standby" and
"prefer-standby" from accepted values for "target_server_type"
(instead just support "secondary" and "prefer-secondary") to match the
similar targetServerType PGJDBC option.
So currently have as supported option values:

target_session_attrs:
any/read-write/read-only/primary/standby(/secondary)/prefer-standby(/prefer-secondary)
target_server_type: any/primary/secondary/prefer-secondary

See my responses to your review comments below:

>GENERAL COMMENT 1 ("any")
>
>"any" should be included as valid option for target_server_type.
>
>IIUC target_server_type was added mostly to have better alignment with JDBC 
>options.
>Both Vladimir [1] and Dave [2] already said that JDBC does have an "any" 
>option.
>[1] - 
>https://www.postgresql.org/message-id/CAB%3DJe-FwOVE%3D8gR1UDDZRnWZR65fRG40e8zW_U_6mnUqbce68g%40mail.gmail.com
>[2] - 
>https://www.postgresql.org/message-id/CADK3HHJ9316ji7L-97cJBY%3Dwp4E3ddPMn8XdkNz6j8d9u0OhmQ%40mail.gmail.com
>
>Furthermore, the fe-connect.c function makeEmptyPGConn sets default:
>+   conn->requested_server_type = SERVER_TYPE_ANY;
>This means the default type of target_server_type is "any".
>Since this is default, it should also be possible to assign the same value to 
>explicitly.
>
>(Parts of the v17 patch affected by this are itemised below)


GN RESPONSE: After checking the PGJDBC source and previous comments, I agree.
Have updated the patch to allow "any" for target_server_type.



>GENERAL COMMENT 2 (Removal of pg_is_in_recovery)
>
>Around 22/3/2019 Hari added a lot of pg_is_in_recovery code in his patch 0006 
>[1]
>[1] - 
>https://www.postgresql.org/message-id/CAJrrPGd4YeA%2BN%3DxC%2B1XPVoGzMCATJZY4irVQEJ6i0aPqorUi7g%40mail.gmail.com
>
>Much later IIUC the latest v17 patch has taken onboard the recommendation from 
>Alvaro and removed all that code:
>"I would discard the whole thing about checking "SELECT pg_is_in_recovery()"" 
>[2]
>[2] - 
>https://www.postgresql.org/message-id/20191227130828.GA21647%40alvherre.pgsql
>
>However, it seems that not ALL parts of the original code got cleanly removed 
>in v17.
>There are a number of references to CONNECTION_CHECK_RECOVERY and 
>pg_is_in_recovery still lurking.
>
>(Parts of the v17 patch affected by this are itemised below)
>

GN RESPONSE: Agree. The calling code was removed but somehow the
CONNECTION_CHECK_RECOVERY case block (and enum) was not removed. Also,
part of the documentation was not updated, for the case where the
server version is prior to 14.
I have updated the patch to correct this.



>COMMENT libpq.sgml (para blocks)
>
>+   
>
>The v17 patch for target_session_attrs and target_server_type help is 
>currently using  blocks for each of the possible >option values.
>This format is inconsistent document style with other variables in this SGML.
>Other places are using sub-lists for option values. e.g. look at "six modes" 
>of sslmode.

GN RESPONSE: True, but this was the case BEFORE the patch, and these
options are more complex than ones where sub-lists for option values
are used - there needs to be common explanation of what the option
synonyms are, and how the behaviour is version dependent, so it
doesn't really lend itself to simple list items, that would need to
cross-reference other list items.



>COMMENT libpq.sgml (cut/paste parameter description)
>
>I don't think that target_server_type help should be just a cut/paste 
>duplicate of  target_session_attrs. It is confusing >because it leaves the 
>reader doubting the purpose of having such a duplication.
>
>Suggest to simplify the target_server_type help like as follows:
>--
>target_server_type
>The purpose of this parameter is to reflect the similar PGJDBC 
>targetServerType.
>The supported options are same as target_session_attrs.
>This parameter overrides any connection type specified by target_session_attrs.
>--

GN RESPONSE: Agree. Will update documentation, though with some
modifications to the wording because of changes in supported option
values already mentioned, and target_session_attrs could contain
non-server-type options in the future.




>COMMENT libpq.sgml (pg_is_in_recovery)
>
>(As noted in GENERAL COMMENT 2 there are still residual references to 
>pg_is_in_recovery)
>
>+   
>+If this parameter is set to standby, only a 
>connection in which
>+the server is in recovery mode is considered acceptable. If the 
>server is prior to version 14,
>+the query SELECT pg_is_in_recovery() will be sent 
>upon any successful
>+connection; if it returns t, it means the server 
>is in recovery 

RE: Libpq support to connect to standby server as priority

[tsunakawa.ta...@fujitsu.com]

From: Robert Haas 
> I think it would be better to have read-write and read-only check
> trnasaction_read_only, and primary and standby can check the new
> thing. There can never be any real advantage in having synonyms for
> the same thing, but there can be an advantage to letting users choose
> the behavior they want.

+1
"primary" is not always equal to "read-write".  When normal users are only 
allowed to query data on a logically replicated database (ALTER USER SET 
default_transaction_read_only = on), it's the primary read-only server.


Regards
Takayuki Tsunakawa

Re: Libpq support to connect to standby server as priority

[Robert Haas]

On Fri, Dec 27, 2019 at 8:08 AM Alvaro Herrera  wrote:
> So, we can know whether server is primary/standby by checking
> in_recovery, as opposed to knowing whether read-write which is done by
> checking transaction_read_only.  So we can keep read-write as a synonym
> for "primary", and check in_recovery when used in servers that support
> the new GUC, and check transaction_read_only in older servers.

I think it would be better to have read-write and read-only check
trnasaction_read_only, and primary and standby can check the new
thing. There can never be any real advantage in having synonyms for
the same thing, but there can be an advantage to letting users choose
the behavior they want.

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

RE: Libpq support to connect to standby server as priority

[Smith, Peter]

Hi Greg,

I was able to successfully execute all the tests of the v17-0001 patch.

But I do have a couple of additional review comments about the test code.



COMMENT - missing "any" tests

In my earlier code review (previous email) I suggested that "any" should be 
added as valid option to the target_server_type parameter.

But this now means there are some missing test cases for

target_server_type = "any"



COMMENT - negative tests?

IIUC when "standby" (aka "secondary") is specified, and there is no in_recovery 
server available, then the result should be an error like "could not make a 
readonly connection to server "

But I did not find any such error combination tests.

e.g. Where are these test cases?

target_session_attrs = "standby", when no standby is available
target_session_attrs = "secondary", when no standby is available
target_server_type = "standby", when no standby is available
target_server_type = "secondary", when no standby is available

--

And, similarly for "could not make a writable connection to server ".

e.g. Where are these test cases?

target_session_attrs = "primary", when no primary is available
target_session_attrs = "read-write", when no primary is available
target_server_type = "primary", when no primary is available
target_server_type = "read-write", when no primary is available


Kind Regards,
Peter Smith
---
Fujitsu Australia

RE: Libpq support to connect to standby server as priority

[Smith, Peter]

Hi Greg,

I have spent some time reading this discussion thread, and doing a code review 
of the latest (v17-0001) patch.

Below are my review comments; some are trivial, others not so much.



GENERAL COMMENT 1 ("any")

"any" should be included as valid option for target_server_type.

IIUC target_server_type was added mostly to have better alignment with JDBC 
options.
Both Vladimir [1] and Dave [2] already said that JDBC does have an "any" option.
[1] - 
https://www.postgresql.org/message-id/CAB%3DJe-FwOVE%3D8gR1UDDZRnWZR65fRG40e8zW_U_6mnUqbce68g%40mail.gmail.com
[2] - 
https://www.postgresql.org/message-id/CADK3HHJ9316ji7L-97cJBY%3Dwp4E3ddPMn8XdkNz6j8d9u0OhmQ%40mail.gmail.com

Furthermore, the fe-connect.c function makeEmptyPGConn sets default: 
+   conn->requested_server_type = SERVER_TYPE_ANY;
This means the default type of target_server_type is "any".
Since this is default, it should also be possible to assign the same value to 
explicitly.

(Parts of the v17 patch affected by this are itemised below)



GENERAL COMMENT 2 (Removal of pg_is_in_recovery)

Around 22/3/2019 Hari added a lot of pg_is_in_recovery code in his patch 0006 
[1]
[1] - 
https://www.postgresql.org/message-id/CAJrrPGd4YeA%2BN%3DxC%2B1XPVoGzMCATJZY4irVQEJ6i0aPqorUi7g%40mail.gmail.com

Much later IIUC the latest v17 patch has taken onboard the recommendation from 
Alvaro and removed all that code:
"I would discard the whole thing about checking "SELECT pg_is_in_recovery()"" 
[2]
[2] - 
https://www.postgresql.org/message-id/20191227130828.GA21647%40alvherre.pgsql

However, it seems that not ALL parts of the original code got cleanly removed 
in v17.
There are a number of references to CONNECTION_CHECK_RECOVERY and 
pg_is_in_recovery still lurking.

(Parts of the v17 patch affected by this are itemised below)



COMMENT libpq.sgml (para blocks)

+   

The v17 patch for target_session_attrs and target_server_type help is currently 
using  blocks for each of the possible option values. 
This format is inconsistent document style with other variables in this SGML. 
Other places are using sub-lists for option values. e.g. look at "six modes" of 
sslmode.



COMMENT libpq.sgml (cut/paste parameter description)

I don't think that target_server_type help should be just a cut/paste duplicate 
of  target_session_attrs. It is confusing because it leaves the reader doubting 
the purpose of having such a duplication.

Suggest to simplify the target_server_type help like as follows:
--
target_server_type
The purpose of this parameter is to reflect the similar PGJDBC targetServerType.
The supported options are same as target_session_attrs.
This parameter overrides any connection type specified by target_session_attrs.
--



COMMENT libpq.sgml (pg_is_in_recovery)

(As noted in GENERAL COMMENT 2 there are still residual references to 
pg_is_in_recovery)

+   
+If this parameter is set to standby, only a 
connection in which
+the server is in recovery mode is considered acceptable. If the server 
is prior to version 14,
+the query SELECT pg_is_in_recovery() will be sent 
upon any successful
+connection; if it returns t, it means the server is 
in recovery mode.
+   

Suggest change to:
--
If this parameter is set to standby, only a connection in 
which the server is in recovery mode is considered acceptable. The recovery 
mode state is determined by the value of the in_recovery configuration 
parameter that is reported by the server upon successful connection. Otherwise, 
if the server is prior to version 14, only a connection in which read-write 
transactions are not accepted by default is considered acceptable. To determine 
whether the server supports read-write transactions, the query SHOW 
transaction_read_only will be sent upon any successful connection; if it 
returns on, it means the server doesn't support read-write transactions.
--



COMMENT libpq.sgml (Oxford comma)

+   integer_datetimes,
+   standard_conforming_strings and
+   in_recovery.

Previously there was an Oxford comma (e.g. before the "and"). Now there isn't.
The v17 patch should not alter the previous listing style.



COMMENT protocol.sgml (Oxford comma)

+integer_datetimes,
+standard_conforming_strings and
+in_recovery.

Previously there was an Oxford comma (e.g. before the "and"). Now there isn't.
The v17 patch should not alter the previous listing style.



QUESTION standby.c - SendRecoveryExitSignal

+/*
+ * SendRecoveryExitSignal
+ * Signal backends that the server has exited recovery mode.
+ */
+void
+SendRecoveryExitSignal(void)
+{
+   SendSignalToAllBackends(PROCSIG_RECOVERY_EXIT);
+}

I wonder if this function is really necessary?
IIUC the SendRecoveryExitSignal is only called from one place (xlog.c).
Why not just call SendSignalToAllBackends directly from there and remove this 
extra layer?



COMMENT postgres.c (signal comment)

+   /* 

Re: Libpq support to connect to standby server as priority

[Greg Nancarrow]

>
> Thanks, but now the tests no longer work as the nodes in the test suite are
> renamed.  While simple enough for a committer to fix, it's always good to see
> the tests pass in the CFBot to make sure the variable name error isn't hiding
> an actual test error.
>

Rebased patch attached, all tests currently working as of Jul 19
(a766d6ca22ac7c233e69c896ae0c5f19de916db4).


v17-0001-Enhance-libpq-target_session_attrs-and-add-target_se.patch
Description: Binary data

Re: Libpq support to connect to standby server as priority

[Daniel Gustafsson]

> On 6 Jul 2020, at 14:19, Greg Nancarrow  wrote:
> 
>> This patch no longer applies, can you please submit a rebased version?  I've
>> marked the entry as Waiting on Author in the meantime.
>> 
> 
> Here's a rebased version of the patch.

Thanks, but now the tests no longer work as the nodes in the test suite are
renamed.  While simple enough for a committer to fix, it's always good to see
the tests pass in the CFBot to make sure the variable name error isn't hiding
an actual test error.

cheers ./daniel

Re: Libpq support to connect to standby server as priority

[Greg Nancarrow]

> This patch no longer applies, can you please submit a rebased version?  I've
> marked the entry as Waiting on Author in the meantime.
>

Here's a rebased version of the patch.

Regards,
Greg


v16-0001-Enhance-libpq-target_session_attrs-and-add-target_se.patch
Description: Binary data

Re: Libpq support to connect to standby server as priority

[Daniel Gustafsson]

> On 18 May 2020, at 09:33, Greg Nancarrow  wrote:

> I'd like to submit a new version of a patch that I'd previously
> submitted but was eventually Returned with Feedback (closed in
> commitfest 2020-03).

This patch no longer applies, can you please submit a rebased version?  I've
marked the entry as Waiting on Author in the meantime.

cheers ./daniel

Re: Libpq support to connect to standby server as priority

[Greg Nancarrow]

Hi Hackers,

I'd like to submit a new version of a patch that I'd previously
submitted but was eventually Returned with Feedback (closed in
commitfest 2020-03).
The patch enhances the libpq "target_session_attrs" connection
parameter by supporting primary/standby/prefer-standby, and I've
attempted some sort of alignment with similar PGJDBC driver
functionality by adding a "target_server_type" parameter. Now targets
PG14.
I've merged the original set of 3 patches into one patch and tried to
account for most(?) of the requested changes in the feedback comments;
if nothing else, it should be easier to read and understand.
Previous discussion here:
https://www.postgresql.org/message-id/flat/caf3+xm+8-ztokav9ghij3wfgentq97qcjxqt+rbfq6f7onz...@mail.gmail.com

Regards,
Greg Nancarrow
Fujitsu Australia


v15-0001-Enhance-libpq-target_session_attrs-and-add-target_se.patch
Description: Binary data

Re: Libpq support to connect to standby server as priority

[David Steele]

On 2/28/20 11:05 AM, Alvaro Herrera wrote:

MauMau, Greg, is any of you submitting a new patch for this?


This patch has not had any updates in months and now we are halfway 
through the CF so I have marked it Returned with Feedback.


If a patch arrives soon I'll be happy to revive the entry, otherwise 
please submit to a future CF when a new patch is available.


Regards,
--
-David
da...@pgmasters.net

Re: Libpq support to connect to standby server as priority

[Alvaro Herrera]

MauMau, Greg, is any of you submitting a new patch for this?

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

Re: Libpq support to connect to standby server as priority

[Alvaro Herrera]

On 2020-Jan-06, tsunakawa.ta...@fujitsu.com wrote:

> Let me check my understanding.  Are you proposing these?
> 
> * The canonical libpq connection parameter is target_session_attr = {primary 
> | standby | prefer-standby}.  Leave and document read-write as a synonym for 
> primary.
> 
> * When the server version is 13 or later, libpq just checks in_recovery, not 
> checking transaction_read_only or sending SHOW transaction_read_only.
> 
> * When the server version is before 13, libpq sends SHOW 
> transaction_read_only as before.

Yes, that sounds good to me.

> Personally, 100% agreed, considering what we really wanted to do when 
> target_session_attr was introduced is to tell if the server is primary or 
> standby.  The questions are:
> 
> Q1: Should we continue to use the name target_session_attr, or rename it to 
> target_server_type and make target_session_attr a synonym for it?  I'm in 
> favor of the latter.

I'm not 100% sure about this.  I think part of the reason of making it
target_session_attrs (note plural) is that the user could be able to
specify more than one attribute (a comma-separated list, like the
DateStyle GUC), if we supported some hypothetical attributes in the
future that are independent of the existing ones.  I'm not inclined to
break that, unless the authors of the original feature agree to that.

Maybe one possible improvement would be to add target_server_type as an
additional one, that only accepts a single item 
(primary/standby/prefer-standby),
as a convenience, while target_session_attrs retains its ability to
receive more than one value.  The two would be somewhat redundant but
not exact synonyms.

> Q2: Can we accept the subtle incompatibility that
> target_session_attr=read-write and target_server_type=primary are not
> the same, when default_transaction_read_only is on?  (I'd like to hear
> yes)

... on servers versions 12 and older, yes.  (If I understand correctly,
we wouldn't have such a difference in version 13).

> Q3: Can we go without supporting standby and prefer-standby for older
> servers?  (I think yes because we can say that it's a new feature
> effective for new servers.)

Yes.

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

RE: Libpq support to connect to standby server as priority

[tsunakawa.ta...@fujitsu.com]

From: Alvaro Herrera 
> So, we can know whether server is primary/standby by checking
> in_recovery, as opposed to knowing whether read-write which is done by
> checking transaction_read_only.  So we can keep read-write as a synonym
> for "primary", and check in_recovery when used in servers that support
> the new GUC, and check transaction_read_only in older servers.
> 
> It seems there's a lot of code that we can discard from the patch:
> first, we can discard checking for "read-only" altogether.  Second, have
> us check transaction_read_only *only* if the server is of an older
> version.

Let me check my understanding.  Are you proposing these?

* The canonical libpq connection parameter is target_session_attr = {primary | 
standby | prefer-standby}.  Leave and document read-write as a synonym for 
primary.

* When the server version is 13 or later, libpq just checks in_recovery, not 
checking transaction_read_only or sending SHOW transaction_read_only.

* When the server version is before 13, libpq sends SHOW transaction_read_only 
as before.


Personally, 100% agreed, considering what we really wanted to do when 
target_session_attr was introduced is to tell if the server is primary or 
standby.  The questions are:

Q1: Should we continue to use the name target_session_attr, or rename it to 
target_server_type and make target_session_attr a synonym for it?  I'm in favor 
of the latter.

Q2: Can we accept the subtle incompatibility that 
target_session_attr=read-write and target_server_type=primary are not the same, 
when default_transaction_read_only is on?  (I'd like to hear yes)

Q3: Can we go without supporting standby and prefer-standby for older servers?  
(I think yes because we can say that it's a new feature effective for new 
servers.)


Regards
Takayuki Tsunakawa

Re: Libpq support to connect to standby server as priority

[Alvaro Herrera]

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

> From: Alvaro Herrera 
> > I'm not sure I understand why we end up with "prefer-read" in addition
> > to "prefer-standby" (and similar seeming redundancy between "primary"
> > and "read-write").  Do we really need more than one way to identify
> > hosts' roles?  It seems 0001 adds the "prefer-read" modes by checking
> > transaction_read_only, and later 0002 adds the "prefer-standby" modes by
> > checking in_recovery.  I'm not sure that we're serving our users very
> > well by giving them choice that ends up being confusing.  In other words
> > I think we should do only one of these things, not both.  Maybe merge
> > 0001 and 0002 in a single patch, and get rid of redundant modes.
> 
> That's because the distinction read/write is different from
> primary/standby.  If default_transaction_read_only is on, even the
> primary is read-only.  That's why the syntax target_session_attrs =
> {read-write | read-only} was introduced instead of target_server_type
> = {primary | standby}.  Personally, I only want target_server_type =
> {primary | standby | prefer-standby}, and discard target_session_attrs
> for simplicity of the functional specification and the code.

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

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

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

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

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

RE: Libpq support to connect to standby server as priority

[tsunakawa.ta...@fujitsu.com]

From: Alvaro Herrera 
> I'm not sure I understand why we end up with "prefer-read" in addition
> to "prefer-standby" (and similar seeming redundancy between "primary"
> and "read-write").  Do we really need more than one way to identify
> hosts' roles?  It seems 0001 adds the "prefer-read" modes by checking
> transaction_read_only, and later 0002 adds the "prefer-standby" modes by
> checking in_recovery.  I'm not sure that we're serving our users very
> well by giving them choice that ends up being confusing.  In other words
> I think we should do only one of these things, not both.  Maybe merge
> 0001 and 0002 in a single patch, and get rid of redundant modes.

That's because the distinction read/write is different from primary/standby.  
If default_transaction_read_only is on, even the primary is read-only.  That's 
why the syntax target_session_attrs = {read-write | read-only} was introduced 
instead of target_server_type = {primary | standby}.  Personally, I only want 
target_server_type = {primary | standby | prefer-standby}, and discard 
target_session_attrs for simplicity of the functional specification and the 
code.


> Also, Ishii-san said:
> https://postgr.es/m/20190116.150236.2304777214520289427.t-ishii@sraoss.c
> o.jp
>   - When looking for a primary, find a node where pg_is_in_recovery is
> false; if none, libpq should retry until a timeout expires.  Did we
> reject this idea altogether, or is it just unimplemented?

I don't remember well, but I guess this is for eliminating the need for 
applications to retry connection attempts during the database server failover.  
I think that will be convenient, but not mandatory for this patch.  PgJDBC 
doesn't provide it, either.


Regards
Takayuki Tsunakawa

Re: Libpq support to connect to standby server as priority

[Alvaro Herrera]

On 2019-Dec-26, Dave Cramer wrote:

> On Thu, 26 Dec 2019 at 15:07, Alvaro Herrera 
> wrote:

> > There were other comments that I think went largely unaddressed,
> > such as the point that the JDBC driver seems to offer a different
> > syntax for the configuration, and should we offer a compatibility
> > shim of some sort.  (Frankly, I don't think we need to stress over
> > this too much, but it seems that it wasn't even discussed.)
> 
> We seem to ignore prior work here I agree. It would be wonderful if
> there were only one syntax. Is it too late to change the syntax for
> this patch as that ship has sailed for JDBC

So, starting with pg10 we have target_session_attrs in libpq.  These
patches just add some more "attrs" that can be requested for a session.
Tom's proposal[1] was to rename the conninfo option to match JDBC's
targetServerType, adding a compatibility mechanism so that libpq's
target_session_attrs continues to work for values "any" and
"read-write"; but we already discussed all this with regards to the
pgjdbc param names and we still decided not to use them[2] (ending as
commit 721f7bd3cbcc).

Maybe y'all want to relitigate this for some reason.  I can help with
getting an implementation finished once y'all are done with the
politics.

[1] https://postgr.es/m/26251.1547504...@sss.pgh.pa.us
[2] 
https://www.postgresql.org/message-id/CAHg_5grVKbO73CqKNYsCYsX5aJ%3DdeDSAyW44wjmwt1uqngScdQ%40mail.gmail.com

(If we do want to match pgJDBC's option name, then I suppose we need to
add a synonym mechanism to libpq's option parsing.  That doesn't look
particularly difficult, and it would probably help clean up the mess
that we currently track both the "char *" value of the option as well as
a separate enum value for it, in the pgconn struct.)

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

Re: Libpq support to connect to standby server as priority

[Dave Cramer]

On Thu, 26 Dec 2019 at 15:07, Alvaro Herrera 
wrote:

> On 2019-Oct-01, Greg Nancarrow wrote:
>
> > On Wed, Sep 11, 2019 at 10:17 AM Alvaro Herrera from 2ndQuadrant
> >  wrote:
> > >
> > > Oh, oops. Here they are then.
> >
> > With the permission of the original patch author, Haribabu Kommi, I’ve
> > rationalized the existing 8 patches into 3 patches, merging patches
> > 1-5 and 6-7, and tidying up some documentation and code comments. I
> > also rebased them to the latest PG12 source code (as of October 1,
> > 2019). The patch code itself is the same, except for some version
> > checks that I have updated to target the features for PG13 instead of
> > PG12.
>
> I've spent some time the last few days going over these patches and the
> prior discussion.
>
> I'm not sure I understand why we end up with "prefer-read" in addition
> to "prefer-standby" (and similar seeming redundancy between "primary"
> and "read-write").  Do we really need more than one way to identify
> hosts' roles?  It seems 0001 adds the "prefer-read" modes by checking
> transaction_read_only, and later 0002 adds the "prefer-standby" modes by
> checking in_recovery.  I'm not sure that we're serving our users very
> well by giving them choice that ends up being confusing.  In other words
> I think we should do only one of these things, not both.  Maybe merge
> 0001 and 0002 in a single patch, and get rid of redundant modes.
>
> There were other comments that I think went largely unaddressed, such as
> the point that the JDBC driver seems to offer a different syntax for the
> configuration, and should we offer a compatibility shim of some sort.
> (Frankly, I don't think we need to stress over this too much, but it
> seems that it wasn't even discussed.)
>

We seem to ignore prior work here I agree. It would be wonderful if there
were only one
syntax. Is it too late to change the syntax for this patch as that ship has
sailed for JDBC

>
>

Re: Libpq support to connect to standby server as priority

[Alvaro Herrera]

On 2019-Dec-26, Alvaro Herrera wrote:

> The name of the label "consume_checked_write_connection" is not very
> descriptive.  I propose "conn_succeeded" instead.

(I realized later that I should have removed this paragraph -- other
goto labels are added in 0002 that would make such renaming more
confusing than helpful.  My later comment about the if/else/then maze is
more general.)

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

Re: Libpq support to connect to standby server as priority

[Alvaro Herrera]

On 2019-Oct-01, Greg Nancarrow wrote:

> On Wed, Sep 11, 2019 at 10:17 AM Alvaro Herrera from 2ndQuadrant
>  wrote:
> >
> > Oh, oops. Here they are then.
> 
> With the permission of the original patch author, Haribabu Kommi, I’ve
> rationalized the existing 8 patches into 3 patches, merging patches
> 1-5 and 6-7, and tidying up some documentation and code comments. I
> also rebased them to the latest PG12 source code (as of October 1,
> 2019). The patch code itself is the same, except for some version
> checks that I have updated to target the features for PG13 instead of
> PG12.

I've spent some time the last few days going over these patches and the
prior discussion.

I'm not sure I understand why we end up with "prefer-read" in addition
to "prefer-standby" (and similar seeming redundancy between "primary"
and "read-write").  Do we really need more than one way to identify
hosts' roles?  It seems 0001 adds the "prefer-read" modes by checking
transaction_read_only, and later 0002 adds the "prefer-standby" modes by
checking in_recovery.  I'm not sure that we're serving our users very
well by giving them choice that ends up being confusing.  In other words
I think we should do only one of these things, not both.  Maybe merge
0001 and 0002 in a single patch, and get rid of redundant modes.

There were other comments that I think went largely unaddressed, such as
the point that the JDBC driver seems to offer a different syntax for the
configuration, and should we offer a compatibility shim of some sort.
(Frankly, I don't think we need to stress over this too much, but it
seems that it wasn't even discussed.)

0003 contains parts written by Elvis Pranskevichus.  It would be good to
confirm that he is satisfied with how the whole thing ends up working.

Also, Ishii-san said:
https://postgr.es/m/20190116.150236.2304777214520289427.t-is...@sraoss.co.jp
  - When looking for a primary, find a node where pg_is_in_recovery is
false; if none, libpq should retry until a timeout expires.  Did we
reject this idea altogether, or is it just unimplemented?


Looking at 0001, I would move the new "desired connection mode" to
libpq-int.h (from libpq-fe.h), and rename like this

/* Desired connection type */
typedef enum
{
TGT_CONN_TYPE_ANY = 0,  /* Any session (default) */
TGT_CONN_TYPE_READ_WRITE,   /* Read-write session */
TGT_CONN_TYPE_PREFER_READ,  /* Prefer read only session */
TGT_CONN_TYPE_READ_ONLY /* Read only session */
} TargetConnectionType;

The name of the label "consume_checked_write_connection" is not very
descriptive.  I propose "conn_succeeded" instead.

"read_write_host_index" seems a very unimaginative struct member name.
Following "whichhost" I propose to rename this to "which_rw_host", and
rewrite its comment to something like this:

/*
 * Status indicator for read-write host.  The initial value of -1
 * indicates that we don't know which server is the read-write one; a
 * non-negative number (set as soon as we discover one) indicates which
 * server is the read-write one; -2 indicates that the server being tested
 * (whichhost???) is the read-write one.
 */
int which_rw_host;

(I'm not sure that the explanation for value -2 is correct. Please
rewrite that if it isn't.)


I think the if/then/else maze in the CONNECTION_CHECK_TARGET case in
PQconnectPoll() is a nigh unreadable rat's nest after these patches.
Maybe some extra states in the state machine are needed; and probably
that would be helped by some small subroutines to reduce the
duplication.  PQconnectPoll is already 1700 lines long; our job is not
made easier by making it 2000 lines long.

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

RE: Libpq support to connect to standby server as priority

[tsunakawa.ta...@fujitsu.com]

From: Greg Nancarrow 
> With the permission of the original patch author, Haribabu Kommi, I’ve
> rationalized the existing 8 patches into 3 patches, merging patches
> 1-5 and 6-7, and tidying up some documentation and code comments. I
> also rebased them to the latest PG12 source code (as of October 1,
> 2019). The patch code itself is the same, except for some version
> checks that I have updated to target the features for PG13 instead of
> PG12.
> I’ve attached the updated patches.

Thank you for taking over this patch.  Your arrangement has made the patches 
much easier to read!

I've finished reviewing, and my comments are below.  Unfortunately, 0003 failed 
to apply (I guess only slight modification is needed to apply to HEAD.)  I'd 
like to proceed to testing when the revised patch becomes available.



(1) 0001
+   /*
+* Requested type is prefer-read, then record 
this host index
+* and try the other before considering it 
later. If requested
+* type of connection is read-only, ignore this 
connection.
+*/
+   if (conn->requested_session_type == 
SESSION_TYPE_PREFER_READ ||
+   conn->requested_session_type == 
SESSION_TYPE_READ_ONLY)
{

This if statement seems unnecessary, because the following part at the 
beginning of the CONNECTION_CHECK_TARGET case block precludes entering the if 
block.  Cases other than "any" are handled first here.


if (conn->sversion >= 70400 &&
-   conn->target_session_attrs != NULL &&
-   strcmp(conn->target_session_attrs, 
"read-write") == 0)
+   conn->requested_session_type != 
SESSION_TYPE_ANY)
+   {


(2) 0002
-} TargetSessionAttrsType;
+}  TargetSessionAttrsType;

One space after } is replaced with three tabs.  I guess this is an 
unintentional change.
 

(3) 0002
+reject_checked_read_or_write_connection(PGconn *conn)

To follow the naming style of most internal functions in this file, I find it 
better to change the name to rejectCheckedReadOrWriteConnection.


(4) 0003
+reject_checked_recovery_connection(PGconn *conn)

The same as the previous one.


(5) 0003
Don't we have to describe in_recovery in both or either of 
high-availability.sgml and config.sgml?  transaction_read_only is touched in 
the former.


Regards
Takayuki Tsunakawa

Re: Libpq support to connect to standby server as priority

[Greg Nancarrow]

On Wed, Sep 11, 2019 at 10:17 AM Alvaro Herrera from 2ndQuadrant
 wrote:
>
>
> Oh, oops. Here they are then.
>

With the permission of the original patch author, Haribabu Kommi, I’ve
rationalized the existing 8 patches into 3 patches, merging patches
1-5 and 6-7, and tidying up some documentation and code comments. I
also rebased them to the latest PG12 source code (as of October 1,
2019). The patch code itself is the same, except for some version
checks that I have updated to target the features for PG13 instead of
PG12.
I’ve attached the updated patches.

Regards,
Greg Nancarrow
Fujitsu Australia


v14-0001-libpq-target_session_attrs-read_write-prefer_read-read_only.patch
Description: Binary data


v14-0002-libpq-target_session_attrs-primary-prefer_standby-standby.patch
Description: Binary data


v14-0003-Server-recovery-mode-handling.patch
Description: Binary data

Re: Libpq support to connect to standby server as priority

[Greg Nancarrow]

On Wed, Sep 11, 2019 at 10:17 AM Alvaro Herrera from 2ndQuadrant
 wrote:
>
> Oh, oops. Here they are then.
>

With the permission of the original patch author, Haribabu Kommi, I’ve
rationalized the existing 8 patches into 3 patches, merging patches
1-5 and 6-7, and tidying up some documentation and code comments. I
also rebased them to the latest PG12 source code (as of October 1,
2019). The patch code itself is the same, except for some version
checks that I have updated to target the features for PG13 instead of
PG12.
I’ve attached the updated patches.

Regards,
Greg Nancarrow
Fujitsu Australia


v14-0001-libpq-target_session_attrs-read_write-prefer_read-read_only.patch
Description: Binary data


v14-0003-Server-recovery-mode-handling.patch
Description: Binary data


v14-0002-libpq-target_session_attrs-primary-prefer_standby-standby.patch
Description: Binary data

Re: Libpq support to connect to standby server as priority

[Alvaro Herrera from 2ndQuadrant]

On 2019-Sep-11, Tsunakawa, Takayuki wrote:

> From: Alvaro Herrera from 2ndQuadrant [mailto:alvhe...@alvh.no-ip.org]

> > Remaining patchset attached (per my count it's v13 of your patchset.
> 
> I'm afraid those weren't attached.

Oh, oops. Here they are then.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 247e888266882ab673efd04ecf017846400859ad Mon Sep 17 00:00:00 2001
From: Hari Babu 
Date: Wed, 27 Feb 2019 11:50:33 +1100
Subject: [PATCH v13 1/8] New TargetSessionAttrsType enum

This new enum is useful to compare the requested session type
instead of comparing it with string always. This may not show
much improvement with current code, but it will be useful with
further patches
---
 src/interfaces/libpq/fe-connect.c | 12 
 src/interfaces/libpq/libpq-fe.h   |  6 ++
 src/interfaces/libpq/libpq-int.h  |  1 +
 3 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 8ba0159313..f104fd48aa 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -1295,8 +1295,11 @@ connectOptions2(PGconn *conn)
 	 */
 	if (conn->target_session_attrs)
 	{
-		if (strcmp(conn->target_session_attrs, "any") != 0
-			&& strcmp(conn->target_session_attrs, "read-write") != 0)
+		if (strcmp(conn->target_session_attrs, "any") == 0)
+			conn->requested_session_type = SESSION_TYPE_ANY;
+		else if (strcmp(conn->target_session_attrs, "read-write") == 0)
+			conn->requested_session_type = SESSION_TYPE_READ_WRITE;
+		else
 		{
 			conn->status = CONNECTION_BAD;
 			printfPQExpBuffer(>errorMessage,
@@ -3449,8 +3452,7 @@ keep_going:		/* We will come back to here until there is
  * may just skip the test in that case.
  */
 if (conn->sversion >= 70400 &&
-	conn->target_session_attrs != NULL &&
-	strcmp(conn->target_session_attrs, "read-write") == 0)
+	conn->requested_session_type != SESSION_TYPE_ANY)
 {
 	/*
 	 * Save existing error messages across the PQsendQuery
@@ -3791,6 +3793,8 @@ makeEmptyPGconn(void)
 	conn->try_gss = true;
 #endif
 
+	conn->requested_session_type = SESSION_TYPE_ANY;
+
 	/*
 	 * We try to send at least 8K at a time, which is the usual size of pipe
 	 * buffers on Unix systems.  That way, when we are sending a large amount
diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h
index 5f65db30e4..0612e68c62 100644
--- a/src/interfaces/libpq/libpq-fe.h
+++ b/src/interfaces/libpq/libpq-fe.h
@@ -71,6 +71,12 @@ typedef enum
 	CONNECTION_CHECK_TARGET		/* Check if we have a proper target connection */
 } ConnStatusType;
 
+typedef enum
+{
+	SESSION_TYPE_ANY = 0,		/* Any session (default) */
+	SESSION_TYPE_READ_WRITE		/* Read-write session */
+}			TargetSessionAttrsType;
+
 typedef enum
 {
 	PGRES_POLLING_FAILED = 0,
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index d37bb3ce40..20791b5b73 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -367,6 +367,7 @@ struct pg_conn
 
 	/* Type of connection to make.  Possible values: any, read-write. */
 	char	   *target_session_attrs;
+	TargetSessionAttrsType requested_session_type;
 
 	/* Optional file to write trace info to */
 	FILE	   *Pfdebug;
-- 
2.17.1

>From 9399e3f7f5e85f41871d4e586b0582f697380c0b Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Tue, 10 Sep 2019 12:48:28 -0300
Subject: [PATCH v13 2/8] doc change

---
 doc/src/sgml/libpq.sgml | 26 ++
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 560148..23bf1ea632 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1647,17 +1647,27 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
  
   target_session_attrs
   
+   
+The supported options for this parameter are, any and
+read-write. The default value of this parameter,
+any, regards all connections as acceptable.
+If multiple hosts were specified in the connection string, based on the
+specified value, any remaining servers will be tried before confirming
+succesful connection or failure.
+   
+

 If this parameter is set to read-write, only a
 connection in which read-write transactions are accepted 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
-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.
-  
+is considered acceptable.
+   
+
+   
+   

RE: Libpq support to connect to standby server as priority

[Tsunakawa, Takayuki]

From: Alvaro Herrera from 2ndQuadrant [mailto:alvhe...@alvh.no-ip.org]
> Testing protocol version 2 is difficult!  Almost every single test fails
> because of error messages being reported differently; and streaming
> replication (incl. pg_basebackup) doesn't work at all because it's not
> possible to establish replication connections.  Manual inspection shows
> it behaves correctly.

Yeah, the code path for protocol v2 is sometimes annoying.  I wish v2 support 
will be dropped soon.  I know there was a discussion on it, but I didn't track 
the conclusion.


> Remaining patchset attached (per my count it's v13 of your patchset.

I'm afraid those weren't attached.


> think we should merge one half of it with each of the other two patches
> where the changes are introduced (0003 and 0004).  I'm not convinced
> that we need 0004-0006 to be separate commits.

It was hard to review those separate patches, so I think it's better to merge 
those.  OTOH, I can understand Haribabu-san's idea that the community may not 
accept the latter patches, e.g. accept only 0001-0005.


Regards
Takayuki Tsunakawa

Re: Libpq support to connect to standby server as priority

[Alvaro Herrera from 2ndQuadrant]

On 2019-Sep-09, Alvaro Herrera from 2ndQuadrant wrote:

> Question about 0001.  In the CONNECTION_SETENV state, you end by falling
> through to the CONNECTION_CHECK_TARGET case; but in that switch it seems
> a bit unnatural to do that.  I think doing "goto keep_trying" is another
> way of doing the same thing, but more in line with what every other
> piece of code does.

Appears to work.  Pushed like that.

Testing protocol version 2 is difficult!  Almost every single test fails
because of error messages being reported differently; and streaming
replication (incl. pg_basebackup) doesn't work at all because it's not
possible to establish replication connections.  Manual inspection shows
it behaves correctly.

Remaining patchset attached (per my count it's v13 of your patchset.
Please use "git format-patch -v14" and so on when posting patches).  I
stripped the doc change from 0001 (unchanged) because I found it hard to
justify on its own, and it has a couple of grammatical mistakes.  I
think we should merge one half of it with each of the other two patches
where the changes are introduced (0003 and 0004).  I'm not convinced
that we need 0004-0006 to be separate commits.

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

Re: Libpq support to connect to standby server as priority

[Alvaro Herrera from 2ndQuadrant]

Question about 0001.  In the CONNECTION_SETENV state, you end by falling
through to the CONNECTION_CHECK_TARGET case; but in that switch it seems
a bit unnatural to do that.  I think doing "goto keep_trying" is another
way of doing the same thing, but more in line with what every other
piece of code does.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 30fbb4a913697fe35a195dd41ddd5bcfeec53c0e Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Fri, 6 Sep 2019 16:26:01 -0400
Subject: [PATCH] Restructure the code to remove duplicate code

The duplicate code logic of checking for the server version
before issuing the SHOW transaction_read_only to find out whether
the server is read-write or not is restructured under a new
connection status, so that duplicate code is removed. This is
required for the next set of patches

Author: Hari Babu Kommi
Discussion: https://postgr.es/m/cajrrpge_qgdbbn+ybgevpd+ylhxxjtruzk6rmtmhqrfig+3...@mail.gmail.com
---
 src/interfaces/libpq/fe-connect.c | 87 ---
 src/interfaces/libpq/libpq-fe.h   |  3 +-
 2 files changed, 34 insertions(+), 56 deletions(-)

diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index f03d43376c..91316709a5 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -3434,6 +3434,13 @@ keep_going:		/* We will come back to here until there is
 	return PGRES_POLLING_WRITING;
 }
 
+/* Almost there now ... */
+conn->status = CONNECTION_CHECK_TARGET;
+goto keep_going;
+			}
+
+		case CONNECTION_CHECK_TARGET:
+			{
 /*
  * If a read-write connection is required, see if we have one.
  *
@@ -3476,67 +3483,37 @@ keep_going:		/* We will come back to here until there is
 			}
 
 		case CONNECTION_SETENV:
-
-			/*
-			 * Do post-connection housekeeping (only needed in protocol 2.0).
-			 *
-			 * We pretend that the connection is OK for the duration of these
-			 * queries.
-			 */
-			conn->status = CONNECTION_OK;
-
-			switch (pqSetenvPoll(conn))
 			{
-case PGRES_POLLING_OK:	/* Success */
-	break;
-
-case PGRES_POLLING_READING: /* Still going */
-	conn->status = CONNECTION_SETENV;
-	return PGRES_POLLING_READING;
-
-case PGRES_POLLING_WRITING: /* Still going */
-	conn->status = CONNECTION_SETENV;
-	return PGRES_POLLING_WRITING;
-
-default:
-	goto error_return;
-			}
-
-			/*
-			 * If a read-write connection is required, see if we have one.
-			 * (This should match the stanza in the CONNECTION_AUTH_OK case
-			 * above.)
-			 *
-			 * Servers before 7.4 lack the transaction_read_only GUC, but by
-			 * the same token they don't have any read-only mode, so we may
-			 * just skip the test in that case.
-			 */
-			if (conn->sversion >= 70400 &&
-conn->target_session_attrs != NULL &&
-strcmp(conn->target_session_attrs, "read-write") == 0)
-			{
-if (!saveErrorMessage(conn, ))
-	goto error_return;
-
+/*
+ * Do post-connection housekeeping (only needed in protocol 2.0).
+ *
+ * We pretend that the connection is OK for the duration of these
+ * queries.
+ */
 conn->status = CONNECTION_OK;
-if (!PQsendQuery(conn,
- "SHOW transaction_read_only"))
+
+switch (pqSetenvPoll(conn))
 {
-	restoreErrorMessage(conn, );
-	goto error_return;
+	case PGRES_POLLING_OK:	/* Success */
+		break;
+
+	case PGRES_POLLING_READING: /* Still going */
+		conn->status = CONNECTION_SETENV;
+		return PGRES_POLLING_READING;
+
+	case PGRES_POLLING_WRITING: /* Still going */
+		conn->status = CONNECTION_SETENV;
+		return PGRES_POLLING_WRITING;
+
+	default:
+		goto error_return;
 }
-conn->status = CONNECTION_CHECK_WRITABLE;
-restoreErrorMessage(conn, );
-return PGRES_POLLING_READING;
+
+/* Almost there now ... */
+conn->status = CONNECTION_CHECK_TARGET;
+goto keep_going;
 			}
 
-			/* We can release the address list now. */
-			release_conn_addrinfo(conn);
-
-			/* We are open for business! */
-			conn->status = CONNECTION_OK;
-			return PGRES_POLLING_OK;
-
 		case CONNECTION_CONSUME:
 			{
 conn->status = CONNECTION_OK;
diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h
index 22c4954f2b..5f65db30e4 100644
--- a/src/interfaces/libpq/libpq-fe.h
+++ b/src/interfaces/libpq/libpq-fe.h
@@ -67,7 +67,8 @@ typedef enum
  * connection. */
 	CONNECTION_CONSUME,			/* Wait for any pending message and consume
  * them. */
-	CONNECTION_GSS_STARTUP		/* Negotiating GSSAPI. */
+	CONNECTION_GSS_STARTUP,		/* Negotiating GSSAPI. */
+	CONNECTION_CHECK_TARGET		/* Check if we have a proper target connection */
 } ConnStatusType;
 
 typedef enum
-- 
2.17.1

Re: Libpq support to connect to standby server as priority

[Haribabu Kommi]

On Mon, 3 Jun 2019 at 16:32, Haribabu Kommi 
wrote:

>
>
> On Thu, Apr 11, 2019 at 9:13 AM Haribabu Kommi 
> wrote:
>
>> I fixed all the comments that you have raised above and attached the
>> updated
>> patches.
>>
>
> Rebased patches are attached.
>

Rebased patches are attached.

Regards,
Haribabu Kommi


0001-Restructure-the-code-to-remove-duplicate-code.patch
Description: Binary data


0002-New-TargetSessionAttrsType-enum.patch
Description: Binary data


0003-Make-transaction_read_only-as-GUC_REPORT-variable.patch
Description: Binary data


0004-New-prefer-read-target_session_attrs-type.patch
Description: Binary data


0005-New-read-only-target_session_attrs-type.patch
Description: Binary data


0006-Primary-prefer-standby-and-standby-options.patch
Description: Binary data


0007-New-function-to-rejecting-the-checked-write-connecti.patch
Description: Binary data


0008-Server-recovery-mode-handling.patch
Description: Binary data

Re: Libpq support to connect to standby server as priority

[Haribabu Kommi]

On Thu, Apr 11, 2019 at 9:13 AM Haribabu Kommi 
wrote:

> I fixed all the comments that you have raised above and attached the
> updated
> patches.
>

Rebased patches are attached.

Regards,
Haribabu Kommi
Fujitsu Australia


0001-New-pg_basebackup-g-option-to-control-the-group-acce.patch
Description: Binary data


0002-New-TargetSessionAttrsType-enum.patch
Description: Binary data


0003-Make-transaction_read_only-as-GUC_REPORT-varaible.patch
Description: Binary data


0004-New-prefer-read-target_session_attrs-type.patch
Description: Binary data


0005-New-read-only-target_session_attrs-type.patch
Description: Binary data


0006-Primary-prefer-standby-and-standby-options.patch
Description: Binary data


0007-New-function-to-rejecting-the-checked-write-connecti.patch
Description: Binary data


0008-Server-recovery-mode-handling.patch
Description: Binary data

Re: Libpq support to connect to standby server as priority

[Haribabu Kommi]

On Fri, Mar 29, 2019 at 7:06 PM Tsunakawa, Takayuki <
tsunakawa.ta...@jp.fujitsu.com> wrote:

> Hi Hari-san,
>
> I've reviewed all the files.  The patch would be OK when the following
> have been fixed, except for the complexity of fe-connect.c (which probably
> cannot be improved.)
>
> Unfortunately, I'll be absent next week.  The earliest date I can do the
> test will be April 8 or 9.  I hope someone could take care of this patch...
>

Thanks for the review. Apologies that I could not able finish it on time
because of
other work.



>
> (1) 0001
> With this read-only option type, application can connect to
> to a read-only server in the list of hosts, in case
> ...
> before issuing the SHOW transaction_readonly to find out whether
>
>
> "to" appears twice in a row.
> transaction_readonly -> transaction_read_only
>
>
> (2) 0001
> +succesful connection or failure.
>
> succesful -> successful
>
>
> (3) 0008
> to conenct to a standby server with a faster check instead of
>
> conenct -> connect
>
>
> (4) 0008
> Logically, recovery exit should be notified after the following statement:
>
> XLogCtl->SharedRecoveryInProgress = false;
>
>
> (5) 0008
> +   /* Update in_recovery status. */
> +   if (LocalRecoveryInProgress)
> +   SetConfigOption("in_recovery",
> +   "on",
> +   PGC_INTERNAL,
> PGC_S_OVERRIDE);
> +
>
> This SetConfigOption() is called for every RecoveryInProgress() call on
> the standby.  It may cause undesirable overhead.  How about just calling
> SetConfigOption() once in InitPostgres() to set the initial value for
> in_recovery?  InitPostgres() and its subsidiary functions call
> SetConfigOption() likewise.
>
>
> (6) 0008
> async.c is for LISTEN/UNLISTEN/NOTIFY.  How about adding the new functions
> in postgres.c like RecoveryConflictInterrupt()?
>
>
> (7) 0008
> +   if (pid != 0)
> +   {
> +   (void) SendProcSignal(pid, reason,
> procvxid.backendId);
> +   }
>
> The braces are not necessary because the block only contains a single
> statement.
>

I fixed all the comments that you have raised above and attached the updated
patches.

Regards,
Haribabu Kommi
Fujitsu Australia


0001-Restructure-the-code-to-remove-duplicate-code.patch
Description: Binary data


0002-New-TargetSessionAttrsType-enum.patch
Description: Binary data


0003-Make-transaction_read_only-as-GUC_REPORT-varaible.patch
Description: Binary data


0004-New-prefer-read-target_session_attrs-type.patch
Description: Binary data


0005-New-read-only-target_session_attrs-type.patch
Description: Binary data


0006-Primary-prefer-standby-and-standby-options.patch
Description: Binary data


0007-New-function-to-rejecting-the-checked-write-connecti.patch
Description: Binary data


0008-Server-recovery-mode-handling.patch
Description: Binary data

RE: Libpq support to connect to standby server as priority

[Tsunakawa, Takayuki]

Hi Hari-san,

I've reviewed all the files.  The patch would be OK when the following have 
been fixed, except for the complexity of fe-connect.c (which probably cannot be 
improved.)

Unfortunately, I'll be absent next week.  The earliest date I can do the test 
will be April 8 or 9.  I hope someone could take care of this patch...


(1) 0001
With this read-only option type, application can connect to
to a read-only server in the list of hosts, in case
...
before issuing the SHOW transaction_readonly to find out whether


"to" appears twice in a row.
transaction_readonly -> transaction_read_only


(2) 0001
+succesful connection or failure.

succesful -> successful


(3) 0008
to conenct to a standby server with a faster check instead of

conenct -> connect


(4) 0008
Logically, recovery exit should be notified after the following statement:

XLogCtl->SharedRecoveryInProgress = false;


(5) 0008
+   /* Update in_recovery status. */
+   if (LocalRecoveryInProgress)
+   SetConfigOption("in_recovery",
+   "on",
+   PGC_INTERNAL, 
PGC_S_OVERRIDE);
+

This SetConfigOption() is called for every RecoveryInProgress() call on the 
standby.  It may cause undesirable overhead.  How about just calling 
SetConfigOption() once in InitPostgres() to set the initial value for 
in_recovery?  InitPostgres() and its subsidiary functions call 
SetConfigOption() likewise.


(6) 0008
async.c is for LISTEN/UNLISTEN/NOTIFY.  How about adding the new functions in 
postgres.c like RecoveryConflictInterrupt()?


(7) 0008
+   if (pid != 0)
+   {
+   (void) SendProcSignal(pid, reason, procvxid.backendId);
+   }

The braces are not necessary because the block only contains a single statement.


Regards
Takayuki Tsunakawa

Re: Libpq support to connect to standby server as priority

[Haribabu Kommi]

On Wed, Mar 27, 2019 at 5:17 PM Tsunakawa, Takayuki <
tsunakawa.ta...@jp.fujitsu.com> wrote:

> I've looked through 0004-0007.  I've only found the following:
>
> (5) 0005
> With this read-only option type, application can connect to
> connecting to a read-only server in the list of hosts, in case
> if there is any read-only servers available, the connection
> attempt fails.
>
> "connecting to" can be removed.
>
> in case if there is any read-only servers
> -> If There's no read only server
>

Thanks for the review.

I corrected all the comments that are raised by you and attached updated
version of patches
along with implementation of WIP in_recovery GUC_REPORT variable. This
patch has used some
of the implementations that are provided earlier in thread [1].

During the implementation of in_recovery configuration variable, I see a
lot of code addition just
for this variable, is this worth it?

[1] -
https://www.postgresql.org/message-id/2239254.dtfY1H9x0t%40hammer.magicstack.net

Regards,
Haribabu Kommi
Fujitsu Australia
From aa812716104b3fbd787dae4483341894c9e5ed9a Mon Sep 17 00:00:00 2001
From: Hari Babu 
Date: Thu, 21 Feb 2019 23:11:55 +1100
Subject: [PATCH 1/8] Restructure the code to remove duplicate code

The duplicate code logic of checking for the server version
before issuing the SHOW transaction_readonly to find out whether
the server is read-write or not is restructured under a new
connection status, so that duplicate code is removed. This is
required for the next set of patches
---
 doc/src/sgml/libpq.sgml   | 26 +---
 src/interfaces/libpq/fe-connect.c | 99 ---
 src/interfaces/libpq/libpq-fe.h   |  3 +-
 3 files changed, 57 insertions(+), 71 deletions(-)

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index c1d1b6b2db..6221e359f0 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1576,17 +1576,27 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
  
   target_session_attrs
   
+   
+The supported options for this parameter are, any and
+read-write. The default value of this parameter,
+any, regards all connections as acceptable.
+If multiple hosts were specified in the connection string, based on the
+specified value, any remaining servers will be tried before confirming
+succesful connection or failure.
+   
+

 If this parameter is set to read-write, only a
 connection in which read-write transactions are accepted 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
-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.
-  
+is considered acceptable.
+   
+
+   
+To find out whether the server supports read-write transactions are not,
+query SHOW transaction_read_only will be sent upon any
+successful connection; if it returns on, it means server
+doesn't support read-write transactions.
+   
   
 
 
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index e3bf6a7449..c68448786d 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -3188,6 +3188,43 @@ keep_going:		/* We will come back to here until there is
 	return PGRES_POLLING_WRITING;
 }
 
+conn->status = CONNECTION_CHECK_TARGET;
+goto keep_going;
+			}
+
+		case CONNECTION_SETENV:
+			{
+
+/*
+ * Do post-connection housekeeping (only needed in protocol 2.0).
+ *
+ * We pretend that the connection is OK for the duration of these
+ * queries.
+ */
+conn->status = CONNECTION_OK;
+
+switch (pqSetenvPoll(conn))
+{
+	case PGRES_POLLING_OK:	/* Success */
+		break;
+
+	case PGRES_POLLING_READING: /* Still going */
+		conn->status = CONNECTION_SETENV;
+		return PGRES_POLLING_READING;
+
+	case PGRES_POLLING_WRITING: /* Still going */
+		conn->status = CONNECTION_SETENV;
+		return PGRES_POLLING_WRITING;
+
+	default:
+		goto error_return;
+}
+			}
+
+			/* Intentional fall through */
+
+		case CONNECTION_CHECK_TARGET:
+			{
 /*
  * If a read-write connection is required, see if we have one.
  *
@@ -3229,68 +3266,6 @@ keep_going:		/* We will come back to here until there is
 return PGRES_POLLING_OK;
 			}
 
-		case CONNECTION_SETENV:
-
-			/*
-			 * Do post-connection housekeeping (only needed in protocol 2.0).
-			 *
-			 * We pretend that the connection is OK for the duration of these
-			 * queries.
-			 */
-			conn->status = CONNECTION_OK;
-
-			switch (pqSetenvPoll(conn))
-			{
-case 

RE: Libpq support to connect to standby server as priority

[Tsunakawa, Takayuki]

I've looked through 0004-0007.  I've only found the following:

(5) 0005
With this read-only option type, application can connect to
connecting to a read-only server in the list of hosts, in case
if there is any read-only servers available, the connection
attempt fails.

"connecting to" can be removed.

in case if there is any read-only servers
-> If There's no read only server


Regards
Takayuki Tsunakawa

RE: Libpq support to connect to standby server as priority

[Tsunakawa, Takayuki]

From: Haribabu Kommi [mailto:kommi.harib...@gmail.com]
> while going through the old patch where the GUC_REPORT is implemented, Tom
> has commented the logic of sending the signal to all backends to process
> the hot standby exit with SIGHUP, if we add the logic of updating the GUC
> variable value in SIGHUP, we may need to change all the SIGHUP handler code
> paths. It is also possible that there is no need to update the variable
> for other processes except backends.
> 
> If we go with adding the new SIGUSR1 type to check and update the GUC varaible
> can work for most of the backends and background workers.
> 
> opinions

SIGUSR1 looks reasonable.  We can consider it as notifying that the server 
status has changed.

I've fully reviewed 0001-0003 and my comments follow.  I'll review 0004-0007.


(1) 0001
before issuing the transaction_readonly to find out whether
the server is read-write or not is restuctured under a new


transaction_readonly -> "SHOW transaction_read_only"
restuctured -> restructured


(2) 0001
+succesful connection or failure.
+successful connection; if it returns on, means 
server

succesful -> successful
means -> it means


(3) 0003
+But for servers version 12 or greater uses the 
transaction_read_only
+GUC that is reported by the server upon successful connection.

GUC doesn't seem to be a term to be used in the user manual.  Instead:

uses the value of transaction_read_only configuration 
parameter that is...

as in:

https://www.postgresql.org/docs/devel/libpq-connect.html

client_encoding
This sets the client_encoding configuration parameter for this connection.

application_name
Specifies a value for the application_name configuration parameter.


(4) 0003
boolstd_strings;/* standard_conforming_strings */
+   booltransaction_read_only;  /* session_read_only */

Looking at the comment for std_strings, it's better to change the comment to 
transaction_read_only to represent the backing configuration parameter name.


Regards
Takayuki Tsunakawa

Re: Libpq support to connect to standby server as priority

[Haribabu Kommi]

On Fri, Mar 22, 2019 at 6:07 PM Haribabu Kommi 
wrote:

>
> On Fri, Mar 22, 2019 at 7:32 AM Haribabu Kommi 
> wrote:
>
>>
>> On Fri, Mar 22, 2019 at 6:57 AM Robert Haas 
>> wrote:
>>
>>> On Thu, Mar 21, 2019 at 2:26 AM Haribabu Kommi 
>>> wrote:
>>> > Based on the above new options that can be added to
>>> target_session_attrs,
>>> >
>>> > primary - it is just an alias to the read-write option.
>>> > standby, prefer-standby - These options should check whether server is
>>> running in recovery mode or not
>>> > instead of checking whether server accepts read-only connections or
>>> not?
>>>
>>> I think it will be best to have one set of attributes that check
>>> default_transaction_read_only and a differently-named set that check
>>> pg_is_in_recovery().  For each, there should be one value that looks
>>> for a 'true' return and one value that looks for a 'false' return and
>>> perhaps values that accept either but prefer one or the other.
>>>
>>> IOW, there's no reason to make primary an alias for read-write.  If
>>> you want read-write, you can just say read-write.  But we can make
>>> 'primary' or 'master' look for a server that's not in recovery and
>>> 'standby' look for one that is.
>>>
>>
>> OK, I agree with opinion. I will produce a patch for those new options.
>>
>
> Here I attached WIP patch for the new options along with other older
> patches.
> The basic cases are working fine, doc and tests are missing.
>
> Currently this patch doesn't implement the GUC_REPORT for recovery mode
> yet. I am yet to optimize the complex if check.
>

Except in_hotstandby GUC_REPORT, rest of the changes are implemented.
Updated patches are attached.

while going through the old patch where the GUC_REPORT is implemented,
Tom has commented the logic of sending the signal to all backends to process
the hot standby exit with SIGHUP, if we add the logic of updating the GUC
variable value in SIGHUP, we may need to change all the SIGHUP handler
code paths. It is also possible that there is no need to update the
variable for
other processes except backends.

If we go with adding the new SIGUSR1 type to check and update the GUC
varaible
can work for most of the backends and background workers.

opinions

Regards,
Haribabu Kommi
Fujitsu Australia

Note - Attachments order may sometime go wrong.


0001-Restructure-the-code-to-remove-duplicate-code.patch
Description: Binary data


0002-New-TargetSessionAttrsType-enum.patch
Description: Binary data


0003-Make-transaction_read_only-as-GUC_REPORT-varaible.patch
Description: Binary data


0005-New-read-only-target_session_attrs-type.patch
Description: Binary data


0004-New-prefer-read-target_session_attrs-type.patch
Description: Binary data


0006-Primary-prefer-standby-and-standby-options.patch
Description: Binary data


0007-New-function-to-rejecting-the-checked-write-connecti.patch
Description: Binary data

Re: Libpq support to connect to standby server as priority

[Haribabu Kommi]

On Fri, Mar 22, 2019 at 7:32 AM Haribabu Kommi 
wrote:

>
> On Fri, Mar 22, 2019 at 6:57 AM Robert Haas  wrote:
>
>> On Thu, Mar 21, 2019 at 2:26 AM Haribabu Kommi 
>> wrote:
>> > Based on the above new options that can be added to
>> target_session_attrs,
>> >
>> > primary - it is just an alias to the read-write option.
>> > standby, prefer-standby - These options should check whether server is
>> running in recovery mode or not
>> > instead of checking whether server accepts read-only connections or not?
>>
>> I think it will be best to have one set of attributes that check
>> default_transaction_read_only and a differently-named set that check
>> pg_is_in_recovery().  For each, there should be one value that looks
>> for a 'true' return and one value that looks for a 'false' return and
>> perhaps values that accept either but prefer one or the other.
>>
>> IOW, there's no reason to make primary an alias for read-write.  If
>> you want read-write, you can just say read-write.  But we can make
>> 'primary' or 'master' look for a server that's not in recovery and
>> 'standby' look for one that is.
>>
>
> OK, I agree with opinion. I will produce a patch for those new options.
>

Here I attached WIP patch for the new options along with other older
patches.
The basic cases are working fine, doc and tests are missing.

Currently this patch doesn't implement the GUC_REPORT for recovery mode
yet. I am yet to optimize the complex if check.

Regards,
Haribabu Kommi
Fujitsu Australia


0001-Restructure-the-code-to-remove-duplicate-code.patch
Description: Binary data


0002-New-TargetSessionAttrsType-enum.patch
Description: Binary data


0005-New-read-only-target_session_attrs-type.patch
Description: Binary data


0003-Make-transaction_read_only-as-GUC_REPORT-varaible.patch
Description: Binary data


0004-New-prefer-read-target_session_attrs-type.patch
Description: Binary data


0006-Primary-prefer-standby-and-standby-options.patch
Description: Binary data

Re: Libpq support to connect to standby server as priority

[Haribabu Kommi]

On Fri, Mar 22, 2019 at 6:57 AM Robert Haas  wrote:

> On Thu, Mar 21, 2019 at 2:26 AM Haribabu Kommi 
> wrote:
> > Based on the above new options that can be added to target_session_attrs,
> >
> > primary - it is just an alias to the read-write option.
> > standby, prefer-standby - These options should check whether server is
> running in recovery mode or not
> > instead of checking whether server accepts read-only connections or not?
>
> I think it will be best to have one set of attributes that check
> default_transaction_read_only and a differently-named set that check
> pg_is_in_recovery().  For each, there should be one value that looks
> for a 'true' return and one value that looks for a 'false' return and
> perhaps values that accept either but prefer one or the other.
>
> IOW, there's no reason to make primary an alias for read-write.  If
> you want read-write, you can just say read-write.  But we can make
> 'primary' or 'master' look for a server that's not in recovery and
> 'standby' look for one that is.
>

OK, I agree with opinion. I will produce a patch for those new options.

Regards,
Haribabu Kommi
Fujitsu Australia

Re: Libpq support to connect to standby server as priority

[Robert Haas]

On Thu, Mar 21, 2019 at 2:26 AM Haribabu Kommi  wrote:
> Based on the above new options that can be added to target_session_attrs,
>
> primary - it is just an alias to the read-write option.
> standby, prefer-standby - These options should check whether server is 
> running in recovery mode or not
> instead of checking whether server accepts read-only connections or not?

I think it will be best to have one set of attributes that check
default_transaction_read_only and a differently-named set that check
pg_is_in_recovery().  For each, there should be one value that looks
for a 'true' return and one value that looks for a 'false' return and
perhaps values that accept either but prefer one or the other.

IOW, there's no reason to make primary an alias for read-write.  If
you want read-write, you can just say read-write.  But we can make
'primary' or 'master' look for a server that's not in recovery and
'standby' look for one that is.

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

Re: Libpq support to connect to standby server as priority

[Haribabu Kommi]

On Wed, Mar 20, 2019 at 5:01 PM Tsunakawa, Takayuki <
tsunakawa.ta...@jp.fujitsu.com> wrote:

> From: Robert Haas [mailto:robertmh...@gmail.com]
> > I really dislike having both target_sesion_attrs and
> > target_server_type.  It doesn't solve any actual problem.  master,
> > slave, prefer-save, or whatever you like could be put in
> > target_session_attrs just as easily, and then we wouldn't end up with
> > two keywords doing closely related things.  'master' is no more or
> > less a server attribute than 'read-write'.
>
> Hmm, that may be OK.  At first, I felt it strange to treat the server type
> (primary or standby) as a session attribute.  But we can see the server
> type as one attribute in a sense that a session is established for.  I'm
> inclined to agree with:
>
> target_session_attr = {any | read-write | read-only | prefer-read |
> primary | standby | prefer-standby}
>

Thanks for your suggestions.

Based on the above new options that can be added to target_session_attrs,

primary - it is just an alias to the read-write option.
standby, prefer-standby - These options should check whether server is
running in recovery mode or not
instead of checking whether server accepts read-only connections or not?

Regards,
Haribabu Kommi
Fujitsu Australia

RE: Libpq support to connect to standby server as priority

[Tsunakawa, Takayuki]

From: Robert Haas [mailto:robertmh...@gmail.com]
> I really dislike having both target_sesion_attrs and
> target_server_type.  It doesn't solve any actual problem.  master,
> slave, prefer-save, or whatever you like could be put in
> target_session_attrs just as easily, and then we wouldn't end up with
> two keywords doing closely related things.  'master' is no more or
> less a server attribute than 'read-write'.

Hmm, that may be OK.  At first, I felt it strange to treat the server type 
(primary or standby) as a session attribute.  But we can see the server type as 
one attribute in a sense that a session is established for.  I'm inclined to 
agree with:

target_session_attr = {any | read-write | read-only | prefer-read | primary | 
standby | prefer-standby}


Regards
Takayuki Tsunakawa

Re: Libpq support to connect to standby server as priority

[Robert Haas]

On Mon, Mar 18, 2019 at 9:33 PM Haribabu Kommi  wrote:
> While working on implementation of target_server_type new connection option 
> for the libpq
> to specify master, slave and etc, there is no problem when the newly added 
> target_server_type
> option is used separate, but when it is combined with the existing 
> target_session_attrs, there
> may be some combinations that are not valid or such servers doesn't exist.
>
> Target_session_attrs  Target_server_type
>
> read-write   prefer-slave, slave
> prefer-read master, slave
> read-onlymaster, prefer-slave
>
> I know that some of the cases above is possible, like master server with by 
> default accepts
> read-only sessions. Instead of we put a check to validate what is right 
> combination, how
> about allowing the combinations and in case if such combination is not 
> possible, means
> there shouldn't be any server the supports the requirement, and connection 
> fails.
>
> comments?

I really dislike having both target_sesion_attrs and
target_server_type.  It doesn't solve any actual problem.  master,
slave, prefer-save, or whatever you like could be put in
target_session_attrs just as easily, and then we wouldn't end up with
two keywords doing closely related things.  'master' is no more or
less a server attribute than 'read-write'.

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

RE: Libpq support to connect to standby server as priority

[Tsunakawa, Takayuki]

From: Haribabu Kommi [mailto:kommi.harib...@gmail.com]
> Target_session_attrs  Target_server_type
> 
> read-write   prefer-slave, slave
> 
> prefer-read master, slave
> read-onlymaster, prefer-slave
> 
> I know that some of the cases above is possible, like master server with
> by default accepts
> read-only sessions. Instead of we put a check to validate what is right
> combination, how
> about allowing the combinations and in case if such combination is not
> possible, means
> there shouldn't be any server the supports the requirement, and connection
> fails.
> 
> comments?

I think that's OK.

To follow the existing naming, it seems better to use "primary" and "standby" 
instead of master and slave -- primary_conninfo, synchronous_standby_names, 
hot_standby, max_standby_streaming_delay and such.


> And also as we need to support the new option to connect to servers < 12
> also, this option
> sends the command "select pg_is_in_recovery()" to the server to find out
> whether the server
> is recovery mode or not?

The query looks good.  OTOH, I think we can return an error when 
target_server_type is specified against older servers because the parameter is 
new, if the libpq code would get uglier if we support older servers.


> And also regarding the implementation point of view, the new
> target_server_type option
> validation is separately handled, means the check for the required server
> is handled in a separate
> switch case, when both options are given, first find out the required server
> for target_session_attrs
> and validate the same again with target_server_type?

Logically, it seems the order should be reverse; check the server type first, 
then the session attributes, considering other session attributes in the future.


Regards
Takayuki Tsunakawa

Re: Libpq support to connect to standby server as priority

[Haribabu Kommi]

On Thu, Feb 28, 2019 at 1:00 PM Tsunakawa, Takayuki <
tsunakawa.ta...@jp.fujitsu.com> wrote:

> From: Haribabu Kommi [mailto:kommi.harib...@gmail.com]
> > Attached are the updated patches.
>
> Thanks, all look fixed.
>
>
> > The target_server_type option yet to be implemented.
>
> Please let me review once more and proceed to testing when the above is
> added, to make sure the final code looks good.  I'd like to see how complex
> the if conditions in multiple places would be after adding
> target_server_type, and consider whether we can simplify them together with
> you.  Even now, the if conditions seem complicated to me... that's probably
> due to the existence of read_write_host_index.
>

Yes, if checks are little bit complex because of additional checks to
identify, I will check if there is
any easier way to update them without introducing code duplication.

While working on implementation of target_server_type new connection option
for the libpq
to specify master, slave and etc, there is no problem when the newly added
target_server_type
option is used separate, but when it is combined with the existing
target_session_attrs, there
may be some combinations that are not valid or such servers doesn't exist.

Target_session_attrs  Target_server_type

read-write   prefer-slave, slave
prefer-read master, slave
read-onlymaster, prefer-slave

I know that some of the cases above is possible, like master server with by
default accepts
read-only sessions. Instead of we put a check to validate what is right
combination, how
about allowing the combinations and in case if such combination is not
possible, means
there shouldn't be any server the supports the requirement, and connection
fails.

comments?

And also as we need to support the new option to connect to servers < 12
also, this option
sends the command "select pg_is_in_recovery()" to the server to find out
whether the server
is recovery mode or not?

And also regarding the implementation point of view, the new
target_server_type option
validation is separately handled, means the check for the required server
is handled in a separate
switch case, when both options are given, first find out the required
server for target_session_attrs
and validate the same again with target_server_type?

Regards,
Haribabu Kommi
Fujitsu Australia

Re: Libpq support to connect to standby server as priority

[Dave Cramer]

> Now I will add the another parameter target_server_type to choose the
> primary, standby or prefer-standby
> as discussed in the upthreads with a new GUC variable.
>


So just to further confuse things here is a use case for "preferPrimary"

This is from the pgjdbc list.

"if the master instance fails, we would like the driver to communicate with
the secondary instance for read-only operations before the failover process
is commenced. The second use-case is when the master instance is
deliberately shut down for maintenance reasons and we do not want to fail
over to the secondary instance, but instead allow it to process user
queries throughout the maintenance."


see this for the thread.
https://www.postgresql.org/message-id/VI1PR05MB5295AE43EF9525EACC9E57ECBC750%40VI1PR05MB5295.eurprd05.prod.outlook.com

Dave Cramer

da...@postgresintl.com
www.postgresintl.com

RE: Libpq support to connect to standby server as priority

[Tsunakawa, Takayuki]

From: Haribabu Kommi [mailto:kommi.harib...@gmail.com]
> Attached are the updated patches.

Thanks, all look fixed.


> The target_server_type option yet to be implemented.

Please let me review once more and proceed to testing when the above is added, 
to make sure the final code looks good.  I'd like to see how complex the if 
conditions in multiple places would be after adding target_server_type, and 
consider whether we can simplify them together with you.  Even now, the if 
conditions seem complicated to me... that's probably due to the existence of 
read_write_host_index.


Regards
Takayuki Tsunakawa

Re: Libpq support to connect to standby server as priority

[Haribabu Kommi]

On Mon, Feb 25, 2019 at 11:38 AM Tsunakawa, Takayuki <
tsunakawa.ta...@jp.fujitsu.com> wrote:

> Hi Hari-san,
>
> I've reviewed all files.  I think I'll proceed to testing when I've
> reviewed the revised patch and the patch for target_server_type.
>
>
Thanks for the review.


>
> (1) patch 0001
> CONNECTION_CHECK_WRITABLE,  /* Check if we could make a
> writable
>  *
> connection. */
> +   CONNECTION_CHECK_TARGET,/* Check if we have a proper target
> +*
> connection */
> CONNECTION_CONSUME  /* Wait for any pending
> message and consume
>  * them. */
>
> According to the following comment, a new enum value should be added at
> the end.
>
> /*
>  * Although it is okay to add to these lists, values which become unused
>  * should never be removed, nor should constants be redefined - that would
>  * break compatibility with existing code.
>  */
>

fixed.


>
>
> (2) patch 0002
> It seems to align better with the existing code to set the default value
> to pg_conn.requested_session_type explicitly in makeEmptyPGconn(), even if
> the default value is 0.  Although I feel it's redundant, other member
> variables do so.
>
>
fixed.


>
> (3) patch 0003
>  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
> +transaction_read_only was not reported by releases
> before 12.0.)
>
> ";" is missing at the end of the third line.
>
>
fixed.


>
> (4) patch 0004
> -   /* Type of connection to make.  Possible values: any, read-write.
> */
> +   /* Type of connection to make.  Possible values: any, read-write,
> perfer-read. */
> char   *target_session_attrs;
>
> perfer -> prefer
>
>
fixed.


>
> (5) patch 0004
> @@ -3608,6 +3691,9 @@ makeEmptyPGconn(void)
> conn = NULL;
> }
>
> +   /* Initial value */
> +   conn->read_write_host_index = -1;
>
> The new member should be initialized earlier in this function.  Otherwise,
> as you can see in the above fragment, conn can be NULL in an out-of-memory
> case.
>
>
fixed.



>
> (6) patch 0004
> Don't we add read-only as well as prefer-read, which corresponds to Slave
> or Secondary of PgJDBC's targetServerType?  I thought the original proposal
> was to add both.
>
>
Added read-only option.



>
> (7) patch 0004
> @@ -2347,6 +2367,7 @@ keep_going:
>  /* We will come back to here until there is
>
> conn->try_next_addr = true;
> goto keep_going;
> }
> +
>
> appendPQExpBuffer(>errorMessage,
>
> libpq_gettext("could not create socket: %s\n"),
>
> Is this an unintended newline addition?
>
>
removed.


>
> (8) patch 0004
> +   const char *type =
> (conn->requested_session_type == SESSION_TYPE_PREFER_READ) ?
> +
>  "read-only" : "writable";
>
> I'm afraid these strings are not translatable into other languages.
>
>
OK. I added two separate error message prepare statements for both
read-write and read-only
so, it shouldn't be a problem.



>
> (9) patch 0004
> +   /* Record read-write host
> index */
> +   if
> (conn->read_write_host_index == -1)
> +
>  conn->read_write_host_index = conn->whichhost;
>
> At this point, the session can be either read-write or read-only, can't
> it?  Add the check "!conn->transaction_read_only" in this if condition?
>

Yes, fixed.


>
> (10) patch 0004
> +   if ((conn->target_session_attrs != NULL) &&
> +  (conn->requested_session_type
> == SESSION_TYPE_PREFER_READ) &&
> +  (conn->read_write_host_index !=
> -2))
>
> The first condition is not necessary because the second one exists.
>
> The parenthesis surrounding each of these conditions are redundant.  It
> would be better to remove them for readability.  This also applies to the
> following part:
>
> +   if (((strncmp(val, "on", 2) == 0)
> &&
> +
>  (conn->requested_session_type == SESSION_TYPE_READ_WRITE)) ||
> +   ((strncmp(val, "off", 3)
> == 0) &&
> +
>  (conn->requested_session_type == SESSION_TYPE_PREFER_READ) &&
> +
>  (conn->read_write_host_index != -2)))
>

fixed.

Attached are the updated patches.
The target_server_type option yet to be implemented.

Regards,
Haribabu Kommi
Fujitsu Australia


0003-Make-transaction_read_only-as-GUC_REPORT-varaible.patch
Description: Binary data


0002-New-TargetSessionAttrsType-enum.patch

RE: Libpq support to connect to standby server as priority

[Tsunakawa, Takayuki]

Hi Hari-san,

I've reviewed all files.  I think I'll proceed to testing when I've reviewed 
the revised patch and the patch for target_server_type.


(1) patch 0001
CONNECTION_CHECK_WRITABLE,  /* Check if we could make a writable
 * connection. 
*/
+   CONNECTION_CHECK_TARGET,/* Check if we have a proper target
+* connection */
CONNECTION_CONSUME  /* Wait for any pending message 
and consume
 * them. */

According to the following comment, a new enum value should be added at the end.

/*
 * Although it is okay to add to these lists, values which become unused
 * should never be removed, nor should constants be redefined - that would
 * break compatibility with existing code.
 */



(2) patch 0002
It seems to align better with the existing code to set the default value to 
pg_conn.requested_session_type explicitly in makeEmptyPGconn(), even if the 
default value is 0.  Although I feel it's redundant, other member variables do 
so.


(3) patch 0003
 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
+transaction_read_only was not reported by releases 
before 12.0.)

";" is missing at the end of the third line.


(4) patch 0004
-   /* Type of connection to make.  Possible values: any, read-write. */
+   /* Type of connection to make.  Possible values: any, read-write, 
perfer-read. */
char   *target_session_attrs;

perfer -> prefer


(5) patch 0004
@@ -3608,6 +3691,9 @@ makeEmptyPGconn(void)
conn = NULL;
}
 
+   /* Initial value */
+   conn->read_write_host_index = -1;

The new member should be initialized earlier in this function.  Otherwise, as 
you can see in the above fragment, conn can be NULL in an out-of-memory case.


(6) patch 0004
Don't we add read-only as well as prefer-read, which corresponds to Slave or 
Secondary of PgJDBC's targetServerType?  I thought the original proposal was to 
add both.


(7) patch 0004
@@ -2347,6 +2367,7 @@ keep_going:   
/* We will come back to here until there is
conn->try_next_addr = 
true;
goto keep_going;
}
+

appendPQExpBuffer(>errorMessage,

  libpq_gettext("could not create socket: %s\n"),

Is this an unintended newline addition?


(8) patch 0004
+   const char *type = 
(conn->requested_session_type == SESSION_TYPE_PREFER_READ) ?
+   
"read-only" : "writable";

I'm afraid these strings are not translatable into other languages.


(9) patch 0004
+   /* Record read-write host index 
*/
+   if (conn->read_write_host_index 
== -1)
+   
conn->read_write_host_index = conn->whichhost;

At this point, the session can be either read-write or read-only, can't it?  
Add the check "!conn->transaction_read_only" in this if condition?


(10) patch 0004
+   if ((conn->target_session_attrs != NULL) &&
+  (conn->requested_session_type == 
SESSION_TYPE_PREFER_READ) &&
+  (conn->read_write_host_index != -2))

The first condition is not necessary because the second one exists.

The parenthesis surrounding each of these conditions are redundant.  It would 
be better to remove them for readability.  This also applies to the following 
part:

+   if (((strncmp(val, "on", 2) == 0) &&
+   
(conn->requested_session_type == SESSION_TYPE_READ_WRITE)) ||
+   ((strncmp(val, "off", 3) == 0) 
&&
+   
(conn->requested_session_type == SESSION_TYPE_PREFER_READ) &&
+   
(conn->read_write_host_index != -2)))


Regards
Takayuki Tsunakawa

Re: Libpq support to connect to standby server as priority

[Haribabu Kommi]

On Fri, Feb 22, 2019 at 5:47 PM Tsunakawa, Takayuki <
tsunakawa.ta...@jp.fujitsu.com> wrote:

> From: Haribabu Kommi [mailto:kommi.harib...@gmail.com]
> Here I attached first set of patches that implemented the prefer-read
> option
> > after reporting the transaction_read_only GUC to client. Along the lines
> > of adding prefer-read option patch,
>
> Great, thank you!  I'll review and test it.
>

Thanks.


> > 3. Existing read-write code is modified to use the new reported GUC
> instead
> > of executing the show command.
>
> Is the code path left to use SHOW for older servers?
>

Yes, for older versions less than 12, still uses the SHOW command approach.

Regards,
Haribabu Kommi
Fujitsu Australia

RE: Libpq support to connect to standby server as priority

[Tsunakawa, Takayuki]

From: Haribabu Kommi [mailto:kommi.harib...@gmail.com]
Here I attached first set of patches that implemented the prefer-read option
> after reporting the transaction_read_only GUC to client. Along the lines
> of adding prefer-read option patch,

Great, thank you!  I'll review and test it.


> 3. Existing read-write code is modified to use the new reported GUC instead
> of executing the show command.

Is the code path left to use SHOW for older servers?


Regards
Takayuki Tsunakawa

Re: Libpq support to connect to standby server as priority

[Haribabu Kommi]

On Thu, Feb 14, 2019 at 1:04 PM Tsunakawa, Takayuki <
tsunakawa.ta...@jp.fujitsu.com> wrote:

> From: Haribabu Kommi [mailto:kommi.harib...@gmail.com]
> >   No.  It's not good if the user has to be bothered by
> > default_transaction_read_only when he simply wants to a standby.
> >
> >
> >
> > OK. Understood.
> > so if we are going to differentiate between readonly and standby types,
> > then I still
> > feel that adding a prefer-read to target_session_attrs is still valid
> > improvement.
>
> I agree that it's valid improvement to add prefer-read to
> target_session_attr, as a means to "get a read-only session."
>
> > But the above improvement can be enhanced once the base work of
> GUC_REPORT
> > is finished.
>
> Is it already in progress in some thread, or are you trying to start from
> scratch?  (I may have done it, but I don't remember it well...)
>
> > Yes, I want to work on this patch, hopefully by next commitfest. In case
> > if I didn't get time,
> > I can ask for your help.
>
> I'm glad to hear that.  Sure.  I'd like to review your patch, and possibly
> add/modify code if necessary.  Are you going to add
> target_server_type={primary | standby | prefer_standby} as well as add
> prefer-read to target_session_attr?
>
>
> >   (I wonder which of server_type or server_role feels natural in
> > English.)
> >
> >
> >
> > server_type may be good as it stands with connection option
> > (target_server_type).
>
> Thanks, agreed.  That also follows PgJDBC's targetServerType.
>

Here I attached first set of patches that implemented the prefer-read
option after reporting
the transaction_read_only GUC to client. Along the lines of adding
prefer-read option patch,

1. I refactor the existing code to reduce the duplicate.
2. Added a enum to represent the user requested target_session_attrs type,
this is used in
comparison instead of doing a strcmp always.
3. Existing read-write code is modified to use the new reported GUC instead
of executing the
show command.

Basic patches are working, there may still need some documentation works.

Now I will add the another parameter target_server_type to choose the
primary, standby or prefer-standby
as discussed in the upthreads with a new GUC variable.

Regards,
Haribabu Kommi
Fujitsu Australia


0004-New-prefer-read-target_session_attrs-type.patch
Description: Binary data


0001-Restructure-the-code-to-remove-duplicate-code.patch
Description: Binary data


0002-New-TargetSessionAttrsType-enum.patch
Description: Binary data


0003-Make-transaction_read_only-as-GUC_REPORT-varaible.patch
Description: Binary data

RE: Libpq support to connect to standby server as priority

[Tsunakawa, Takayuki]

From: Haribabu Kommi [mailto:kommi.harib...@gmail.com]
>   No.  It's not good if the user has to be bothered by
> default_transaction_read_only when he simply wants to a standby.
> 
> 
> 
> OK. Understood.
> so if we are going to differentiate between readonly and standby types,
> then I still
> feel that adding a prefer-read to target_session_attrs is still valid
> improvement.

I agree that it's valid improvement to add prefer-read to target_session_attr, 
as a means to "get a read-only session."

> But the above improvement can be enhanced once the base work of GUC_REPORT
> is finished.

Is it already in progress in some thread, or are you trying to start from 
scratch?  (I may have done it, but I don't remember it well...)

> Yes, I want to work on this patch, hopefully by next commitfest. In case
> if I didn't get time,
> I can ask for your help.

I'm glad to hear that.  Sure.  I'd like to review your patch, and possibly 
add/modify code if necessary.  Are you going to add target_server_type={primary 
| standby | prefer_standby} as well as add prefer-read to target_session_attr?


>   (I wonder which of server_type or server_role feels natural in
> English.)
> 
> 
> 
> server_type may be good as it stands with connection option
> (target_server_type).

Thanks, agreed.  That also follows PgJDBC's targetServerType.


Regards
Takayuki Tsunakawa

Re: Libpq support to connect to standby server as priority

[Haribabu Kommi]

On Fri, Feb 8, 2019 at 8:16 PM Tsunakawa, Takayuki <
tsunakawa.ta...@jp.fujitsu.com> wrote:

> From: Haribabu Kommi [mailto:kommi.harib...@gmail.com]
> > target_session_attrs checks for the default_transaction_readonly or not?
>
> PG 11 uses transaction_read_only, not default_transaction_readonly.
> That's fine, because its purpose is to get a read-only session as the name
> suggests, not to connect to a standby.
>

Thanks for correction, yes it uses the transaction_readonly.


> > target_server_type checks for whether the server is in recovery or not?
>
> Yes.
>
>
> > I feel having two options make this feature complex to use it from the
> user
> > point of view?
> >
> > The need of two options came because of a possibility of a master server
> > with default_transaction_readonly set to true. Even if the default
> > transaction
> > is readonly, it is user changeable parameter, so there shouldn't be any
> > problem.
>
> No.  It's not good if the user has to be bothered by
> default_transaction_read_only when he simply wants to a standby.
>

OK. Understood.
so if we are going to differentiate between readonly and standby types,
then I still
feel that adding a prefer-read to target_session_attrs is still valid
improvement.

But the above improvement can be enhanced once the base work of GUC_REPORT
is finished.



> > how about just adding one parameter that takes the options similar like
> > JDBC?
> > target_server_type - Master, standby and prefer-standby. (The option
> names
> > can revised based on the common words on the postgresql docs?)
>
> "Getting a read-only session" is not equal to "connecting to a standby",
> so two different parameters make sense.
>
>
> > And one more thing, what happens when the server promotes to master but
> > the connection requested is standby? I feel we can maintain the existing
> > connections
> > and later new connections can be redirected? comments?
>
> Ideally, it should be possible for the user to choose the behavior like
> Oracle below.  But that's a separate feature.
>
>
> 9.2 Role Transitions Involving Physical Standby Databases
>
> https://docs.oracle.com/en/database/oracle/oracle-database/18/sbydb/managing-oracle-data-guard-role-transitions.html#GUID-857F6F45-DC1C-4345-BD39-F3BE7D79F1CD
> --
> Keeping Physical Standby Sessions Connected During Role Transition
>
> As of Oracle Database 12c Release 2 (12.2.0.1), when a physical standby
> database is converted into a primary you have the option to keep any
> sessions connected to the physical standby connected, without disruption,
> during the switchover/failover.
>
> To enable this feature, set the STANDBY_DB_PRESERVE_STATES initialization
> parameter in your init.ora file before the standby instance is started.
> This parameter applies to physical standby databases only. The allowed
> values are:
>
> NONE — No sessions on the standby are retained during a
> switchover/failover. This is the default value.
>
> ALL — User sessions are retained during switchover/failover.
>
> SESSION — User sessions are retained during switchover/failover.
> --
>

Yes, the above feature is completely a different role enhancement feature,
that can taken up separately.


> Would you like to work on this patch?  I'm not sure if I can take time,
> but I'm willing to do it if you don't have enough time.
>
> As Tom mentioned, we need to integrate and clean patches in three mail
> threads:
>
> * Make a new GUC_REPORT parameter, server_type, to show the server role
> (primary or standby).
> * Add target_server_type libpq connection parameter, whose values are
> either primary, standby, or prefer_standby.
> * Failover timeout, load balancing, etc. that someone proposed in the
> other thread?
>

Yes, I want to work on this patch, hopefully by next commitfest. In case if
I didn't get time,
I can ask for your help.


> (I wonder which of server_type or server_role feels natural in English.)
>

server_type may be good as it stands with connection option
(target_server_type).

Regards,
Haribabu Kommi
Fujitsu Australia

RE: Libpq support to connect to standby server as priority

[Tsunakawa, Takayuki]

From: Haribabu Kommi [mailto:kommi.harib...@gmail.com]
> target_session_attrs checks for the default_transaction_readonly or not?

PG 11 uses transaction_read_only, not default_transaction_readonly.  That's 
fine, because its purpose is to get a read-only session as the name suggests, 
not to connect to a standby.

> target_server_type checks for whether the server is in recovery or not?

Yes.


> I feel having two options make this feature complex to use it from the user
> point of view?
> 
> The need of two options came because of a possibility of a master server
> with default_transaction_readonly set to true. Even if the default
> transaction
> is readonly, it is user changeable parameter, so there shouldn't be any
> problem.

No.  It's not good if the user has to be bothered by 
default_transaction_read_only when he simply wants to a standby.

> how about just adding one parameter that takes the options similar like
> JDBC?
> target_server_type - Master, standby and prefer-standby. (The option names
> can revised based on the common words on the postgresql docs?)

"Getting a read-only session" is not equal to "connecting to a standby", so two 
different parameters make sense.


> And one more thing, what happens when the server promotes to master but
> the connection requested is standby? I feel we can maintain the existing
> connections
> and later new connections can be redirected? comments?

Ideally, it should be possible for the user to choose the behavior like Oracle 
below.  But that's a separate feature.


9.2 Role Transitions Involving Physical Standby Databases
https://docs.oracle.com/en/database/oracle/oracle-database/18/sbydb/managing-oracle-data-guard-role-transitions.html#GUID-857F6F45-DC1C-4345-BD39-F3BE7D79F1CD
--
Keeping Physical Standby Sessions Connected During Role Transition

As of Oracle Database 12c Release 2 (12.2.0.1), when a physical standby 
database is converted into a primary you have the option to keep any sessions 
connected to the physical standby connected, without disruption, during the 
switchover/failover. 

To enable this feature, set the STANDBY_DB_PRESERVE_STATES initialization 
parameter in your init.ora file before the standby instance is started. This 
parameter applies to physical standby databases only. The allowed values are: 

NONE — No sessions on the standby are retained during a switchover/failover. 
This is the default value. 

ALL — User sessions are retained during switchover/failover. 

SESSION — User sessions are retained during switchover/failover. 
--


Would you like to work on this patch?  I'm not sure if I can take time, but I'm 
willing to do it if you don't have enough time.

As Tom mentioned, we need to integrate and clean patches in three mail threads:

* Make a new GUC_REPORT parameter, server_type, to show the server role 
(primary or standby).
* Add target_server_type libpq connection parameter, whose values are either 
primary, standby, or prefer_standby.
* Failover timeout, load balancing, etc. that someone proposed in the other 
thread?

(I wonder which of server_type or server_role feels natural in English.)

Or, would you like to share the work, e.g., libpq and server-side?


Regards
Takayuki Tsunakawa

Re: Libpq support to connect to standby server as priority

[Haribabu Kommi]

On Mon, Jan 21, 2019 at 5:48 PM Tsunakawa, Takayuki <
tsunakawa.ta...@jp.fujitsu.com> wrote:

> From: Haribabu Kommi [mailto:kommi.harib...@gmail.com]
> > Thanks for finding out the problem, how about the following way of
> checking
> > for prefer-read/prefer-standby.
> >
> > 1. (default_transaction_read_only = true and recovery = true)
> >
> > 2. If none of the host satisfies the above scenario, then recovery = true
> > 3. Last check is for default_transaction_read_only = true
>
> That would be fine.  But as I mentioned in another mail, I think "get
> read-only session" and "connect to standby" differ.  So I find it better to
> separate parameters for those request; target_session_attr and
> target_server_type.
>

Thanks for your opinion.

target_session_attrs checks for the default_transaction_readonly or not?
target_server_type checks for whether the server is in recovery or not?

I feel having two options make this feature complex to use it from the user
point of view?

The need of two options came because of a possibility of a master server
with default_transaction_readonly set to true. Even if the default
transaction
is readonly, it is user changeable parameter, so there shouldn't be any
problem.

The same can be applied for logical replication also, the user can change
the
default transaction mode once the connection is established, if it is not
according
to it's requirement.

how about just adding one parameter that takes the options similar like
JDBC?
target_server_type - Master, standby and prefer-standby. (The option names
can revised based on the common words on the postgresql docs?)

And one more thing, what happens when the server promotes to master but
the connection requested is standby? I feel we can maintain the existing
connections
and later new connections can be redirected? comments?

Regards,
Haribabu Kommi
Fujitsu Australia

Re: Libpq support to connect to standby server as priority

[Michael Paquier]

On Mon, Jan 21, 2019 at 06:48:14AM +, Tsunakawa, Takayuki wrote:
> That would be fine.  But as I mentioned in another mail, I think
> "get read-only session" and "connect to standby" differ.  So I find
> it better to separate parameters for those request;
> target_session_attr and target_server_type. 

We've had plenty of discussions about this patch, and nothing really
got out of the crowd.  For now I am marking the patch as returned with
feedback as it has been marked as waiting on author for two weeks now.
It may be worth continuing the discussion, still we need to come up
with an agreement first.
--
Michael


signature.asc
Description: PGP signature

RE: Libpq support to connect to standby server as priority

[Tsunakawa, Takayuki]

From: Haribabu Kommi [mailto:kommi.harib...@gmail.com]
> Thanks for finding out the problem, how about the following way of checking
> for prefer-read/prefer-standby.
> 
> 1. (default_transaction_read_only = true and recovery = true)
> 
> 2. If none of the host satisfies the above scenario, then recovery = true
> 3. Last check is for default_transaction_read_only = true

That would be fine.  But as I mentioned in another mail, I think "get read-only 
session" and "connect to standby" differ.  So I find it better to separate 
parameters for those request; target_session_attr and target_server_type.


Regards
Takayuki Tsunakawa

RE: Libpq support to connect to standby server as priority

[Tsunakawa, Takayuki]

From: Laurenz Albe [mailto:laurenz.a...@cybertec.at]
> Tsunakawa, Takayuki wrote:
> > I'm sorry to repeat myself, but anyway, I think we need a method to connect
> to a standby
> > as the original desire, because the primary instance may be read only
> by default while
> > only limited users update data.  That's for reducing the burdon on the
> primary and
> > minimizing the impact on users who update data.  For example,
> >
> > * run data reporting on the standby
> > * backup the database from the standby
> > * cascade replication from the standby
> 
> I see.
> 
> But then the new value should not be called "prefer-read", because that
> would be
> misleading.  It would also not be related to the existing "read-write".
> 
> For what you have in mind, there should be the options "primary-required"
> and
> "standby-preferred", however we implement them.

Yes, that's what I'm proposing and expecting with a new parameter whose naming 
follows PgJDBC's. 

> Have there been a lot of complaints that the existing "read-write" is not
> good
> enough to detect replication primaries?

I haven't heard anything.  I guess almost nobody uses 
default_transaction_read_only.

Before that, see the description of target_session_attr:

https://www.postgresql.org/docs/devel/libpq-connect.html#LIBPQ-PARAMKEYWORDS

I'm afraid most users don't know whether they can connect to the 
primary/standby.  Just searching "primary", "master" or "standby" in this page 
doesn't show anything relevant.


> One use case I can think of is logical replication (or other replication
> methods like
> Slony).  You can use the feature by setting default_transaction_read_only
> = on
> on the standby.

I know that, but I suspect that's really a practical use case.  Anyway, I'm OK 
with relying on target_session_attr to fulfill that need.


Regards
Takayuki Tsunakawa

Re: Libpq support to connect to standby server as priority

[Haribabu Kommi]

On Fri, Jan 18, 2019 at 5:33 PM Tsunakawa, Takayuki <
tsunakawa.ta...@jp.fujitsu.com> wrote:

> From: Tsunakawa, Takayuki [mailto:tsunakawa.ta...@jp.fujitsu.com]
> > As for prefer-standby/prefer-read, if host parameter specifies
> host1,host2
> > in this order, and host1 is the primary with
> > default_transaction_read_only=true, does the app get a connection to
> host1?
> > I want to connect to host2 (standby) only if host1 is down.
>
> Oops, reverse -- I wanted to say "I want to connect to host1 (primary)
> only if host2 is down."
>

Thanks for finding out the problem, how about the following way of checking
for prefer-read/prefer-standby.

1. (default_transaction_read_only = true and recovery = true)
2. If none of the host satisfies the above scenario, then recovery = true
3. Last check is for default_transaction_read_only = true

Regards,
Haribabu Kommi
Fujitsu Australia

Re: Libpq support to connect to standby server as priority

[Laurenz Albe]

Tsunakawa, Takayuki wrote:
> From: Laurenz Albe [mailto:laurenz.a...@cybertec.at]
> > I think that transaction_read_only is good.
> > 
> > If it is set to false, we are sure to be on a replication primary or
> > stand-alone server, which is enough to know for the load balancing use case.
> 
> As Tatsuo-san said, setting default_transaction_read_only leads to a 
> misjudgement of the primary.

Yes, you can have a false negative, i.e. fail to recognize a primary server.

> > I deem it unlikely that someone will set default_transaction_read_only to
> > FALSE and then complain that the feature is not working as expected, but
> > again
> > I cannot prove that claim.
> 
> I wonder what default_transaction_read_only exists for.  For maing the 
> database by default
> and allowing only specific users to write to the database with "CREATE/ALTER 
> USER SET
> default_transaction_read_only = false"?

I'd guess that the main use of default_transaction_read_only is to make sure an
application (that isn't smart enough to change the parameter) won't modify any 
data.

> I'm sorry to repeat myself, but anyway, I think we need a method to connect 
> to a standby
> as the original desire, because the primary instance may be read only by 
> default while
> only limited users update data.  That's for reducing the burdon on the 
> primary and
> minimizing the impact on users who update data.  For example,
> 
> * run data reporting on the standby
> * backup the database from the standby
> * cascade replication from the standby

I see.

But then the new value should not be called "prefer-read", because that would be
misleading.  It would also not be related to the existing "read-write".

For what you have in mind, there should be the options "primary-required" and
"standby-preferred", however we implement them.

Have there been a lot of complaints that the existing "read-write" is not good
enough to detect replication primaries?

> > As Robert said, transaction_read_only might even give you the option to
> > use the feature for more than just load balancing between replication master
> > and standby.
> 
> What use case do you think of?  If you want to load balance the workload 
> between
> the primary and standbys, we can follow PgJDBC -- targetServerType=any.

One use case I can think of is logical replication (or other replication 
methods like
Slony).  You can use the feature by setting default_transaction_read_only = on
on the standby.

Yours,
Laurenz Albe

RE: Libpq support to connect to standby server as priority

[Tsunakawa, Takayuki]

From: Tsunakawa, Takayuki [mailto:tsunakawa.ta...@jp.fujitsu.com]
> As for prefer-standby/prefer-read, if host parameter specifies host1,host2
> in this order, and host1 is the primary with
> default_transaction_read_only=true, does the app get a connection to host1?
> I want to connect to host2 (standby) only if host1 is down.

Oops, reverse -- I wanted to say "I want to connect to host1 (primary) only if 
host2 is down."

Regards
Takayuki Tsunakawa

Re: Libpq support to connect to standby server as priority

[Tatsuo Ishii]

>> If you need some input from me regarding finding a primary node,
>> please say so.  While working on Pgpool-II project, I learned the
>> necessity in a hard way.
>>
>>
> I would really like to have a consistent way of doing this, and consistent
> terms for the connection parameters.
> 
> that said yes, I would like input from you.

Sure, no problem.

- Upon Pgpool-II starting up or recieving failover event or switch
  over event, primary node finding is executed.

- It repeats following until timeout parameter
  ("search_primary_node_timeout" is expired)

do until the timeout is expired
{
for all_live_backends
{
connect to the backend.
execute "SELECT pg_is_in_recovery()".

if it returns false, the we find the primary node. Assume
other backend as standbys and we are done.
disconnect to the backend
}
sleep 1 second;
}

If no primary node was found, all backends are regarded as standbys.

In addition to above, recent Pgpool-II versions does optional checking
to verify backend status, for example, finding a case where there
are two primary nodes.

- If there are two primaries, check the connectivity between each
  primary and standbys using pg_stat_wal_receiver() (so this can not
  be executed with PostgreSQL version 9.5 or before)

- If there's a primary (call it "A") which is not connected to any of
  standbys while there's a primary (call it "B") which is connected to
  all of standbys, then A is regarded as a "false primary" (and
  Pgpool-II detaches it from the streaming replication cluster managed
  by Pgpool-II if detach_false_primary is enabled).

See Pgpool-II manual "detach_false_primary" section in
http://tatsuo-ishii.github.io/pgpool-II/current/runtime-config-failover.html 
for more details.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

RE: Libpq support to connect to standby server as priority

[Tsunakawa, Takayuki]

From: Haribabu Kommi [mailto:kommi.harib...@gmail.com]
> IMO, if we try to use only pg_is_in_recovery() only to decide to connect,
> we may not
> support all the target_session_attrs that are possible. how about using
> both to decide?

I favor adding a new parameter like target_server_type whose purpose is to 
select the server role.  That aligns better with the PgJDBC's naming, which 
conveys consistency to PostgreSQL users.  Again, the original desire should 
have been to connect to a standby to eliminate the burdon on the primary, where 
the primary may be read-only by default and only a limited group of users are 
allowed to write to the database.

I don't know what kind of realistic use cases there are that request read-only 
session in the logical replication configuration.  I think we can just leave 
target_session_attrs as what it is now in PostgreSQL 11, for compatibility and 
possibly future new use cases.


> Master/read-write -- recovery = false and default_transaction_read_only
> = false
> Standby/read-only -- recovery = true
> prefer-standby/prefer-read -- recovery = true or
> default_transaction_read_only = true
> any -- Nothing to be verified
> 
> 
> I feel above verifications can cover for both physical and logical
> replication.

As for prefer-standby/prefer-read, if host parameter specifies host1,host2 in 
this order, and host1 is the primary with default_transaction_read_only=true, 
does the app get a connection to host1?  I want to connect to host2 (standby) 
only if host1 is down.


Regards
Takayuki Tsunakawa

Re: Libpq support to connect to standby server as priority

[Haribabu Kommi]

On Fri, Jan 18, 2019 at 2:34 PM Tsunakawa, Takayuki <
tsunakawa.ta...@jp.fujitsu.com> wrote:

> From: Laurenz Albe [mailto:laurenz.a...@cybertec.at]
> > I think that transaction_read_only is good.
> >
> > If it is set to false, we are sure to be on a replication primary or
> > stand-alone server, which is enough to know for the load balancing use
> case.
>
> As Tatsuo-san said, setting default_transaction_read_only leads to a
> misjudgement of the primary.
>
>
> > I deem it unlikely that someone will set default_transaction_read_only to
> > FALSE and then complain that the feature is not working as expected, but
> > again
> > I cannot prove that claim.
>
> I wonder what default_transaction_read_only exists for.  For maing the
> database by default and allowing only specific users to write to the
> database with "CREATE/ALTER USER SET default_transaction_read_only = false"?
>

default_transaction_read_only is a user settable parameter, even if it set
as true by default,
any user can change it later. Deciding server type based on this whether it
supports read-write
or read-only can go wrong, as the user can change it later.


> I'm sorry to repeat myself, but anyway, I think we need a method to
> connect to a standby as the original desire, because the primary instance
> may be read only by default while only limited users update data.  That's
> for reducing the burdon on the primary and minimizing the impact on users
> who update data.  For example,
>
> * run data reporting on the standby
> * backup the database from the standby
> * cascade replication from the standby
>

IMO, if we try to use only pg_is_in_recovery() only to decide to connect,
we may not
support all the target_session_attrs that are possible. how about using
both to decide?

Master/read-write -- recovery = false and default_transaction_read_only =
false
Standby/read-only -- recovery = true
prefer-standby/prefer-read -- recovery = true or
default_transaction_read_only = true
any -- Nothing to be verified

I feel above verifications can cover for both physical and logical
replication.
we can decide what type of options that we can support? and also if we
don't want to rely on default_transaction_read_only user settable parameter,
we can add a new parameter that cannot be changed only with server restart?

Regards,
Haribabu Kommi
Fujitsu Australia

RE: Libpq support to connect to standby server as priority

[Tsunakawa, Takayuki]

From: Laurenz Albe [mailto:laurenz.a...@cybertec.at]
> I think that transaction_read_only is good.
> 
> If it is set to false, we are sure to be on a replication primary or
> stand-alone server, which is enough to know for the load balancing use case.

As Tatsuo-san said, setting default_transaction_read_only leads to a 
misjudgement of the primary.


> I deem it unlikely that someone will set default_transaction_read_only to
> FALSE and then complain that the feature is not working as expected, but
> again
> I cannot prove that claim.

I wonder what default_transaction_read_only exists for.  For maing the database 
by default and allowing only specific users to write to the database with 
"CREATE/ALTER USER SET default_transaction_read_only = false"?

I'm sorry to repeat myself, but anyway, I think we need a method to connect to 
a standby as the original desire, because the primary instance may be read only 
by default while only limited users update data.  That's for reducing the 
burdon on the primary and minimizing the impact on users who update data.  For 
example,

* run data reporting on the standby
* backup the database from the standby
* cascade replication from the standby



> As Robert said, transaction_read_only might even give you the option to
> use the feature for more than just load balancing between replication master
> and standby.

What use case do you think of?  If you want to load balance the workload 
between the primary and standbys, we can follow PgJDBC -- targetServerType=any.


Regards
Takayuki Tsunakawa

Re: Libpq support to connect to standby server as priority

[Dave Cramer]

On Thu, 17 Jan 2019 at 19:56, Tatsuo Ishii  wrote:

> >> > I'm curious; under what circumstances would the above occur?
> >>
> >> Former primary goes down and one of standbys is promoting but it is
> >> not promoted to new primary yet.
> >>
> >
> > seems like JDBC might have some work to do...Thanks
> >
> > I'm going to wait to implement until we resolve this discussion
>
> If you need some input from me regarding finding a primary node,
> please say so.  While working on Pgpool-II project, I learned the
> necessity in a hard way.
>
>
I would really like to have a consistent way of doing this, and consistent
terms for the connection parameters.

that said yes, I would like input from you.

Thanks,

Dave

Re: Libpq support to connect to standby server as priority

[Tatsuo Ishii]

>> > I'm curious; under what circumstances would the above occur?
>>
>> Former primary goes down and one of standbys is promoting but it is
>> not promoted to new primary yet.
>>
> 
> seems like JDBC might have some work to do...Thanks
> 
> I'm going to wait to implement until we resolve this discussion

If you need some input from me regarding finding a primary node,
please say so.  While working on Pgpool-II project, I learned the
necessity in a hard way.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

Re: Libpq support to connect to standby server as priority

[Dave Cramer]

On Thu, 17 Jan 2019 at 19:38, Tatsuo Ishii  wrote:

> >> >> >> > From: Tatsuo Ishii [mailto:is...@sraoss.co.jp]
> >> >> >> >> But pg_is_in_recovery() returns true even for a promoting
> >> standby. So
> >> >> >> >> you have to wait and retry to send pg_is_in_recovery() until it
> >> >> >> >> finishes the promotion to find out it is now a primary. I am
> not
> >> sure
> >> >> >> >> if backend out to be responsible for this process. If not,
> libpq
> >> >> would
> >> >> >> >> need to handle it but I doubt it would be possible.
> >> >> >> >
> >> >> >> > Yes, the application needs to retry connection attempts until
> >> success.
> >> >> >> That's not different from PgJDBC and other DBMSs.
> >> >> >>
> >> >> >> I don't know what PgJDBC is doing, however I think libpq needs to
> do
> >> >> >> more than just retrying.
> >> >> >>
> >> >> >> 1) Try to find a node on which pg_is_in_recovery() returns false.
> If
> >> >> >>found, then we assume that is the primary. We also assume that
> >> >> >>other nodes are standbys. done.
> >> >> >>
> >> >> >> 2) If there's no node on which pg_is_in_recovery() returns false,
> >> then
> >> >> >>we need to retry until we find it. To not retry forever, there
> >> >> >>should be a timeout counter parameter.
> >> >> >>
> >> >> >>
> >> >> > IIRC this is essentially what pgJDBC does.
> >> >>
> >> >> Thanks for clarifying that. Pgpool-II also does that too. Seems like
> a
> >> >> common technique to find out a primary node.
> >> >>
> >> >>
> >> > Checking the code I see we actually use show transaction_read_only.
> >> >
> >> > Sorry for the confusion
> >>
> >> So if all PostgreSQL servers returns transaction_read_only = on, how
> >> does pgJDBC find the primary node?
> >>
> >> well preferSecondary would return a connection.
>
> This is not my message :-)
>
> > I'm curious; under what circumstances would the above occur?
>
> Former primary goes down and one of standbys is promoting but it is
> not promoted to new primary yet.
>

seems like JDBC might have some work to do...Thanks

I'm going to wait to implement until we resolve this discussion

Dave

>
>

Re: Libpq support to connect to standby server as priority

[Tatsuo Ishii]

>> >> >> > From: Tatsuo Ishii [mailto:is...@sraoss.co.jp]
>> >> >> >> But pg_is_in_recovery() returns true even for a promoting
>> standby. So
>> >> >> >> you have to wait and retry to send pg_is_in_recovery() until it
>> >> >> >> finishes the promotion to find out it is now a primary. I am not
>> sure
>> >> >> >> if backend out to be responsible for this process. If not, libpq
>> >> would
>> >> >> >> need to handle it but I doubt it would be possible.
>> >> >> >
>> >> >> > Yes, the application needs to retry connection attempts until
>> success.
>> >> >> That's not different from PgJDBC and other DBMSs.
>> >> >>
>> >> >> I don't know what PgJDBC is doing, however I think libpq needs to do
>> >> >> more than just retrying.
>> >> >>
>> >> >> 1) Try to find a node on which pg_is_in_recovery() returns false. If
>> >> >>found, then we assume that is the primary. We also assume that
>> >> >>other nodes are standbys. done.
>> >> >>
>> >> >> 2) If there's no node on which pg_is_in_recovery() returns false,
>> then
>> >> >>we need to retry until we find it. To not retry forever, there
>> >> >>should be a timeout counter parameter.
>> >> >>
>> >> >>
>> >> > IIRC this is essentially what pgJDBC does.
>> >>
>> >> Thanks for clarifying that. Pgpool-II also does that too. Seems like a
>> >> common technique to find out a primary node.
>> >>
>> >>
>> > Checking the code I see we actually use show transaction_read_only.
>> >
>> > Sorry for the confusion
>>
>> So if all PostgreSQL servers returns transaction_read_only = on, how
>> does pgJDBC find the primary node?
>>
>> well preferSecondary would return a connection.

This is not my message :-)

> I'm curious; under what circumstances would the above occur?

Former primary goes down and one of standbys is promoting but it is
not promoted to new primary yet.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

Re: Libpq support to connect to standby server as priority

[Dave Cramer]

On Thu, 17 Jan 2019 at 19:09, Tsunakawa, Takayuki <
tsunakawa.ta...@jp.fujitsu.com> wrote:

> From: Dave Cramer [mailto:p...@fastcrypt.com]
> >   >> 2) If there's no node on which pg_is_in_recovery() returns
> false,
> > then
> >   >>we need to retry until we find it. To not retry forever,
> there
> >   >>should be a timeout counter parameter.
>
> > Checking the code I see we actually use show transaction_read_only.
>
> Also, does PgJDBC really repeat connection attempts for a user-specified
> duration?  Having a quick look at the code, it seemed to try each host once
> in a while loop.
>

You are correct looking at the code again. On the initial connection
attempt we only try once.


Dave Cramer

da...@postgresintl.com
www.postgresintl.com

>
>

Re: Libpq support to connect to standby server as priority

[Dave Cramer]

On Thu, 17 Jan 2019 at 19:15, Tatsuo Ishii  wrote:

> > On Thu, 17 Jan 2019 at 18:03, Tatsuo Ishii  wrote:
> >
> >> > On Wed, 16 Jan 2019 at 01:02, Tatsuo Ishii 
> wrote:
> >> >
> >> >> > From: Tatsuo Ishii [mailto:is...@sraoss.co.jp]
> >> >> >> But pg_is_in_recovery() returns true even for a promoting
> standby. So
> >> >> >> you have to wait and retry to send pg_is_in_recovery() until it
> >> >> >> finishes the promotion to find out it is now a primary. I am not
> sure
> >> >> >> if backend out to be responsible for this process. If not, libpq
> >> would
> >> >> >> need to handle it but I doubt it would be possible.
> >> >> >
> >> >> > Yes, the application needs to retry connection attempts until
> success.
> >> >> That's not different from PgJDBC and other DBMSs.
> >> >>
> >> >> I don't know what PgJDBC is doing, however I think libpq needs to do
> >> >> more than just retrying.
> >> >>
> >> >> 1) Try to find a node on which pg_is_in_recovery() returns false. If
> >> >>found, then we assume that is the primary. We also assume that
> >> >>other nodes are standbys. done.
> >> >>
> >> >> 2) If there's no node on which pg_is_in_recovery() returns false,
> then
> >> >>we need to retry until we find it. To not retry forever, there
> >> >>should be a timeout counter parameter.
> >> >>
> >> >>
> >> > IIRC this is essentially what pgJDBC does.
> >>
> >> Thanks for clarifying that. Pgpool-II also does that too. Seems like a
> >> common technique to find out a primary node.
> >>
> >>
> > Checking the code I see we actually use show transaction_read_only.
> >
> > Sorry for the confusion
>
> So if all PostgreSQL servers returns transaction_read_only = on, how
> does pgJDBC find the primary node?
>
> well preferSecondary would return a connection.

I'm curious; under what circumstances would the above occur?

Regards,
Dave

Re: Libpq support to connect to standby server as priority

[Tatsuo Ishii]

> On Thu, 17 Jan 2019 at 18:03, Tatsuo Ishii  wrote:
> 
>> > On Wed, 16 Jan 2019 at 01:02, Tatsuo Ishii  wrote:
>> >
>> >> > From: Tatsuo Ishii [mailto:is...@sraoss.co.jp]
>> >> >> But pg_is_in_recovery() returns true even for a promoting standby. So
>> >> >> you have to wait and retry to send pg_is_in_recovery() until it
>> >> >> finishes the promotion to find out it is now a primary. I am not sure
>> >> >> if backend out to be responsible for this process. If not, libpq
>> would
>> >> >> need to handle it but I doubt it would be possible.
>> >> >
>> >> > Yes, the application needs to retry connection attempts until success.
>> >> That's not different from PgJDBC and other DBMSs.
>> >>
>> >> I don't know what PgJDBC is doing, however I think libpq needs to do
>> >> more than just retrying.
>> >>
>> >> 1) Try to find a node on which pg_is_in_recovery() returns false. If
>> >>found, then we assume that is the primary. We also assume that
>> >>other nodes are standbys. done.
>> >>
>> >> 2) If there's no node on which pg_is_in_recovery() returns false, then
>> >>we need to retry until we find it. To not retry forever, there
>> >>should be a timeout counter parameter.
>> >>
>> >>
>> > IIRC this is essentially what pgJDBC does.
>>
>> Thanks for clarifying that. Pgpool-II also does that too. Seems like a
>> common technique to find out a primary node.
>>
>>
> Checking the code I see we actually use show transaction_read_only.
> 
> Sorry for the confusion

So if all PostgreSQL servers returns transaction_read_only = on, how
does pgJDBC find the primary node?

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp