Обсуждение: Libpq support to connect to standby server as priority

From: Jing Wang [mailto:jingwangian@gmail.com]
> This is a proposal that let libpq support 'prefer-read' option in
> target_session_attrs in pg_conn. The 'prefer-read' means the libpq will
> try to connect to a 'read-only' server firstly from the multiple server
> addresses. If failed to connect to the 'read-only' server then it will try
> to connect to the 'read-write' server.

There's a pending patch I started.  I'd be happy if you could continue this.

https://commitfest.postgresql.org/15/1148/

Regards
Takayuki Tsunakawa

Haribabu Kommi wrote:
> On Wed, Jan 24, 2018 at 9:01 AM Jing Wang <jingwangian@gmail.com> wrote:
> > Hi All,
> > 
> > Recently I put a proposal to support 'prefer-read' parameter in target_session_attrs in libpq. Now I updated the
patchwith adding content in the sgml and regression test case.
 
> > 
> > Some people may have noticed there is already another patch (https://commitfest.postgresql.org/15/1148/ ) which
lookssimilar with this. But I would say this patch is more complex than my proposal. 
 
> > 
> > It is better separate these 2 patches to consider.
> 
> I also feel prefer-read and read-only options needs to take as two different options.
> prefer-read is simple to support than read-only.
> 
> Here I attached an updated patch that is rebased to the latest master and also
> fixed some of the corner scenarios.

The patch applies, builds and passes "make check-world".

I think the "prefer-read" functionality is desirable: It is exactly what you need
if you want to use replication for load balancing, and your application supports
different database connections for reading and writing queries.

"read-only" does not have a clear use case in my opinion.

With the patch, PostgreSQL behaves as expected if I have a primary and a standby and run:

  psql "host=/tmp,/tmp port=5433,5434 target_session_attrs=prefer-read"

But if I stop the standby (port 5434), libpq goes into an endless loop.

Concerning the code:

- The documentation needs some attention. Suggestion:

   If this parameter is set to <literal>prefer-read</literal>, connections
   where <literal>SHOW transaction_read_only</literal> returns off are preferred.
   If no such connection can be found, a connection that allows read-write
   transactions will be accepted.

- I think the construction with "read_write_host_index" makes the code even more
  complicated than it already is.

  What about keeping the first successful connection open and storing it in a
  variable if we are in "prefer-read" mode.
  If we get the read-only connection we desire, close that cached connection,
  otherwise use it.

Yours,
Laurenz Albe

Haribabu Kommi wrote:
> > On Wed, Jul 4, 2018 at 11:14 PM Laurenz Albe <laurenz.albe@cybertec.at> wrote:
> > > Haribabu Kommi wrote:
> > >  
> > > - I think the construction with "read_write_host_index" makes the code even more
> > >   complicated than it already is.
> > > 
> > >   What about keeping the first successful connection open and storing it in a
> > >   variable if we are in "prefer-read" mode.
> > >   If we get the read-only connection we desire, close that cached connection,
> > >   otherwise use it.
> > 
> > Even if we add a variable to cache the connection, I don't think the logic of checking
> > the next host for the read-only host logic may not change, but the extra connection
> > request to the read-write host again will be removed.
> 
> I evaluated your suggestion of caching the connection and reuse it when there is no
> read only server doesn't find, but I am thinking that it will add more complexity and also
> the connection to the other servers delays, the cached connection may be closed by
> the server also because of timeout.
> 
> I feel the extra time during connection may be fine, if user is preferring the prefer-read
> mode, instead of adding more complexity in handling the cached connection? 
> 
> comments?

I tested the new patch, and it works as expected.

I don't think that time-out of the cached session is a valid concern, because that
would have to be a really short timeout.
On the other hand, establishing the connection twice (first to check if it is read-only,
then again because no read-only connection is found) can be quite costly.

But that is a matter of debate, as is the readability of the code.

Since I don't think I can contribute more to this patch, I'll mark it as
ready for committer.

Yours,
Laurenz Albe

On Wed, Jul 4, 2018 at 9:14 AM, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
>   What about keeping the first successful connection open and storing it in a
>   variable if we are in "prefer-read" mode.
>   If we get the read-only connection we desire, close that cached connection,
>   otherwise use it.

I like this idea.  If I recall correctly, the logic in this area is
getting pretty complex, so we might need to refactor it for better
readability and maintainability.

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

Haribabu Kommi wrote:
> On Thu, Jul 19, 2018 at 10:59 PM Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
> > On Wed, Jul 18, 2018 at 10:53 PM Robert Haas <robertmhaas@gmail.com> wrote:
> > > On Wed, Jul 4, 2018 at 9:14 AM, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
> > > >   What about keeping the first successful connection open and storing it in a
> > > >   variable if we are in "prefer-read" mode.
> > > >   If we get the read-only connection we desire, close that cached connection,
> > > >   otherwise use it.
> > > 
> > > I like this idea.  If I recall correctly, the logic in this area is
> > > getting pretty complex, so we might need to refactor it for better
> > > readability and maintainability.
> > 
> > OK. I will work on the code refactoring first and then provide the
> > prefer-read option on top it.
> 
> commits d1c6a14bacf and 5ca00774194 have refactored the logic
> of handling the different connection states.
> 
> Attached is a rebased patch after further refactoring the new option
> code for easier maintenance.

The code is much more readable now, thanks.

--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -397,6 +397,7 @@ struct pg_conn
    int         nconnhost;      /* # of hosts named in conn string */
    int         whichhost;      /* host we're currently trying/connected to */
    pg_conn_host *connhost;     /* details about each named host */
+   int         read_write_host_index; /* index for first read-write host in connhost */
 
    /* Connection data */
    pgsocket    sock;           /* FD for socket, PGINVALID_SOCKET if

I think the comment could use more love.

This would be the place to document the logic:
Initial value is -1, then then index of the first working server
we found, and -2 for the second attempt to connect to that server.


I notice that you don't keep the first connection open, but close
and reopen it.  I guess that is a matter of taste, but it would be
easier on resources (and reduce connection time) if the connection
were kept open.
Admittedly, it would be more difficult and might further complicate
code that is not very clear as it is.

If you work on some more on the comment above, I will mark it as
ready for committer.

Yours,
Laurenz Albe

Haribabu Kommi wrote:
> Added comments along the lines that you mentioned. And also try
> to update some more comments.

Looks ok to me, I'll mark it as "ready for committer".

Yours,
Laurenz Albe

Laurenz Albe <laurenz.albe@cybertec.at> writes:
> Haribabu Kommi wrote:
>> Added comments along the lines that you mentioned. And also try
>> to update some more comments.

> Looks ok to me, I'll mark it as "ready for committer".

I don't like this patch at all: the business with keeping two connections
open seems impossibly fragile and full of race conditions.  (For instance,
by the time you return the read-only session to the application, it might
not be read-only any more.  I also wonder what inquiry functions like
PQsocket ought to return while in this state.)  I think the feature
definition needs to be re-thought to make that unnecessary.

Also, we really need to consider the interaction between this and the
feature(s) being discussed in the thread at

https://www.postgresql.org/message-id/flat/1700970.cRWpxnom9y%40hammer.magicstack.net

            regards, tom lane

Tom Lane wrote:
> Laurenz Albe <laurenz.albe@cybertec.at> writes:
> > Haribabu Kommi wrote:
> > > Added comments along the lines that you mentioned. And also try
> > > to update some more comments.
> > Looks ok to me, I'll mark it as "ready for committer".
> 
> I don't like this patch at all: the business with keeping two connections
> open seems impossibly fragile and full of race conditions.  (For instance,
> by the time you return the read-only session to the application, it might
> not be read-only any more.  I also wonder what inquiry functions like
> PQsocket ought to return while in this state.)  I think the feature
> definition needs to be re-thought to make that unnecessary.

As it is now, the patch doesn't keep two connections open.  It remembers
the index of the host of the first successful writable connection, but
closes the connection, and opens another one to that host if no read-only
host can be found.

If the read-only connection turns writable after it has been tested,
but before it is returned, that can hardly be avoided.
I don't think that's so bad - after all, you asked for a read-only
connection *if possible*.
If you demand that the server be not promoted until the connection has
been returned to the client, you'd somehow have to block the server
from being promoted, right?

> Also, we really need to consider the interaction between this and the
> feature(s) being discussed in the thread at
> 
> https://www.postgresql.org/message-id/flat/1700970.cRWpxnom9y%40hammer.magicstack.net

That's a good point.

Yours,
Laurenz Albe

On Tue, Nov 13, 2018 at 05:54:17PM +1100, Haribabu Kommi wrote:
> On Tue, Nov 13, 2018 at 7:26 AM Laurenz Albe <laurenz.albe@cybertec.at>
> wrote:
>> As it is now, the patch doesn't keep two connections open.  It remembers
>> the index of the host of the first successful writable connection, but
>> closes the connection, and opens another one to that host if no read-only
>> host can be found.

That's commented in the patch as follows:
+    /*
+     * Requested type is prefer-read, then record this host index
+     * and try the other before considering it later
+     */
+    if ((conn->target_session_attrs != NULL) &&
+        (strcmp(conn->target_session_attrs, "prefer-read") == 0) &&
+        (conn->read_write_host_index != -2))

>> If the read-only connection turns writable after it has been tested,
>> but before it is returned, that can hardly be avoided.
>> I don't think that's so bad - after all, you asked for a read-only
>> connection *if possible*.

Yeah, it is difficult to guarantee that except that checking from time
to time that the connection is still read-only after establishing it.
It is in my opinion mostly a matter of documentation, meaning that the
selection is done when the connection is attempted from the defined
set.

> I also have the same opinion of Laurenz, that this option is letting the
> application to connect to read-only server if possible, otherwise let it
> connect to read-write server.
>
> I feel that any of the state changes during the connection and after
> connection, needs not to be reflected on the existing connection for
> these type of connections.

+      /*
+       * Reset the host index value to avoid recursion during the
+       * second connection attempt.
+       */
+      conn->read_write_host_index = -2;

Okay, this gets ugly.  I am pretty sure that we should use instead a
status flag and actually avoid any kind of recursion risk in the logic.
Or else it would get hard to track to which value what needs to be set
where in the code.

From purely the code point of view, it seems to me that it is actually
more simple to implement a "read-only" mode as this way there is no need
to mix between CONNECTION_CHECK_READONLY and CONNECTION_CHECK_WRITABLE,
remembering the past index of a connection which may be needed later on
if the next ones don't meet with the wanted conditions.

Each time I have heard about load balancing, applications did not really
care about whether only standbys were used for a set of queries and
accepted that the primary also shared some of the read-only load, be it
for analytics or OLTP, in which case "any" covers already everything
needed.  And if you really want a standby, "read-only" would also be
useful so as an application layer can properly fail if there is only a
primary available.

JDBC has its own set of options with targetServerType: master, slave,
secondary, preferSlave and preferSecondary.  What's proposed here is
preferSlave and what we would miss is slave at the end.
--
Michael

On Thu, Nov 15, 2018 at 05:14:33PM +0900, Michael Paquier wrote:
> JDBC has its own set of options with targetServerType: master, slave,
> secondary, preferSlave and preferSecondary.  What's proposed here is
> preferSlave and what we would miss is slave at the end.

So thinking a couple of extra minutes on this one, I am wondering if it
would be better to close completely the gap with two patches:
1) Get "read-only" done first, which uses most of the existing
infrastructure.  That seems simple enough.
2) Get "prefer-read" and "prefer-write", which need some new
infrastructure to track the last preferred connection depending on what
the client wants.
--
Michael

Laurenz Albe <laurenz.albe@cybertec.at> writes:
> Tom Lane wrote:
>> I don't like this patch at all: the business with keeping two connections
>> open seems impossibly fragile and full of race conditions.

> As it is now, the patch doesn't keep two connections open.  It remembers
> the index of the host of the first successful writable connection, but
> closes the connection, and opens another one to that host if no read-only
> host can be found.

Oh!  The reason I assumed it wasn't doing that is that such a behavior
seems completely insane.  If the point is to keep down the load on your
master server, then connecting only to immediately disconnect is not
a friendly way to do that --- even without counting the fact that you
might later come back and connect again.

If that's the best we can do, we should forget the whole feature and
just recommend putting slave servers first in your hosts list when
you want prefer-slave.

            regards, tom lane

Tom Lane wrote:
> > As it is now, the patch doesn't keep two connections open.  It remembers
> > the index of the host of the first successful writable connection, but
> > closes the connection, and opens another one to that host if no read-only
> > host can be found.
> 
> Oh!  The reason I assumed it wasn't doing that is that such a behavior
> seems completely insane.  If the point is to keep down the load on your
> master server, then connecting only to immediately disconnect is not
> a friendly way to do that --- even without counting the fact that you
> might later come back and connect again.

That's why I had argued initially to keep the session open, but you
seem to dislike that idea as well.

> If that's the best we can do, we should forget the whole feature and
> just recommend putting slave servers first in your hosts list when
> you want prefer-slave.

If you know which is which, certainly.
But in a setup with automated failover you cannot be certain which is which.
That's what the proposed feature targets.

Yours,
Laurenz Albe

On Sat, Nov 17, 2018 at 10:41:54AM +1100, Haribabu Kommi wrote:
> Yes, we need either session open or reconnect it approach to find out
> the whether server is read-write or read-only.

Even if there is no agreement on this part, wouldn't a read-only option
be enough to support any case?  With a cluster of two nodes, one primary
and one standby, a connection string listing both nodes would fail only
in the middle of a planned failover.  If you rinse and repeat it is
possible to have a larger control at application level because you
precisely know the whole state of the cluster at a given instant.

> And also for read-only or prefer-read connection types, once after the
> connection establishment is done, later server promotes to read-write,
> I feel we can continue the connection, that decision makes the feature
> simple, or do we want to stop the connection?

That feels like something the application needs to care about once the
session is established, but that's only my take on the matter.
--
Michael

On Fri, Nov 16, 2018 at 11:35 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Oh!  The reason I assumed it wasn't doing that is that such a behavior
> seems completely insane.  If the point is to keep down the load on your
> master server, then connecting only to immediately disconnect is not
> a friendly way to do that --- even without counting the fact that you
> might later come back and connect again.

That seems like a really weak argument.  Opening a connection to the
master surely isn't free, but it must be vastly cheaper than the cost
of the queries you intend to run.  I mean, no reasonable production
user of PostgreSQL opens a connection, runs one or two short queries,
and then closes the connection.  You open a connection and keep it
open for minutes, hours, days, or longer, running hundreds, thousands,
or millions of queries.  The cost of checking whether you've got a
master or a standby is a drop in the bucket.

And, I mean, if there's some scenario where what I just said isn't
true, well then don't use this feature in that particular case.

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

This patch is marked as "ready for committer", but that characterization
seems way over-optimistic.  It looks like there are several unsettled
questions:

1. The connection parameter name and values are unlike the very similar
feature in pgJDBC.  I think this is a fair complaint.  Now I'm not in love
with "hostRecheckSeconds" --- that seems like a very squishily defined
thing with limited use-case, given Robert's argument that you shouldn't
be using this feature at all for short-lived connections.  And
"loadBalanceHosts" is something we could leave for later.  But it seems
pretty unfortunate not to follow pgJDBC's lead on the main parameter,
"targetServerType=(any | master | secondary | preferSecondary)".

The problem here of course is that whoever invented target_session_attrs
was unconcerned with following that precedent, so what we have is
"target_session_attrs=(any | read-write)".
Are we prepared to add some aliases in service of unifying these names?

2. Whether or not you want to follow pgJDBC's naming, it seems like we
ought to have both "require read only" and "prefer read only" behaviors
in this patch, and maybe likewise "require read write" versus "prefer
read write".

3. We ought to sync this up with whatever's going to happen in
https://commitfest.postgresql.org/21/1090/
at least to the extent of agreeing on what GUCs we'd like to see
the server start reporting.

4. Given that other discussion, it's not quite clear what we should
even be checking.  The existing logic devolves to checking that
transaction_read_only is true, but that's not really the same thing as
"is a master server", eg you might have connected to a master server
under a role that has SET ROLE default_transaction_read_only = false.
(I wonder what pgJDBC is really checking, under the hood.)
Do we want to have modes that are checking hot-standby state in some
fashion, rather than the transaction_read_only state?

            regards, tom lane

From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
> The problem here of course is that whoever invented target_session_attrs
> was unconcerned with following that precedent, so what we have is
> "target_session_attrs=(any | read-write)".
> Are we prepared to add some aliases in service of unifying these names?

I think "yes".

> 2. Whether or not you want to follow pgJDBC's naming, it seems like we
> ought to have both "require read only" and "prefer read only" behaviors
> in this patch, and maybe likewise "require read write" versus "prefer
> read write".

Agreed, although I don't see a use case for "prefer read write".  I don't think there's an app like "I want to write,
butI'm OK if I cannot."
 


> 3. We ought to sync this up with whatever's going to happen in
> https://commitfest.postgresql.org/21/1090/
> at least to the extent of agreeing on what GUCs we'd like to see
> the server start reporting.

Yes.

> 4. Given that other discussion, it's not quite clear what we should
> even be checking.  The existing logic devolves to checking that
> transaction_read_only is true, but that's not really the same thing as
> "is a master server", eg you might have connected to a master server
> under a role that has SET ROLE default_transaction_read_only = false.
> (I wonder what pgJDBC is really checking, under the hood.)
> Do we want to have modes that are checking hot-standby state in some
> fashion, rather than the transaction_read_only state?

PgJDBC uses transaction_read_only like this:

[core/v3/ConnectionFactoryImpl.java]
  private boolean isMaster(QueryExecutor queryExecutor) throws SQLException, IOException {
    byte[][] results = SetupQueryRunner.run(queryExecutor, "show transaction_read_only", true);
    String value = queryExecutor.getEncoding().decode(results[0]);
    return value.equalsIgnoreCase("off");
  }

But as some people said, I don't think this is the right way.  I suppose what's leading to the current somewhat
complicatedsituation is that there was no easy way for the client to know whether the server is the master.  That ended
upin using "SHOW transaction_read_only" instead, and people supported that compromise by saying "read only status is
moreuseful than whether the server is standby or not," I'm afraid.
 

The original desire should have been the ability to connect to a primary or a standby.  So, I think we should go back
tothe original thinking (and not complicate the feature), and create a read only GUC_REPORT variable, say, server_role,
thatidentifies whether the server is a primary or a standby.
 


Regards
Takayuki Tsunakawa

On Tue, Jan 15, 2019 at 02:00:57AM +0000, Tsunakawa, Takayuki wrote:
> But as some people said, I don't think this is the right way.  I
> suppose what's leading to the current somewhat complicated situation
> is that there was no easy way for the client to know whether the
> server is the master.  That ended up in using "SHOW
> transaction_read_only" instead, and people supported that compromise
> by saying "read only status is more useful than whether the server
> is standby or not," I'm afraid.

Right.  Another pin point is that it is complicated for a client to be
sure that it is connected to a standby as at the time between
transaction_read_only is checked and the connection is reported as
ready to be used for the application, you may be actually linked to a
primary which has just recently been promoted.  I am not personally
sure if it is worth caring about that in such level of details to get
to get something useful, but there have been doubts about not making
that absolutely right to leverage correctly applications willing to
use read-only clients.

> The original desire should have been the ability to connect to a
> primary or a standby.  So, I think we should go back to the original
> thinking (and not complicate the feature), and create a read only
> GUC_REPORT variable, say, server_role, that identifies whether the
> server is a primary or a standby.

From the point of view of making sure that a client is really
connected to  a primary or a standby, this is the best idea around.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Tue, Jan 15, 2019 at 02:00:57AM +0000, Tsunakawa, Takayuki wrote:
>> The original desire should have been the ability to connect to a
>> primary or a standby.  So, I think we should go back to the original
>> thinking (and not complicate the feature), and create a read only
>> GUC_REPORT variable, say, server_role, that identifies whether the
>> server is a primary or a standby. 

> From the point of view of making sure that a client is really
> connected to  a primary or a standby, this is the best idea around.

There are a couple of issues here:

1. Are you sure there are no use-cases for testing transaction_read_only
as such?

2. What will the fallback implementation be, when connecting to a server
too old to have the variable you want?

            regards, tom lane

On Mon, Jan 14, 2019 at 5:17 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> The problem here of course is that whoever invented target_session_attrs
> was unconcerned with following that precedent, so what we have is
> "target_session_attrs=(any | read-write)".
> Are we prepared to add some aliases in service of unifying these names?

I wasn't unconcerned about the problem, but I wasn't prepared to to be
the first person who added a connection parameter that used
namesLikeThis instead of names_like_this, especially if the semantics
weren't exactly the same.  That seemed to be a recipe for somebody
yelling at me, and I try to avoid that when I can.

> 4. Given that other discussion, it's not quite clear what we should
> even be checking.  The existing logic devolves to checking that
> transaction_read_only is true, but that's not really the same thing as
> "is a master server", eg you might have connected to a master server
> under a role that has SET ROLE default_transaction_read_only = false.
> (I wonder what pgJDBC is really checking, under the hood.)
> Do we want to have modes that are checking hot-standby state in some
> fashion, rather than the transaction_read_only state?

Well, this has been discussed before, too, I'm pretty sure, but I'm
too lazy to go find the old discussion right now.  The upshot is that
default_transaction_read_only lets an administrator make a server look
read-only even if it technically isn't, which somebody might find
useful.  Otherwise what do you do if, for example, you are using
logical replication?  None of your servers are in recovery, but you
can make some of them report default_transaction_read_only = true if
you like.  To me, that kind of configurability is a feature, not a
bug.

That being said, I don't object to having even more values for
target_session_attrs that check other things.  You could have:

read_only: default_transaction_read_only => true
read_write: default_transaction_read_only => false
master: pg_is_in_recovery => false
standby: pg_is_in_recovery => true

But what I think would be a Very Bad Plan is to use confused naming
that looks for something different than what it purports to do.  For
example, if you were to change things so that read_write checks
pg_is_in_recovery(), then you might ask for a "read-write" server and
get one where only read-only transactions are permitted.  We need not
assume that "read-write master" and "read-only standby" are the only
two kinds of things that can ever exist, as long as we're careful
about the names we choose.  Choosing the names carefully also helps to
avoid POLA violations.

Another point I'd like to mention is that target_session_attrs could
be extended to care about other kinds of properties which someone
might want a server to have, quite apart from
master/standby/read-only/read-write.  I don't know exactly what sort
of thing somebody might care about, but the name is such that we can
decide to care about other properties in the future without having to
add a whole new parameter.  You can imagine a day when someone can say
target_session_attrs=read-write,v42+,ftl to get a server connection
that is read-write on a server running PostgreSQL 42 or greater that
also has a built-in hyperdrive.  Or whatever.

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

From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
> Michael Paquier <michael@paquier.xyz> writes:
> > On Tue, Jan 15, 2019 at 02:00:57AM +0000, Tsunakawa, Takayuki wrote:
> >> The original desire should have been the ability to connect to a
> >> primary or a standby.  So, I think we should go back to the original
> >> thinking (and not complicate the feature), and create a read only
> >> GUC_REPORT variable, say, server_role, that identifies whether the
> >> server is a primary or a standby.
> 
> > From the point of view of making sure that a client is really
> > connected to  a primary or a standby, this is the best idea around.
> 
> There are a couple of issues here:


> 1. Are you sure there are no use-cases for testing transaction_read_only
> as such?

I don't find any practical use case, but I won't object to leaving the current target_session_attrs as-is.  Alide from
that,I think a parameter like PgJDBC's is necessary, e.g., target_server_type = {primary | standby | prefer_standby},
whichacts based on just the server role (primary or standby).
 



> 2. What will the fallback implementation be, when connecting to a server
> too old to have the variable you want?

One of the following:

1. "Unsupported" error.  I'll take this.
2. libpq issues "SELECT pg_is_in_recovery()".
3. Blindly accepts the first successful connection.


Regards
Takayuki Tsunakawa

From: Dave Cramer [mailto:pg@fastcrypt.com]
>     The original desire should have been the ability to connect to a
> primary or a standby.  So, I think we should go back to the original thinking
> (and not complicate the feature), and create a read only GUC_REPORT variable,
> say, server_role, that identifies whether the server is a primary or a
> standby.
> 
> 
> 
> I'm confused as to how this would work. Who or what determines if the server
> is a primary or standby?

Overall, the server determines the server role (primary or standby) using the same mechanism as pg_is_in_recovery(),
andset the server_role GUC parameter.  As the parameter is GUC_REPORT, the change is reported to the clients using the
ParameterStatus('S') message.  The clients also get the value at connection.
 


Regards
Takayuki Tsunakawa

>> I'm confused as to how this would work. Who or what determines if the server
>> is a primary or standby?
> 
> Overall, the server determines the server role (primary or standby) using the same mechanism as pg_is_in_recovery(),
andset the server_role GUC parameter.  As the parameter is GUC_REPORT, the change is reported to the clients using the
ParameterStatus('S') message.  The clients also get the value at connection.
 

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.

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

From: Tatsuo Ishii [mailto:ishii@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.


Regards
Takayuki Tsunakawa

> From: Tatsuo Ishii [mailto:ishii@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.

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

From: Tatsuo Ishii [mailto:ishii@sraoss.co.jp]
> 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.

It may be convenient for libpq to be able to retry connection attempts for a specified duration (by failover_timeout or
such),because it eliminates the need for the application to do the retry.  But I think it's a desirable feature, not a
requiredone.
 


Regards
Takayuki Tsunakawa

Tsunakawa, Takayuki wrote:
> From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
> > The problem here of course is that whoever invented target_session_attrs
> > was unconcerned with following that precedent, so what we have is
> > "target_session_attrs=(any | read-write)".
> > Are we prepared to add some aliases in service of unifying these names?
> 
> I think "yes".
> 
> > 2. Whether or not you want to follow pgJDBC's naming, it seems like we
> > ought to have both "require read only" and "prefer read only" behaviors
> > in this patch, and maybe likewise "require read write" versus "prefer
> > read write".
> 
> Agreed, although I don't see a use case for "prefer read write".  I don't think
> there's an app like "I want to write, but I'm OK if I cannot."

I don't think so either, although of course I cannot prove it.

My opinion is that we shouldn't add options like "prefer read write"
just out of a fuzzy desire for symmetry.  It would probably make the code
even more complicated, and more choice means that it becomes harder for
the user to pick the right one (the latter may be a weak argument).

The motivation behind all this is to load balance reading and writing
sessions among a group of replicating servers where you don't know for sure
who is what at the moment, so "preferably read-only", "must be able to write"
and "don't care" are choice enough.

There is nothing that bars future patches from adding additional modes
if the need really arises.

> > 4. Given that other discussion, it's not quite clear what we should
> > even be checking.  The existing logic devolves to checking that
> > transaction_read_only is true, but that's not really the same thing as
> > "is a master server", eg you might have connected to a master server
> > under a role that has SET ROLE default_transaction_read_only = false.
> 
> PgJDBC uses transaction_read_only like this: [...]
> 
> But as some people said, I don't think this is the right way.  I suppose what's leading
> to the current somewhat complicated situation is that there was no easy way for the
> client to know whether the server is the master.  That ended up in using
> "SHOW transaction_read_only" instead, and people supported that compromise by saying
> "read only status is more useful than whether the server is standby or not," I'm afraid.
> 
> The original desire should have been the ability to connect to a primary or a standby.
> So, I think we should go back to the original thinking (and not complicate the feature),
> and create a read only GUC_REPORT variable, say, server_role, that identifies whether
> the server is a primary or a standby.

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.

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.

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.

Yours,
Laurenz Albe

> On Wed, 16 Jan 2019 at 01:02, Tatsuo Ishii <ishii@sraoss.co.jp> wrote:
> 
>> > From: Tatsuo Ishii [mailto:ishii@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.

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

From: Dave Cramer [mailto:pg@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
seemedto try each host once in a while loop.
 


Regards
Takayuki Tsunakawa

> On Thu, 17 Jan 2019 at 18:03, Tatsuo Ishii <ishii@sraoss.co.jp> wrote:
> 
>> > On Wed, 16 Jan 2019 at 01:02, Tatsuo Ishii <ishii@sraoss.co.jp> wrote:
>> >
>> >> > From: Tatsuo Ishii [mailto:ishii@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

>> >> >> > From: Tatsuo Ishii [mailto:ishii@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

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

From: Laurenz Albe [mailto:laurenz.albe@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
usersto 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,
becausethe primary instance may be read only by default while only limited users update data.  That's for reducing the
burdonon 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
followPgJDBC -- targetServerType=any.
 


Regards
Takayuki Tsunakawa

From: Haribabu Kommi [mailto:kommi.haribabu@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
withthe PgJDBC's naming, which conveys consistency to PostgreSQL users.  Again, the original desire should have been to
connectto a standby to eliminate the burdon on the primary, where the primary may be read-only by default and only a
limitedgroup 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
possiblyfuture 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
ifhost1 is down.
 


Regards
Takayuki Tsunakawa

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

From: Tsunakawa, Takayuki [mailto:tsunakawa.takay@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

Tsunakawa, Takayuki wrote:
> From: Laurenz Albe [mailto:laurenz.albe@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

From: Laurenz Albe [mailto:laurenz.albe@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
fulfillthat need.
 


Regards
Takayuki Tsunakawa

From: Haribabu Kommi [mailto:kommi.haribabu@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

On Mon, Jan 21, 2019 at 06:48:14AM +0000, 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

From: Haribabu Kommi [mailto:kommi.haribabu@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-onlysession 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
theoption 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
standbyinstance 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
enoughtime.
 

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

From: Haribabu Kommi [mailto:kommi.haribabu@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
rememberit 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
toadd 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

From: Haribabu Kommi [mailto:kommi.haribabu@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

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
inmakeEmptyPGconn(), even if the default value is 0.  Although I feel it's redundant, other member variables do so.
 


(3) patch 0003
     <varname>IntervalStyle</varname> was not reported by releases before 8.4;
-    <varname>application_name</varname> was not reported by releases before 9.0.)
+    <varname>application_name</varname> was not reported by releases before 9.0
+    <varname>transaction_read_only</varname> 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
canbe 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
thoughtthe 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(&conn->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

From: Haribabu Kommi [mailto:kommi.haribabu@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'dlike to see how complex the if conditions in multiple places would be after adding target_server_type, and consider
whetherwe can simplify them together with you.  Even now, the if conditions seem complicated to me... that's probably
dueto the existence of read_write_host_index.
 


Regards
Takayuki Tsunakawa

From: Haribabu Kommi [mailto:kommi.haribabu@gmail.com]
> Target_session_attrs      Target_server_type
> 
> read-write                       prefer-slave, slave
> 
> prefer-read                     master, slave
> read-only                        master, 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
becausethe 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
othersession attributes in the future.
 


Regards
Takayuki Tsunakawa

On Mon, Mar 18, 2019 at 9:33 PM Haribabu Kommi <kommi.haribabu@gmail.com> 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-only                        master, 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

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

On Thu, Mar 21, 2019 at 2:26 AM Haribabu Kommi <kommi.haribabu@gmail.com> 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

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

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 Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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 Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

From: Alvaro Herrera from 2ndQuadrant [mailto:alvherre@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
discussionon 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'sidea that the community may not accept the latter patches, e.g. accept only 0001-0005.
 


Regards
Takayuki Tsunakawa

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

> From: Alvaro Herrera from 2ndQuadrant [mailto:alvherre@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 Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Sep 11, 2019 at 10:17 AM Alvaro Herrera from 2ndQuadrant
<alvherre@alvh.no-ip.org> 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

On Wed, Sep 11, 2019 at 10:17 AM Alvaro Herrera from 2ndQuadrant
<alvherre@alvh.no-ip.org> 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

From: Greg Nancarrow <gregn4422@gmail.com>
> 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
modificationis 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
blockprecludes 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_onlyis touched in the former.
 


Regards
Takayuki Tsunakawa

On 2019-Oct-01, Greg Nancarrow wrote:

> On Wed, Sep 11, 2019 at 10:17 AM Alvaro Herrera from 2ndQuadrant
> <alvherre@alvh.no-ip.org> 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-ishii@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 Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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 Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2019-Dec-26, Dave Cramer wrote:

> On Thu, 26 Dec 2019 at 15:07, Alvaro Herrera <alvherre@2ndquadrant.com>
> 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.1547504236@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 Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

From: Alvaro Herrera <alvherre@2ndquadrant.com>
> 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,
eventhe primary is read-only.  That's why the syntax target_session_attrs = {read-write | read-only} was introduced
insteadof 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
duringthe database server failover.  I think that will be convenient, but not mandatory for this patch.  PgJDBC doesn't
provideit, either.
 


Regards
Takayuki Tsunakawa

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

> From: Alvaro Herrera <alvherre@2ndquadrant.com>
> > 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 Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

From: Alvaro Herrera <alvherre@2ndquadrant.com>
> 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
documentread-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
SHOWtransaction_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
theserver 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_attra 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
thesame, 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'sa new feature effective for new servers.)
 


Regards
Takayuki Tsunakawa

On 2020-Jan-06, tsunakawa.takay@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
documentread-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
sendingSHOW 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
ifthe 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_attra 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 Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

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

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
david@pgmasters.net

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+rbFQ6F7oNzt9A@mail.gmail.com

Regards,
Greg Nancarrow
Fujitsu Australia

> On 18 May 2020, at 09:33, Greg Nancarrow <gregn4422@gmail.com> 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

> 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

> On 6 Jul 2020, at 14:19, Greg Nancarrow <gregn4422@gmail.com> 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

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

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)

+       <para>

The v17 patch for target_session_attrs and target_server_type help is currently using <para> blocks for each of the
possibleoption 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
confusingbecause 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)

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

Suggest change to:
--
If this parameter is set to <literal>standby</literal>, only a connection in which the server is in recovery mode is
consideredacceptable. The recovery mode state is determined by the value of the in_recovery configuration parameter
thatis reported by the server upon successful connection. Otherwise, if the server is prior to version 14, only a
connectionin which read-write transactions are not accepted by default is considered acceptable. To determine whether
theserver 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)

+       <varname>integer_datetimes</varname>,
+       <varname>standard_conforming_strings</varname> and
+       <varname>in_recovery</varname>.

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)

+    <varname>integer_datetimes</varname>,
+    <varname>standard_conforming_strings</varname> and
+    <varname>in_recovery</varname>.

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)

+    /* signal that work needs to be done */
+    recoveryExitInterruptPending = true;

Suggest change comment to say: 
/* flag that work needs to be done */

====

COMMENT fe-connect.c (sizeof)

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

According to the SGML "prefer-secondary" is also an acceptable value for target_session_attrs, so the display field
widthshould be 17 /* sizeof("prefer-secondary") */ not 15.
 

====

COMMENT fe-connect.c (CONNECTION_CHECK_RECOVERY)

@@ -2310,6 +2461,7 @@ PQconnectPoll(PGconn *conn)
         case CONNECTION_CHECK_WRITABLE:
         case CONNECTION_CONSUME:
         case CONNECTION_GSS_STARTUP:
+        case CONNECTION_CHECK_RECOVERY:
             break;

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

Probably this CONNECTION_CHECK_RECOVERY case should be removed.

====

COMMENT fe-connect.c - function validateAndRecordTargetServerType

As noted in GENERAL COMMENT 1, I suggest "any" needs to be included in this function as a valid option.

====

COMMENT fe-connect.c (target_session_attrs validation)

@@ -1396,8 +1425,9 @@ 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_server_type = SERVER_TYPE_ANY;
+        else if (!validateAndRecordTargetServerType(conn->target_session_attrs, &conn->requested_server_type))

I suggest introducing a 2nd function for target_session_attrs (e.g. validateAndRecordTargetSessionAttrs). 
Even though these parameters are functionally the same today, in future they may not be [1].
[1] - https://www.postgresql.org/message-id/20200109152539.GA29017%40alvherre.pgsql

Regardless, the special "any" handling can be removed from here because (from GENERAL COMMENT 1) the
validateAndRecordTargetServerTypeshould now accept "any".
 

====

COMMENT fe-connect.c (message typo)

Found an existing typo, unrelated to the v17 patch.

"target_settion_attrs", --> "target_session_attrs",

====

COMMENT fe-connect.c (libpq_gettext)

+            printfPQExpBuffer(&conn->errorMessage,
+                              libpq_gettext("invalid target_server_type value: \"%s\"\n"),
+                              conn->target_server_type);

The parameter name "target_server_type" should be separated from the format string as "%s", the same as is done by the
libpq_gettextof the preceding code.
 

====

COMMENT fe-connect.c (indentation)

+                goto error_return;
+            }
         }
+        else
         conn->whichhost++;

Bad indentation of the else's statement.

====

COMMENT fe-connect.c (if/else complexity)

+                    else if ((conn->in_recovery &&
+                          conn->requested_server_type == SERVER_TYPE_PRIMARY) ||
+                         (!conn->in_recovery &&
+                          (conn->requested_server_type == SERVER_TYPE_PREFER_STANDBY ||
+                           conn->requested_server_type == SERVER_TYPE_STANDBY)))
+                    {

TBH I was unable to read this code without first drawing up a matrix of combinations to deduce what was going on. 
It should not be so inscrutable.

Suggestion1:
Consider putting a large comment at the top of this CONNECTION_CHECK_TARGET to give the overview what this code is
tryingto acheive. 
 

e.g. something like this:
---
Mode           |in_recovery |version < 7.4      |version < 14               |version >= 14
---------------+------------+-------------------+---------------------------+-------------
ANY            |NA          |OK                 |OK                         |OK
PRIMARY        |true        |OK                 |SHOW transaction_read_only |keep_going
PRIMARY        |false       |OK                 |SHOW transaction_read_only |OK
PREFER_STANDBY |true        |keep_going (or -2)    |SHOW transaction_read_only    |OK
PREFER_STANDBY |false       |keep_going (or -2) |SHOW transaction_read_only    |keep_going (or -2)
STANDBY        |true        |keep_going         |SHOW transaction_read_only    |OK
STANDBY        |false       |keep_going         |SHOW transaction_read_only    |keep_going
---

Suggestion2:
Consider to separate out the requested_server_type cases instead of trying to hand everything in the same else block.
Thecode may be a bit longer, but by aligning it more closely with the SGML documentation it can be made easier to
understand.

e.g. something like this:
---
if (conn->requested_server_type == SERVER_TYPE_PRIMARY) {
    /* If not-in-recovery, reject, else OK. */
    if (conn->in_recovery) {
        rejectCheckedRecoveryConnection(conn);
        goto keep_going;
    }
    goto consume_checked_target_connection;
}
 
if (conn->requested_server_type == SERVER_TYPE_STANDBY) {
    /* Only a connection in recovery mode is acceptable. */
    if (!conn->in_recovery) {
        rejectCheckedRecoveryConnection(conn);
        goto keep_going;
    }
    goto consume_checked_target_connection;
}
 
if (conn->requested_server_type == SERVER_TYPE_PREFER_STANDBY) {
    /* A connection in recovery mode is preferred. */
    if (conn->in_recovery)
        goto consume_checked_target_connection;
 
    /*
     * The following scenario is possible only for the
     * prefer-standby type for the next pass of the list
     * of connections, as it couldn't find any servers that
     * are in recovery.
     */
    if (conn->which_rw_host == -2)
        goto consume_checked_target_connection;
 
    /* reject function below remembers this r/w host index in case it is needed later */
    rejectCheckedRecoveryConnection(conn);
    goto keep_going;
}
---

====

COMMENT fe-connect.c (case CONNECTION_CHECK_RECOVERY)

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

v17 patch has removed the previous call to pg_is_in_recovery.
IIUC this means that there is currently no way for the remaining CONNECTION_CHECK_RECOVERY case to even be executed.

If I am correct, then a significant slab of code (~100 lines) can be deleted.
See case CONNECTION_CHECK_RECOVERY (lines ~ 4007 thru 4110)

====

COMMENT fe-connect.c - function  freePGConn (missing free?)

There is code to free(conn->target_session_attrs), but there is no code to free target_server_type. 
Appears to be accidental omission.

====

COMMENT fe-exec.c (altered comment)

-     * Special hacks: remember client_encoding and
+     * Special hacks: remember client_encoding, and

A comma was added. 
Suggest avoid altering comments not directly related to the v17 patch logic.

====

COMMENT libpq-fe.h (CONNECTION_CHECK_RECOVERY)

+    CONNECTION_CHECK_TARGET,    /* Check if we have a proper target connection */
+    CONNECTION_CHECK_RECOVERY    /* Check whether server is in recovery */

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

Probably this CONNECTION_CHECK_RECOVERY case should be removed.

====

Kind Regards,
Peter Smith
---
Fujitsu Australia

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

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

On Fri, Dec 27, 2019 at 8:08 AM Alvaro Herrera <alvherre@2ndquadrant.com> 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

From: Robert Haas <robertmhaas@gmail.com>
> 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
replicateddatabase (ALTER USER SET default_transaction_read_only = on), it's the primary read-only server.
 


Regards
Takayuki Tsunakawa

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)
>
>+       <para>
>
>The v17 patch for target_session_attrs and target_server_type help is currently using <para> 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)
>
>+       <para>
>+        If this parameter is set to <literal>standby</literal>, only a connection in which
>+        the server is in recovery mode is considered acceptable. If the server is prior to version 14,
>+        the query <literal>SELECT pg_is_in_recovery()</literal> will be sent upon any successful
>+        connection; if it returns <literal>t</literal>, it means the server is in recovery mode.
>+       </para>
>
>Suggest change to:
>--
>If this parameter is set to <literal>standby</literal>, 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
thatis reported by >the server upon successful connection. Otherwise, if the server is prior to version 14, only a
connectionin which read-write >transactions are not accepted by default is considered acceptable. To determine whether
theserver 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. 
>--

GN RESPONSE: I've removed the residual references to
pg_is_in_recovery, and updated the documentation in a similar way.

====

>COMMENT libpq.sgml (Oxford comma)
>
>+       <varname>integer_datetimes</varname>,
>+       <varname>standard_conforming_strings</varname> and
>+       <varname>in_recovery</varname>.
>
>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.

GN RESPONSE: I have restored the Oxford comma to its former glory.

====

>COMMENT protocol.sgml (Oxford comma)
>
>+    <varname>integer_datetimes</varname>,
>+    <varname>standard_conforming_strings</varname> and
>+    <varname>in_recovery</varname>.
>
>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.

GN RESPONSE: I have restored the Oxford comma to its former glory.

====

>QUESTION standby.c - SendRecoveryExitSignal

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

GN RESPONSE: It's not much of a layer. It could be argued that having
a common function for this makes sense, in case additional code needs
to be added (so it's then not repeated/missed in places).


====

COMMENT postgres.c (signal comment)

>+       /* signal that work needs to be done */
>+       recoveryExitInterruptPending = true;
>
>Suggest change comment to say:
>/* flag that work needs to be done */

GN RESPONSE: Agree, have updated the patch.

====

>COMMENT fe-connect.c (sizeof)
>
>-               "Target-Session-Attrs", "", 11, /* sizeof("read-write") = 11 */
>+               "Target-Session-Attrs", "", 15, /* sizeof("prefer-standby") = 15 */
>
>According to the SGML "prefer-secondary" is also an acceptable value for target_session_attrs, so the display field
width>should be 17 /* sizeof("prefer-secondary") */ not 15. 

GN RESPONSE: I'm not sure about this, it's debatable. The intention of
these settings is to provide information for a "generic database
connection dialog". For "Target-Session-Attrs" I'd probably expect the
dialog to list the option "prefer-standby" rather than the
(PGJDBC-compatible) synonym "prefer-secondary" (whose length of 17 was
used in the case of "Target-Server-Type").

====

>COMMENT fe-connect.c (CONNECTION_CHECK_RECOVERY)
>
>@@ -2310,6 +2461,7 @@ PQconnectPoll(PGconn *conn)
>                case CONNECTION_CHECK_WRITABLE:
>                case CONNECTION_CONSUME:
>                case CONNECTION_GSS_STARTUP:
>+               case CONNECTION_CHECK_RECOVERY:
>                        break;
>
>(As noted in GENERAL COMMENT 2 there are still residual references to pg_is_in_recovery)
>
>Probably this CONNECTION_CHECK_RECOVERY case should be removed.

GN RESPONSE: Agree, removed because it is no longer used.

====

>COMMENT fe-connect.c - function validateAndRecordTargetServerType
>
>As noted in GENERAL COMMENT 1, I suggest "any" needs to be included in this function as a valid option.

GN RESPONSE: Agree, updated patch.

====

>COMMENT fe-connect.c (target_session_attrs validation)
>
>@@ -1396,8 +1425,9 @@ 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_server_type = SERVER_TYPE_ANY;
>+               else if (!validateAndRecordTargetServerType(conn->target_session_attrs, &conn->requested_server_type))
>
>I suggest introducing a 2nd function for target_session_attrs (e.g. validateAndRecordTargetSessionAttrs).
>Even though these parameters are functionally the same today, in future they may not be [1].
>[1] - https://www.postgresql.org/message-id/20200109152539.GA29017%40alvherre.pgsql
>
>Regardless, the special "any" handling can be removed from here because (from GENERAL COMMENT 1) the
>validateAndRecordTargetServerTypeshould now accept "any". 

GN RESPONSE: Agree, have added separate validation functions.

====

>COMMENT fe-connect.c (message typo)
>
>Found an existing typo, unrelated to the v17 patch.
>
>"target_settion_attrs", --> "target_session_attrs",

GN RESPONSE: Have updated the patch to correct that.

====

>COMMENT fe-connect.c (libpq_gettext)
>
>+                       printfPQExpBuffer(&conn->errorMessage,
>+                                                         libpq_gettext("invalid target_server_type value: \"%s\"\n"),
>+                                                         conn->target_server_type);
>
>The parameter name "target_server_type" should be separated from the format string as "%s", the same as is done by the
>libpq_gettextof the preceding code. 

GN RESPONSE: Agree, was not correct in the v17 patch, have updated the patch.

====

>COMMENT fe-connect.c (indentation)
>
>+                               goto error_return;
>+                       }
>                }
>+               else
>                conn->whichhost++;
>
>Bad indentation of the else's statement.

GN RESPONSE: Updated the patch to fix that.

====

>COMMENT fe-connect.c (if/else complexity)
>
>+                                       else if ((conn->in_recovery &&
>+                                                 conn->requested_server_type == SERVER_TYPE_PRIMARY) ||
>+                                                (!conn->in_recovery &&
>+                                                 (conn->requested_server_type == SERVER_TYPE_PREFER_STANDBY ||
>+                                                  conn->requested_server_type == SERVER_TYPE_STANDBY)))
>+                                       {
>
>TBH I was unable to read this code without first drawing up a matrix of combinations to deduce what was going on.
>It should not be so inscrutable.
>
>Suggestion1:
>Consider putting a large comment at the top of this CONNECTION_CHECK_TARGET to give the overview what this code is
tryingto >acheive. 
>...
>
>Suggestion2:
>Consider to separate out the requested_server_type cases instead of trying to hand everything in the same else block.
Thecode >may be a bit longer, but by aligning it more closely with the SGML documentation it can be made easier to
understand.

GN RESPONSE: Some slight restructuring has been made and comments
updated to express the logic in words, to assist in understanding.
(It's actually not that bad, but maybe I've been looking at this for too long).

====

>COMMENT fe-connect.c (case CONNECTION_CHECK_RECOVERY)
>
>(As noted in GENERAL COMMENT 2 there are still residual references to pg_is_in_recovery)
>
>v17 patch has removed the previous call to pg_is_in_recovery.
>IIUC this means that there is currently no way for the remaining CONNECTION_CHECK_RECOVERY case to even be executed.
>
>If I am correct, then a significant slab of code (~100 lines) can be deleted.
>See case CONNECTION_CHECK_RECOVERY (lines ~ 4007 thru 4110)

GN RESPONSE: Agree, have removed code that is no longer called.

====

>COMMENT fe-connect.c - function  freePGConn (missing free?)
>
>There is code to free(conn->target_session_attrs), but there is no code to free target_server_type.
>Appears to be accidental omission.

GN RESPONSE: Have added missing free(), oops.

====

>COMMENT fe-exec.c (altered comment)
>
>-        * Special hacks: remember client_encoding and
>+        * Special hacks: remember client_encoding, and
>
>A comma was added.
>Suggest avoid altering comments not directly related to the v17 patch logic.

GN RESPONSE: Have removed the (Oxford!) comma, accidently added.

====

>COMMENT libpq-fe.h (CONNECTION_CHECK_RECOVERY)
>
>+       CONNECTION_CHECK_TARGET,        /* Check if we have a proper target connection */
>+       CONNECTION_CHECK_RECOVERY       /* Check whether server is in recovery */
>
>(As noted in GENERAL COMMENT 2 there are still residual references to pg_is_in_recovery)
>
>Probably this CONNECTION_CHECK_RECOVERY case should be removed.

GN RESPONSE: Have removed, no longer used.


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

GN RESPONSE: Have added "any" tests for target_server_type.

====

>COMMENT - negative tests?
>
>IIUC when "standby" (aka "secondary") is specified, and there is no in_recovery server available, then the result
shouldbe 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

GN RESPONSE: No such negative tests existed for target_session_attrs
prior to this patch.
I have added some negative tests for both target_session_attrs and
target_server_type.
Note that in the v18 patch, "standby" and "read-write" are no longer
allowed for "target_server_type" (since not PGJDBC driver compatible).
Also, "read-write" is no longer considered a synonym for "primary" -
"read-write" means writeable (non read-only) and "primary" means not
in recovery.
Tests were adjusted accordingly.


Regards,
Greg Nancarrow
Fujitsu Australia

On Thu, Aug 20, 2020 at 10:26 AM Greg Nancarrow <gregn4422@gmail.com> 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:

+       <para>
+        If this parameter is set to <literal>read-write</literal>,
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
+        <varname>transaction_read_only</varname> configuration
parameter that is reported by
+        the server upon successful connection. Otherwise if the
server is prior to version 14,
+        the query <literal>SHOW transaction_read_only</literal> will
be sent upon any successful
+        connection; if it returns <literal>on</literal>, it means the
server doesn't support
+        read-write transactions.
+       </para>

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

====

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"

====

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.

====

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?

====

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.

====

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"

====

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 hosts before
+ * considering it later. If the requested type of connection
+ * is read-only or standby, ignore this connection.
+ */
+
+ if (conn->requested_server_type == SERVER_TYPE_PREFER_STANDBY ||
+ conn->requested_server_type == SERVER_TYPE_READ_ONLY ||
+ conn->requested_server_type == SERVER_TYPE_STANDBY)
+ {

IIUC the only way to reach this code (because of all the previous
gotos) is when the server version is < 7.4.

So to make this more readable that "if" should ideally be "else if"
because the prior if block already says
+ if (conn->sversion >= 70400)

====

COMMENT (fe-connect - conn->sversion < 140000)

+ if (conn->sversion < 140000)
+ {
+ /*
+ * Save existing error messages across the
+ * PQsendQuery attempt.  This is necessary because
+ * PQsendQuery is going to reset
+ * conn->errorMessage, so we would lose error
+ * messages related to previous hosts we have
+ * tried and failed to connect to.
+ */
+ if (!saveErrorMessage(conn, &savedMessage))
+ goto error_return;
+
+ conn->status = CONNECTION_OK;
+ if (!PQsendQuery(conn, "SHOW transaction_read_only"))
+ {
+ restoreErrorMessage(conn, &savedMessage);
+ goto error_return;
+ }
+ conn->status = CONNECTION_CHECK_WRITABLE;
+ restoreErrorMessage(conn, &savedMessage);
+ return PGRES_POLLING_READING;
+ }

I am suspicious of the duplicate code blocks for (conn->sversion < 140000).

Both appear to be doing exactly the same thing for all requests types
(excluding "any") so IMO these can be refactored into a single if
which is just beneath the check for (conn->sversion >= 70400). The
result can remove 25 lines and also be easier to read.

====

COMMENT (fe-connect.c - if comment)

+ else if ((conn->in_recovery &&
+   conn->requested_server_type == SERVER_TYPE_PRIMARY) ||
+ (!conn->in_recovery &&
+   (conn->requested_server_type == SERVER_TYPE_PREFER_STANDBY ||
+    conn->requested_server_type == SERVER_TYPE_STANDBY)))
+ {
+ /*
+ * Server is in recovery but requested primary, or
+ * server is not in recovery but requested
+ * prefer-standby/standby.
+ */

This comment does not have much value because it reads almost exactly
the same as the code it is describing.
Maybe it can be reworded to be more useful, or if not, just remove it?

====

COMMENT (fe-connect.c - CHECK_WRITABLE wrong goto?)

+ if ((readonly_server &&
+ (conn->requested_server_type == SERVER_TYPE_READ_WRITE ||
+   conn->requested_server_type == SERVER_TYPE_PRIMARY)) ||
+ (!readonly_server &&
+ (conn->requested_server_type == SERVER_TYPE_PREFER_STANDBY ||
+   conn->requested_server_type == SERVER_TYPE_READ_ONLY ||
+   conn->requested_server_type == SERVER_TYPE_STANDBY)))
  {
- /* Not writable; fail this connection. */
+ if (conn->which_primary_or_rw_host == -2)
+ {
+ /*
+ * This scenario is possible only for the
+ * prefer-standby type for the next pass of the
+ * list of connections as it couldn't find any
+ * servers that are read-only.
+ */
+ goto consume_checked_target_connection;
+ }

Is this goto consume_checked_target_connection deliberate?
Previously (in the v17 patch) there was a another label, and so this
same code did goto consume_checked_write_connection;

The v17 code seems more correct than the current v18 code, which is
now jumping to a label not even in the same case block!

====

Kind Regards,
Peter Smith.
Fujitsu Australia

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 <smithpb2250@gmail.com> 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 hosts before
> + * considering it later. If the requested type of connection
> + * is read-only or standby, ignore this connection.
> + */
> +
> + if (conn->requested_server_type == SERVER_TYPE_PREFER_STANDBY ||
> + conn->requested_server_type == SERVER_TYPE_READ_ONLY ||
> + conn->requested_server_type == SERVER_TYPE_STANDBY)
> + {
>
> IIUC the only way to reach this code (because of all the previous
> gotos) is when the server version is < 7.4.
>
> So to make this more readable that "if" should ideally be "else if"
> because the prior if block already says
> + if (conn->sversion >= 70400)
>

GN RESPONSE: Changed to "else if".

>
> COMMENT (fe-connect - conn->sversion < 140000)
>
> ...
>
> I am suspicious of the duplicate code blocks for (conn->sversion < 140000).
>
> Both appear to be doing exactly the same thing for all requests types
> (excluding "any") so IMO these can be refactored into a single if
> which is just beneath the check for (conn->sversion >= 70400). The
> result can remove 25 lines and also be easier to read.
>

GN RESPONSE:
I was able to refactor the code to make it a bit simpler and remove
the duplicate code block, after first adding a condition to exclude
"any".

>
> COMMENT (fe-connect.c - if comment)
>
> + else if ((conn->in_recovery &&
> +   conn->requested_server_type == SERVER_TYPE_PRIMARY) ||
> + (!conn->in_recovery &&
> +   (conn->requested_server_type == SERVER_TYPE_PREFER_STANDBY ||
> +    conn->requested_server_type == SERVER_TYPE_STANDBY)))
> + {
> + /*
> + * Server is in recovery but requested primary, or
> + * server is not in recovery but requested
> + * prefer-standby/standby.
> + */
>
> This comment does not have much value because it reads almost exactly
> the same as the code it is describing.
> Maybe it can be reworded to be more useful, or if not, just remove it?
>

GN RESPONSE: I've enhanced the comment.

>
> COMMENT (fe-connect.c - CHECK_WRITABLE wrong goto?)
>
> + if ((readonly_server &&
> + (conn->requested_server_type == SERVER_TYPE_READ_WRITE ||
> +   conn->requested_server_type == SERVER_TYPE_PRIMARY)) ||
> + (!readonly_server &&
> + (conn->requested_server_type == SERVER_TYPE_PREFER_STANDBY ||
> +   conn->requested_server_type == SERVER_TYPE_READ_ONLY ||
> +   conn->requested_server_type == SERVER_TYPE_STANDBY)))
>   {
> - /* Not writable; fail this connection. */
> + if (conn->which_primary_or_rw_host == -2)
> + {
> + /*
> + * This scenario is possible only for the
> + * prefer-standby type for the next pass of the
> + * list of connections as it couldn't find any
> + * servers that are read-only.
> + */
> + goto consume_checked_target_connection;
> + }
>
> Is this goto consume_checked_target_connection deliberate?
> Previously (in the v17 patch) there was a another label, and so this
> same code did goto consume_checked_write_connection;
>
> The v17 code seems more correct than the current v18 code, which is
> now jumping to a label not even in the same case block!
>

GN RESPONSE:
Not deliberate, seems to have been messed up (possibly by copying
another block, to get a comment), but has now been corrected.


Regards,
Greg Nancarrow
Fujitsu Australia

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

Greg Nancarrow <gregn4422@gmail.com> 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
         },
         &XactReadOnly,
         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

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 */


@@ -5828,7 +5830,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 +6220,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,26 +6265,64 @@ BeginReportingGUCOptions(void)
         if (conf->flags & GUC_REPORT)
             ReportGUCOption(conf);
     }
+
+    report_needed = false;
 }

 /*
- * ReportGUCOption: if appropriate, transmit option value to frontend
+ * 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.  We do not, however, go to
+ * the length of trying to suppress sending anything when the variable was
+ * changed and then reverted to its original value.
+ */
+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: transmit option value to frontend
+ *
+ * Caller is now fully responsible for deciding whether this should be done.
+ * However, we do clear the NEEDS_REPORT flag here.
  */
 static void
 ReportGUCOption(struct config_generic *record)
 {
-    if (reporting_enabled && (record->flags & GUC_REPORT))
-    {
-        char       *val = _ShowOption(record, false);
-        StringInfoData msgbuf;
+    char       *val = _ShowOption(record, false);
+    StringInfoData msgbuf;

-        pq_beginmessage(&msgbuf, 'S');
-        pq_sendstring(&msgbuf, record->name);
-        pq_sendstring(&msgbuf, val);
-        pq_endmessage(&msgbuf);
+    pq_beginmessage(&msgbuf, 'S');
+    pq_sendstring(&msgbuf, record->name);
+    pq_sendstring(&msgbuf, val);
+    pq_endmessage(&msgbuf);

-        pfree(val);
-    }
+    pfree(val);
+
+    record->status &= ~GUC_NEEDS_REPORT;
 }

 /*
@@ -7667,7 +7713,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 2819282181..76236fb0c0 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -362,6 +362,7 @@ extern void AtStart_GUC(void);
 extern int    NewGUCNestLevel(void);
 extern void AtEOXact_GUC(bool isCommit, int nestLevel);
 extern void BeginReportingGUCOptions(void);
+extern void ReportChangedGUCOptions(void);
 extern void ParseLongOption(const char *string, char **name, char **value);
 extern bool parse_int(const char *value, int *result, int flags,
                       const char **hintmsg);
diff --git a/src/include/utils/guc_tables.h b/src/include/utils/guc_tables.h
index 04431d0eb2..a29c2b01b4 100644
--- a/src/include/utils/guc_tables.h
+++ b/src/include/utils/guc_tables.h
@@ -172,7 +172,8 @@ struct config_generic
  * Caution: the GUC_IS_IN_FILE bit is transient state for ProcessConfigFile.
  * Do not assume that its value represents useful information elsewhere.
  */
-#define GUC_PENDING_RESTART 0x0002
+#define GUC_PENDING_RESTART 0x0002    /* changed value cannot be applied yet */
+#define GUC_NEEDS_REPORT    0x0004    /* new value must be reported to client */


 /* GUC records for specific variable types */
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index ddfc7ea05d..207dc9bf4d 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -209,6 +209,7 @@ static bool check_cluster_name(char **newval, void **extra, GucSource source);
 static const char *show_unix_socket_permissions(void);
 static const char *show_log_file_mode(void);
 static const char *show_data_directory_mode(void);
+static const char *show_in_hot_standby(void);
 static bool check_backtrace_functions(char **newval, void **extra, GucSource source);
 static void assign_backtrace_functions(const char *newval, void *extra);
 static bool check_recovery_target_timeline(char **newval, void **extra, GucSource source);
@@ -607,6 +608,8 @@ static int    max_identifier_length;
 static int    block_size;
 static int    segment_size;
 static int    wal_block_size;
+static bool in_hot_standby;
+static bool last_reported_in_hot_standby;
 static bool data_checksums;
 static bool integer_datetimes;
 static bool assert_enabled;
@@ -1844,6 +1847,17 @@ static struct config_bool ConfigureNamesBool[] =
         NULL, NULL, NULL
     },

+    {
+        {"in_hot_standby", PGC_INTERNAL, PRESET_OPTIONS,
+            gettext_noop("Shows whether hot standby is currently active."),
+            NULL,
+            GUC_REPORT | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
+        },
+        &in_hot_standby,
+        false,
+        NULL, NULL, show_in_hot_standby
+    },
+
     {
         {"allow_system_table_mods", PGC_SUSET, DEVELOPER_OPTIONS,
             gettext_noop("Allows modifications of the structure of system tables."),
@@ -6266,6 +6280,9 @@ BeginReportingGUCOptions(void)
             ReportGUCOption(conf);
     }

+    /* Hack for in_hot_standby: remember the value we just sent */
+    last_reported_in_hot_standby = in_hot_standby;
+
     report_needed = false;
 }

@@ -6287,6 +6304,23 @@ ReportChangedGUCOptions(void)
     if (!reporting_enabled)
         return;

+    /*
+     * Since in_hot_standby isn't actually changed by normal GUC actions, we
+     * need a hack to check whether a new value needs to be reported to the
+     * client.  For speed, we rely on the assumption that it can never
+     * transition from false to true.
+     */
+    if (last_reported_in_hot_standby && !RecoveryInProgress())
+    {
+        struct config_generic *record;
+
+        record = find_option("in_hot_standby", false, ERROR);
+        Assert(record != NULL);
+        record->status |= GUC_NEEDS_REPORT;
+        report_needed = true;
+        last_reported_in_hot_standby = false;
+    }
+
     /* Quick exit if no values have been changed */
     if (!report_needed)
         return;
@@ -11738,6 +11772,18 @@ show_data_directory_mode(void)
     return buf;
 }

+static const char *
+show_in_hot_standby(void)
+{
+    /*
+     * Unlike most show hooks, this has a side-effect of updating the dummy
+     * GUC variable to contain the value last shown.  See confederate code in
+     * BeginReportingGUCOptions and ReportChangedGUCOptions.
+     */
+    in_hot_standby = RecoveryInProgress();
+    return in_hot_standby ? "on" : "off";
+}
+
 /*
  * We split the input string, where commas separate function names
  * and certain whitespace chars are ignored, into a \0-separated (and

On Sun, Sep 27, 2020 at 4:34 AM Tom Lane <tgl@sss.pgh.pa.us> 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

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

Anastasia Lubennikova <a.lubennikova@postgrespro.ru> 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

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.

Alvaro Herrera <alvherre@alvh.no-ip.org> 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

I wrote:
> Alvaro Herrera <alvherre@alvh.no-ip.org> 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(&msgbuf, 'S');
@@ -6275,8 +6327,19 @@ ReportGUCOption(struct config_generic *record)
         pq_sendstring(&msgbuf, val);
         pq_endmessage(&msgbuf);

-        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
+++ b/src/include/utils/guc.h
@@ -363,6 +363,7 @@ extern void AtStart_GUC(void);
 extern int    NewGUCNestLevel(void);
 extern void AtEOXact_GUC(bool isCommit, int nestLevel);
 extern void BeginReportingGUCOptions(void);
+extern void ReportChangedGUCOptions(void);
 extern void ParseLongOption(const char *string, char **name, char **value);
 extern bool parse_int(const char *value, int *result, int flags,
                       const char **hintmsg);
diff --git a/src/include/utils/guc_tables.h b/src/include/utils/guc_tables.h
index 04431d0eb2..7f36e1146f 100644
--- a/src/include/utils/guc_tables.h
+++ b/src/include/utils/guc_tables.h
@@ -161,6 +161,8 @@ struct config_generic
     GucContext    reset_scontext; /* context that set the reset value */
     GucStack   *stack;            /* stacked prior values */
     void       *extra;            /* "extra" pointer for current actual value */
+    char       *last_reported;    /* if variable is GUC_REPORT, value last sent
+                                 * to client (NULL if not yet sent) */
     char       *sourcefile;        /* file current setting is from (NULL if not
                                  * set in config file) */
     int            sourceline;        /* line in source file */
@@ -172,7 +174,8 @@ struct config_generic
  * Caution: the GUC_IS_IN_FILE bit is transient state for ProcessConfigFile.
  * Do not assume that its value represents useful information elsewhere.
  */
-#define GUC_PENDING_RESTART 0x0002
+#define GUC_PENDING_RESTART 0x0002    /* changed value cannot be applied yet */
+#define GUC_NEEDS_REPORT    0x0004    /* new value must be reported to client */


 /* GUC records for specific variable types */

On Wed, Nov 25, 2020 at 12:07 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
>
> Here's a v2 that does it like that.
>

Looks OK to me.



Regards,
Greg Nancarrow
Fujitsu Australia

Greg Nancarrow <gregn4422@gmail.com> writes:
> On Wed, Nov 25, 2020 at 12:07 PM Tom Lane <tgl@sss.pgh.pa.us> 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

On Thu, Nov 26, 2020 at 3:43 AM Tom Lane <tgl@sss.pgh.pa.us> 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

On Thu, Nov 26, 2020 at 11:07 AM Greg Nancarrow <gregn4422@gmail.com> wrote:
>
> On Thu, Nov 26, 2020 at 3:43 AM Tom Lane <tgl@sss.pgh.pa.us> 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

Greg Nancarrow <gregn4422@gmail.com> 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

On Wed, Jan 6, 2021 at 3:05 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Greg Nancarrow <gregn4422@gmail.com> 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

On Mon, Feb 8, 2021 at 8:17 PM vignesh C <vignesh21@gmail.com> 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:

       <para>
        The support of read-write transactions is determined by the value of the
        <varname>default_transaction_read_only</varname> and
        <varname>in_hot_standby</varname> configuration parameters,
that, if supported,
        are reported by the server upon successful connection. If the
value of either
        of these parameters is <literal>on</literal>, 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 <literal>SHOW transaction_read_only</literal> after
successful
        connection; if it returns <literal>on</literal>, it means the
server doesn't
        support read-write transactions. The standby mode state is
determined by either
        the value of the <varname>in_hot_standby</varname> configuration
        parameter, that is reported by the server (if supported) upon
        successful connection, or is otherwise explicitly queried by sending
        <literal>SELECT pg_is_in_recovery()</literal> after successful
        connection; if it returns <literal>t</literal>, it means the server is
        in hot standby mode.
       </para>


Regards,
Greg Nancarrow
Fujitsu Australia

On Mon, Feb 8, 2021 at 8:17 PM vignesh C <vignesh21@gmail.com> 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

       <varname>default_transaction_read_only</varname>  and
       <varname>in_hot_standby</varname> were not reported by releases before
       14.)

should be:

       <varname>default_transaction_read_only</varname>  and
       <varname>in_hot_standby</varname> were not reported by releases before
       14.0)

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

   <para>
    During hot standby, the parameter <varname>in_hot_standby</varname> and
    <varname>default_transaction_read_only</varname> are always true and may
    not be changed.

should be:

   <para>
    During hot standby, the parameters <varname>in_hot_standby</varname> and
    <varname>transaction_read_only</varname> 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

On Tue, Feb 9, 2021 at 5:47 AM Greg Nancarrow <gregn4422@gmail.com> wrote:
>
> On Mon, Feb 8, 2021 at 8:17 PM vignesh C <vignesh21@gmail.com> 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:
>
>        <para>
>         The support of read-write transactions is determined by the value of the
>         <varname>default_transaction_read_only</varname> and
>         <varname>in_hot_standby</varname> configuration parameters,
> that, if supported,
>         are reported by the server upon successful connection. If the
> value of either
>         of these parameters is <literal>on</literal>, 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 <literal>SHOW transaction_read_only</literal> after
> successful
>         connection; if it returns <literal>on</literal>, it means the
> server doesn't
>         support read-write transactions. The standby mode state is
> determined by either
>         the value of the <varname>in_hot_standby</varname> configuration
>         parameter, that is reported by the server (if supported) upon
>         successful connection, or is otherwise explicitly queried by sending
>         <literal>SELECT pg_is_in_recovery()</literal> after successful
>         connection; if it returns <literal>t</literal>, it means the server is
>         in hot standby mode.
>        </para>

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

Regards,
Vignesh

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

On Tue, Feb 9, 2021 at 2:27 PM Greg Nancarrow <gregn4422@gmail.com> wrote:
>
> On Mon, Feb 8, 2021 at 8:17 PM vignesh C <vignesh21@gmail.com> 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
>
>        <varname>default_transaction_read_only</varname>  and
>        <varname>in_hot_standby</varname> were not reported by releases before
>        14.)
>
> should be:
>
>        <varname>default_transaction_read_only</varname>  and
>        <varname>in_hot_standby</varname> were not reported by releases before
>        14.0)
>

Modified.

> 2) doc/src/sgml/high-availability,sgml
>
>    <para>
>     During hot standby, the parameter <varname>in_hot_standby</varname> and
>     <varname>default_transaction_read_only</varname> are always true and may
>     not be changed.
>
> should be:
>
>    <para>
>     During hot standby, the parameters <varname>in_hot_standby</varname> and
>     <varname>transaction_read_only</varname> 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

On Wed, Feb 10, 2021 at 5:09 PM vignesh C <vignesh21@gmail.com> 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
+        <varname>default_transaction_read_only</varname> and
+        <varname>in_hot_standby</varname> 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
+        <varname>default_transaction_read_only</varname> and
+        <varname>in_hot_standby</varname> 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

On Fri, Feb 12, 2021 at 7:07 AM Greg Nancarrow <gregn4422@gmail.com> wrote:
>
> On Wed, Feb 10, 2021 at 5:09 PM vignesh C <vignesh21@gmail.com> 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
> +        <varname>default_transaction_read_only</varname> and
> +        <varname>in_hot_standby</varname> 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
> +        <varname>default_transaction_read_only</varname> and
> +        <varname>in_hot_standby</varname> 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

On Fri, Feb 12, 2021 at 2:42 PM vignesh C <vignesh21@gmail.com> 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
> > +        <varname>default_transaction_read_only</varname> and
> > +        <varname>in_hot_standby</varname> 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
> > +        <varname>default_transaction_read_only</varname> and
> > +        <varname>in_hot_standby</varname> 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