Horton

Thread

Commits

  1. Make test_target_session_attrs more robust against connection failure.

  2. Extend the abilities of libpq's target_session_attrs parameter.

  3. Mark default_transaction_read_only as GUC_REPORT.

  4. Introduce a new GUC_REPORT setting "in_hot_standby".

  5. Avoid spamming the client with multiple ParameterStatus messages.

  6. Avoid harmless Valgrind no-buffer-pin errors.

  7. Restructure libpq code to remove some duplicity

  8. libpq: Add target_session_attrs parameter.

  1. Re: Libpq support to connect to standby server as priority

    Jing Wang <jingwangian@gmail.com> — 2018-01-23T22:00:03Z

    Hi All,

Recently I put a proposal to support 'prefer-read' parameter in
target_session_attrs in libpq. Now I updated the patch with 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 looks similar with this.
But I would say this patch is more complex than my proposal.

It is better separate these 2 patches to consider.

Regards,
Jing Wang
Fujitsu Australia

  2. Re: Libpq support to connect to standby server as priority

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2018-06-19T03:19:29Z

    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 patch with 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 looks similar 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.

Regards,
Haribabu Kommi
Fujitsu Australia

  3. Re: Libpq support to connect to standby server as priority

    Laurenz Albe <laurenz.albe@cybertec.at> — 2018-07-04T13:14:38Z

    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 patch with 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 looks similar 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

  4. Re: Libpq support to connect to standby server as priority

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2018-07-11T08:00:37Z

    On Wed, Jul 4, 2018 at 11:14 PM Laurenz Albe <laurenz.albe@cybertec.at>
wrote:

> 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 patch with 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 looks similar 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.
>

Thanks for the review.


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

There was a problem in reusing the primary host index and it leads to loop.
Attached patch fixed the issue.


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

updated as per you comment.


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

Regards,
Haribabu Kommi
Fujitsu Australia

  5. Re: Libpq support to connect to standby server as priority

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2018-07-16T08:14:57Z

    On Wed, Jul 11, 2018 at 6:00 PM Haribabu Kommi <kommi.haribabu@gmail.com>
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?

Regards,
Haribabu Kommi
Fujitsu Australia

  6. Re: Libpq support to connect to standby server as priority

    Laurenz Albe <laurenz.albe@cybertec.at> — 2018-07-16T14:42:23Z

    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

  7. Re: Libpq support to connect to standby server as priority

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2018-07-17T12:15:12Z

    On Tue, Jul 17, 2018 at 12:42 AM Laurenz Albe <laurenz.albe@cybertec.at>
wrote:

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

Thanks for the confirmation.


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

Thanks for your opinion, let's wait for opinion from others also.
I can go for the modification, if others also find it useful.

Regards,
Haribabu Kommi
Fujitsu Australia

  8. Re: Libpq support to connect to standby server as priority

    Robert Haas <robertmhaas@gmail.com> — 2018-07-18T12:53:42Z

    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

  9. Re: Libpq support to connect to standby server as priority

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2018-07-19T12:59:28Z

    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.

Regards,
Haribabu Kommi
Fujitsu Australia

  10. Re: Libpq support to connect to standby server as priority

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2018-09-28T07:31:47Z

    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.

Regards,
Haribabu Kommi
Fujitsu Australia

  11. Re: Libpq support to connect to standby server as priority

    Laurenz Albe <laurenz.albe@cybertec.at> — 2018-10-05T14:34:34Z

    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

  12. Re: Libpq support to connect to standby server as priority

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2018-11-09T11:48:02Z

    On Fri, Sep 28, 2018 at 5:31 PM Haribabu Kommi <kommi.haribabu@gmail.com>
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.
>

[some how i didn't receive this mail, copy pasted from mailing list ]

>The code is much more readable now, thanks.

Thanks for the review.

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

Added comments along the lines that you mentioned. And also try
to update some more comments.


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

Yes, I didn't add that logic of keeping the first connection open, Currently
I feel that adds more complexity in supporting the same. If everyone feels
that is required, I will add that logic.

Updated patch attached.

Regards,
Haribabu Kommi
Fujitsu Australia

  13. Re: Libpq support to connect to standby server as priority

    Laurenz Albe <laurenz.albe@cybertec.at> — 2018-11-09T11:53:01Z

    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

  14. Re: Libpq support to connect to standby server as priority

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-11-12T18:23:32Z

    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

  15. Re: Libpq support to connect to standby server as priority

    Laurenz Albe <laurenz.albe@cybertec.at> — 2018-11-12T20:26:53Z

    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

  16. Re: Libpq support to connect to standby server as priority

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2018-11-13T06:54:17Z

    On Tue, Nov 13, 2018 at 7:26 AM Laurenz Albe <laurenz.albe@cybertec.at>
wrote:

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

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.


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

Thanks for the link. Based on the conclusion on the other thread of
GUC_REPORT,
this patch also can use that logic, but that is limited only till the
connection establishment
for these connection types.

Regards,
Haribabu Kommi
Fujitsu Australia

  17. Re: Libpq support to connect to standby server as priority

    Michael Paquier <michael@paquier.xyz> — 2018-11-15T08:14:33Z

    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

  18. Re: Libpq support to connect to standby server as priority

    Michael Paquier <michael@paquier.xyz> — 2018-11-15T09:13:51Z

    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

  19. Re: Libpq support to connect to standby server as priority

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-11-16T16:35:47Z

    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

  20. Re: Libpq support to connect to standby server as priority

    Laurenz Albe <laurenz.albe@cybertec.at> — 2018-11-16T17:56:11Z

    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

  21. Re: Libpq support to connect to standby server as priority

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2018-11-16T23:41:54Z

    On Sat, Nov 17, 2018 at 4:56 AM Laurenz Albe <laurenz.albe@cybertec.at>
wrote:

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

Yes, we need either session open or reconnect it approach to find out
the whether server is read-write or read-only.

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?

Regards,
Haribabu Kommi
Fujitsu Australia

  22. Re: Libpq support to connect to standby server as priority

    Michael Paquier <michael@paquier.xyz> — 2018-11-17T00:14:09Z

    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

  23. Re: Libpq support to connect to standby server as priority

    Vladimir Sitnikov <sitnikov.vladimir@gmail.com> — 2018-11-20T11:23:01Z

    Tom>Yes, we need either session open or reconnect it approach to find out
Tom>the whether server is read-write or read-only.

Just in case, pgjdbc has that feature for quite a while, and the behavior
there is to keep the connection until it fails or application decides to
close it.

pgjdbc uses three parameters (since 2014):
1) targetServerType=(any | master | secondary | preferSecondary). Default
is "any". When set to "master" it will look for "read-write" server. If set
to "preferSecondary" it would search for "read-only" server first, then
fall back to master, and so on.
2) loadBalanceHosts=(true | false). pgjdbc enables to load-balance across
servers provided in the connection URL. When set to "false", pgjdbc tries
connections in order, otherwise it shuffles the connections.
3) hostRecheckSeconds=int. pgjdbc caches "read/write" status of a host:port
combination, so it don't re-check the status if multiple connections are
created within hostRecheckSeconds timeframe.

It is sad that essentially the same feature is re-implemented in core with
different name/semantics.
Does it make sense to align parameter names/semantics?

Vladimir

  24. Re: Libpq support to connect to standby server as priority

    Dave Cramer <pg@fastcrypt.com> — 2018-11-20T11:45:58Z

    On Tue, 20 Nov 2018 at 06:23, Vladimir Sitnikov <sitnikov.vladimir@gmail.com>
wrote:

> Tom>Yes, we need either session open or reconnect it approach to find out
> Tom>the whether server is read-write or read-only.
>
> Just in case, pgjdbc has that feature for quite a while, and the behavior
> there is to keep the connection until it fails or application decides to
> close it.
>
> pgjdbc uses three parameters (since 2014):
> 1) targetServerType=(any | master | secondary | preferSecondary). Default
> is "any". When set to "master" it will look for "read-write" server. If set
> to "preferSecondary" it would search for "read-only" server first, then
> fall back to master, and so on.
> 2) loadBalanceHosts=(true | false). pgjdbc enables to load-balance across
> servers provided in the connection URL. When set to "false", pgjdbc tries
> connections in order, otherwise it shuffles the connections.
> 3) hostRecheckSeconds=int. pgjdbc caches "read/write" status of a
> host:port combination, so it don't re-check the status if multiple
> connections are created within hostRecheckSeconds timeframe.
>
> It is sad that essentially the same feature is re-implemented in core with
> different name/semantics.
> Does it make sense to align parameter names/semantics?
>

Looking at

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

Which Tom points out as being relevant to this discussion ISTM that this is
becoming a half baked "feature" that is being cobbled together instead of
being designed. Admittedly biased but I agree with Vladimir that libpq did
not implement the above feature using the same name and semantics. This
just serves to confuse the users.

Just my 2c worth


Dave Cramer

  25. Re: Libpq support to connect to standby server as priority

    Robert Haas <robertmhaas@gmail.com> — 2018-11-21T14:05:08Z

    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

  26. Re: Libpq support to connect to standby server as priority

    Dave Cramer <pg@fastcrypt.com> — 2018-11-21T14:14:52Z

    On Wed, 21 Nov 2018 at 09:05, Robert Haas <robertmhaas@gmail.com> wrote:

> 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.
>
>
And to enforce Robert's argument even further almost every pool
implementation I am aware of
has a keep alive query. So why not use the opportunity to check to see if
is a primary or standby at the same time


Dave Cramer

davec@postgresintl.com
www.postgresintl.com

>
>

  27. Re: Libpq support to connect to standby server as priority

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-14T22:17:16Z

    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

  28. RE: Libpq support to connect to standby server as priority

    Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> — 2019-01-15T02:00:57Z

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


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


Regards
Takayuki Tsunakawa

  29. Re: Libpq support to connect to standby server as priority

    Michael Paquier <michael@paquier.xyz> — 2019-01-15T02:48:55Z

    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

  30. Re: Libpq support to connect to standby server as priority

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-15T02:53:17Z

    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

  31. Re: Libpq support to connect to standby server as priority

    Dave Cramer <pg@fastcrypt.com> — 2019-01-15T13:00:24Z

    On Mon, 14 Jan 2019 at 21:19, Tsunakawa, Takayuki <
tsunakawa.takay@jp.fujitsu.com> 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".
>
Agreed. There's no downside to aliasing and I'd really like to see
consistency.

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

> > 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 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'm confused as to how this would work. Who or what determines if the
server is a primary or standby?

Dave Cramer

davec@postgresintl.com
www.postgresintl.com

>
>

  32. Re: Libpq support to connect to standby server as priority

    Robert Haas <robertmhaas@gmail.com> — 2019-01-15T19:50:07Z

    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

  33. RE: Libpq support to connect to standby server as priority

    Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> — 2019-01-16T03:10:12Z

    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}, which acts 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

  34. RE: Libpq support to connect to standby server as priority

    Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> — 2019-01-16T04:20:16Z

    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(), and set 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

  35. Re: Libpq support to connect to standby server as priority

    Tatsuo Ishii <ishii@sraoss.co.jp> — 2019-01-16T04:42:04Z

    >> 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(), and set 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

  36. RE: Libpq support to connect to standby server as priority

    Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> — 2019-01-16T05:41:03Z

    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

  37. Re: Libpq support to connect to standby server as priority

    Tatsuo Ishii <ishii@sraoss.co.jp> — 2019-01-16T06:02:36Z

    > 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

  38. RE: Libpq support to connect to standby server as priority

    Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> — 2019-01-16T08:26:28Z

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


Regards
Takayuki Tsunakawa

  39. Re: Libpq support to connect to standby server as priority

    Laurenz Albe <laurenz.albe@cybertec.at> — 2019-01-17T10:59:18Z

    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

  40. Re: Libpq support to connect to standby server as priority

    Dave Cramer <pg@fastcrypt.com> — 2019-01-17T13:36:06Z

    On Tue, 15 Jan 2019 at 23:21, Tsunakawa, Takayuki <
tsunakawa.takay@jp.fujitsu.com> wrote:

> 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(), and set 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.
>

Thanks, that clarifies it.


Dave Cramer

davec@postgresintl.com
www.postgresintl.com

>
>

  41. Re: Libpq support to connect to standby server as priority

    Dave Cramer <pg@fastcrypt.com> — 2019-01-17T13:36:53Z

    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.


Dave Cramer

davec@postgresintl.com
www.postgresintl.com

>
>

  42. Re: Libpq support to connect to standby server as priority

    Dave Cramer <pg@fastcrypt.com> — 2019-01-17T14:12:23Z

    On Thu, 17 Jan 2019 at 05:59, Laurenz Albe <laurenz.albe@cybertec.at> wrote:

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

I just had a look at the JDBC code there is no prefer read write. There is
a "preferSecondary"
The logic behind this is that the connection would presumably be only doing
reads so ideally it would like a secondary,
but if it can't find one it will connect to a primary.

To be clear there are 4 target server types in pgJDBC, "any",
"master","secondary", and "preferSecondary" (looking at this I need to
alias master to primary, but that's another discussion)

I have no idea where "I want to write but I'm OK if I cannot came from"?

Dave

  43. Re: Libpq support to connect to standby server as priority

    Tatsuo Ishii <ishii@sraoss.co.jp> — 2019-01-17T23:03:20Z

    > 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

  44. Re: Libpq support to connect to standby server as priority

    Dave Cramer <pg@fastcrypt.com> — 2019-01-18T00:02:15Z

    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

Dave Cramer

davec@postgresintl.com
www.postgresintl.com

>
>

  45. RE: Libpq support to connect to standby server as priority

    Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> — 2019-01-18T00:08:45Z

    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 seemed to try each host once in a while loop.


Regards
Takayuki Tsunakawa

  46. Re: Libpq support to connect to standby server as priority

    Tatsuo Ishii <ishii@sraoss.co.jp> — 2019-01-18T00:15:24Z

    > 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

  47. Re: Libpq support to connect to standby server as priority

    Dave Cramer <pg@fastcrypt.com> — 2019-01-18T00:27:34Z

    On Thu, 17 Jan 2019 at 19:15, Tatsuo Ishii <ishii@sraoss.co.jp> wrote:

> > 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?
>
> well preferSecondary would return a connection.

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

Regards,
Dave

  48. Re: Libpq support to connect to standby server as priority

    Dave Cramer <pg@fastcrypt.com> — 2019-01-18T00:28:30Z

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

> 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 seemed to try each host once
> in a while loop.
>

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


Dave Cramer

davec@postgresintl.com
www.postgresintl.com

>
>

  49. Re: Libpq support to connect to standby server as priority

    Tatsuo Ishii <ishii@sraoss.co.jp> — 2019-01-18T00:38:38Z

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

  50. Re: Libpq support to connect to standby server as priority

    Dave Cramer <pg@fastcrypt.com> — 2019-01-18T00:40:35Z

    On Thu, 17 Jan 2019 at 19:38, 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?
> >>
> >> well preferSecondary would return a connection.
>
> This is not my message :-)
>
> > I'm curious; under what circumstances would the above occur?
>
> Former primary goes down and one of standbys is promoting but it is
> not promoted to new primary yet.
>

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

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

Dave

>
>

  51. Re: Libpq support to connect to standby server as priority

    Tatsuo Ishii <ishii@sraoss.co.jp> — 2019-01-18T00:56:45Z

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

  52. Re: Libpq support to connect to standby server as priority

    Dave Cramer <pg@fastcrypt.com> — 2019-01-18T01:01:44Z

    On Thu, 17 Jan 2019 at 19:56, Tatsuo Ishii <ishii@sraoss.co.jp> wrote:

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

that said yes, I would like input from you.

Thanks,

Dave

  53. RE: Libpq support to connect to standby server as priority

    Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> — 2019-01-18T03:33:34Z

    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 users to write to the database with "CREATE/ALTER USER SET default_transaction_read_only = false"?

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

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



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

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


Regards
Takayuki Tsunakawa

  54. Re: Libpq support to connect to standby server as priority

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2019-01-18T05:34:41Z

    On Fri, Jan 18, 2019 at 2:34 PM Tsunakawa, Takayuki <
tsunakawa.takay@jp.fujitsu.com> 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.
>
>
> > I deem it unlikely that someone will set default_transaction_read_only to
> > FALSE and then complain that the feature is not working as expected, but
> > again
> > I cannot prove that claim.
>
> I wonder what default_transaction_read_only exists for.  For maing the
> database by default and allowing only specific users to write to the
> database with "CREATE/ALTER USER SET default_transaction_read_only = false"?
>

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


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

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

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

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

Regards,
Haribabu Kommi
Fujitsu Australia

  55. RE: Libpq support to connect to standby server as priority

    Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> — 2019-01-18T06:20:56Z

    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 with the PgJDBC's naming, which conveys consistency to PostgreSQL users.  Again, the original desire should have been to connect to a standby to eliminate the burdon on the primary, where the primary may be read-only by default and only a limited group of users are allowed to write to the database.

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


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

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


Regards
Takayuki Tsunakawa

  56. Re: Libpq support to connect to standby server as priority

    Tatsuo Ishii <ishii@sraoss.co.jp> — 2019-01-18T06:28:13Z

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

  57. RE: Libpq support to connect to standby server as priority

    Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> — 2019-01-18T06:33:13Z

    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

  58. Re: Libpq support to connect to standby server as priority

    Laurenz Albe <laurenz.albe@cybertec.at> — 2019-01-18T08:27:04Z

    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

  59. Re: Libpq support to connect to standby server as priority

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2019-01-20T23:16:14Z

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

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

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

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

Regards,
Haribabu Kommi
Fujitsu Australia

  60. RE: Libpq support to connect to standby server as priority

    Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> — 2019-01-21T06:33:05Z

    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 fulfill that need.


Regards
Takayuki Tsunakawa

  61. RE: Libpq support to connect to standby server as priority

    Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> — 2019-01-21T06:48:14Z

    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

  62. Re: Libpq support to connect to standby server as priority

    Michael Paquier <michael@paquier.xyz> — 2019-02-04T01:40:40Z

    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

  63. Re: Libpq support to connect to standby server as priority

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2019-02-06T00:33:29Z

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

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

Thanks for your opinion.

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

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

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

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

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

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

Regards,
Haribabu Kommi
Fujitsu Australia

  64. RE: Libpq support to connect to standby server as priority

    Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> — 2019-02-08T09:16:12Z

    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-only session as the name suggests, not to connect to a standby.

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

Yes.


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

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

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

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


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

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


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

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

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

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

ALL — User sessions are retained during switchover/failover. 

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


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

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

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

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

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


Regards
Takayuki Tsunakawa

  65. Re: Libpq support to connect to standby server as priority

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2019-02-13T13:06:57Z

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

> 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-only session as the name
> suggests, not to connect to a standby.
>

Thanks for correction, yes it uses the transaction_readonly.


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

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

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



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

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


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

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


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

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

Regards,
Haribabu Kommi
Fujitsu Australia

  66. RE: Libpq support to connect to standby server as priority

    Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> — 2019-02-14T02:03:32Z

    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 remember it well...)

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

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


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

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


Regards
Takayuki Tsunakawa

  67. Re: Libpq support to connect to standby server as priority

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2019-02-22T05:14:55Z

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

> 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 remember it well...)
>
> > Yes, I want to work on this patch, hopefully by next commitfest. In case
> > if I didn't get time,
> > I can ask for your help.
>
> I'm glad to hear that.  Sure.  I'd like to review your patch, and possibly
> add/modify code if necessary.  Are you going to add
> target_server_type={primary | standby | prefer_standby} as well as add
> prefer-read to target_session_attr?
>
>
> >       (I wonder which of server_type or server_role feels natural in
> > English.)
> >
> >
> >
> > server_type may be good as it stands with connection option
> > (target_server_type).
>
> Thanks, agreed.  That also follows PgJDBC's targetServerType.
>

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

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

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

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

Regards,
Haribabu Kommi
Fujitsu Australia

  68. RE: Libpq support to connect to standby server as priority

    Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> — 2019-02-22T06:45:32Z

    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

  69. Re: Libpq support to connect to standby server as priority

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2019-02-22T07:49:55Z

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

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

Thanks.


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

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

Regards,
Haribabu Kommi
Fujitsu Australia

  70. RE: Libpq support to connect to standby server as priority

    Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> — 2019-02-25T00:38:05Z

    Hi Hari-san,

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


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

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

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



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


(3) patch 0003
     <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 can be NULL in an out-of-memory case.


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


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

  71. Re: Libpq support to connect to standby server as priority

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2019-02-27T06:44:58Z

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

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


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

fixed.


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


>
> (3) patch 0003
>      <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.
>
>
fixed.


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


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



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



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


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



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

Yes, fixed.


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

fixed.

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

Regards,
Haribabu Kommi
Fujitsu Australia

  72. RE: Libpq support to connect to standby server as priority

    Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> — 2019-02-28T01:59:47Z

    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'd like to see how complex the if conditions in multiple places would be after adding target_server_type, and consider whether we can simplify them together with you.  Even now, the if conditions seem complicated to me... that's probably due to the existence of read_write_host_index.


Regards
Takayuki Tsunakawa

  73. Re: Libpq support to connect to standby server as priority

    Dave Cramer <pg@fastcrypt.com> — 2019-02-28T12:32:22Z

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


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

This is from the pgjdbc list.

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


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

Dave Cramer

davec@postgresintl.com
www.postgresintl.com

  74. Re: Libpq support to connect to standby server as priority

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2019-03-19T01:33:39Z

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

> 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'd like to see how complex
> the if conditions in multiple places would be after adding
> target_server_type, and consider whether we can simplify them together with
> you.  Even now, the if conditions seem complicated to me... that's probably
> due to the existence of read_write_host_index.
>

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

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

Target_session_attrs      Target_server_type

read-write                       prefer-slave, slave
prefer-read                     master, slave
read-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?

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

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

Regards,
Haribabu Kommi
Fujitsu Australia

  75. RE: Libpq support to connect to standby server as priority

    Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> — 2019-03-19T03:05:58Z

    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 because the parameter is new, if the libpq code would get uglier if we support older servers.


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

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


Regards
Takayuki Tsunakawa

  76. Re: Libpq support to connect to standby server as priority

    Robert Haas <robertmhaas@gmail.com> — 2019-03-19T16:45:09Z

    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

  77. RE: Libpq support to connect to standby server as priority

    Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> — 2019-03-20T06:01:11Z

    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

  78. Re: Libpq support to connect to standby server as priority

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2019-03-21T06:25:49Z

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

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

Thanks for your suggestions.

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

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

Regards,
Haribabu Kommi
Fujitsu Australia

  79. Re: Libpq support to connect to standby server as priority

    Robert Haas <robertmhaas@gmail.com> — 2019-03-21T19:57:09Z

    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

  80. Re: Libpq support to connect to standby server as priority

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2019-03-21T20:32:43Z

    On Fri, Mar 22, 2019 at 6:57 AM Robert Haas <robertmhaas@gmail.com> wrote:

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

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

Regards,
Haribabu Kommi
Fujitsu Australia

  81. Re: Libpq support to connect to standby server as priority

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2019-03-22T07:07:16Z

    On Fri, Mar 22, 2019 at 7:32 AM Haribabu Kommi <kommi.haribabu@gmail.com>
wrote:

>
> On Fri, Mar 22, 2019 at 6:57 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
>> 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.
>>
>
> OK, I agree with opinion. I will produce a patch for those new options.
>

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

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

Regards,
Haribabu Kommi
Fujitsu Australia

  82. Re: Libpq support to connect to standby server as priority

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2019-03-25T08:07:39Z

    On Fri, Mar 22, 2019 at 6:07 PM Haribabu Kommi <kommi.haribabu@gmail.com>
wrote:

>
> On Fri, Mar 22, 2019 at 7:32 AM Haribabu Kommi <kommi.haribabu@gmail.com>
> wrote:
>
>>
>> On Fri, Mar 22, 2019 at 6:57 AM Robert Haas <robertmhaas@gmail.com>
>> wrote:
>>
>>> 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.
>>>
>>
>> OK, I agree with opinion. I will produce a patch for those new options.
>>
>
> Here I attached WIP patch for the new options along with other older
> patches.
> The basic cases are working fine, doc and tests are missing.
>
> Currently this patch doesn't implement the GUC_REPORT for recovery mode
> yet. I am yet to optimize the complex if check.
>

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

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

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

opinions

Regards,
Haribabu Kommi
Fujitsu Australia

Note - Attachments order may sometime go wrong.

  83. RE: Libpq support to connect to standby server as priority

    Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> — 2019-03-27T04:41:05Z

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

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

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


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


transaction_readonly -> "SHOW transaction_read_only"
restuctured -> restructured


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

succesful -> successful
means -> it means


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

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

uses the value of <varname>transaction_read_only</varname> configuration parameter that is...

as in:

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

client_encoding
This sets the client_encoding configuration parameter for this connection.

application_name
Specifies a value for the application_name configuration parameter.


(4) 0003
 	bool		std_strings;	/* standard_conforming_strings */
+	bool		transaction_read_only;	/* session_read_only */

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


Regards
Takayuki Tsunakawa

  84. RE: Libpq support to connect to standby server as priority

    Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> — 2019-03-27T06:17:08Z

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

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

"connecting to" can be removed.

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


Regards
Takayuki Tsunakawa

  85. Re: Libpq support to connect to standby server as priority

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2019-03-28T13:16:03Z

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

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

Thanks for the review.

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

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

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

Regards,
Haribabu Kommi
Fujitsu Australia

  86. RE: Libpq support to connect to standby server as priority

    Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> — 2019-03-29T08:05:46Z

    Hi Hari-san,

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

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


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


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


(2) 0001
+        succesful connection or failure.

succesful -> successful


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

conenct -> connect


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

    XLogCtl->SharedRecoveryInProgress = false;


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

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


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


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

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


Regards
Takayuki Tsunakawa

  87. Re: Libpq support to connect to standby server as priority

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2019-04-10T23:13:35Z

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

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

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



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

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

Regards,
Haribabu Kommi
Fujitsu Australia

  88. Re: Libpq support to connect to standby server as priority

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2019-06-03T06:32:19Z

    On Thu, Apr 11, 2019 at 9:13 AM Haribabu Kommi <kommi.haribabu@gmail.com>
wrote:

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

Rebased patches are attached.

Regards,
Haribabu Kommi
Fujitsu Australia

  89. Re: Libpq support to connect to standby server as priority

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2019-07-01T10:09:38Z

    On Mon, 3 Jun 2019 at 16:32, Haribabu Kommi <kommi.haribabu@gmail.com>
wrote:

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

Rebased patches are attached.

Regards,
Haribabu Kommi

  90. Re: Libpq support to connect to standby server as priority

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2019-09-09T17:19:49Z

    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

  91. Re: Libpq support to connect to standby server as priority

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2019-09-10T16:05:29Z

    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

  92. RE: Libpq support to connect to standby server as priority

    Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> — 2019-09-11T00:10:59Z

    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 discussion on it, but I didn't track the conclusion.


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

I'm afraid those weren't attached.


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

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


Regards
Takayuki Tsunakawa

  93. Re: Libpq support to connect to standby server as priority

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2019-09-11T00:17:31Z

    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

  94. Re: Libpq support to connect to standby server as priority

    Greg Nancarrow <gregn4422@gmail.com> — 2019-10-01T10:51:21Z

    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

  95. Re: Libpq support to connect to standby server as priority

    Greg Nancarrow <gregn4422@gmail.com> — 2019-10-01T11:10:03Z

    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

  96. RE: Libpq support to connect to standby server as priority

    tsunakawa.takay@fujitsu.com <tsunakawa.takay@fujitsu.com> — 2019-12-20T00:41:21Z

    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 modification is needed to apply to HEAD.)  I'd like to proceed to testing when the revised patch becomes available.



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

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


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


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

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

(3) 0002
+reject_checked_read_or_write_connection(PGconn *conn)

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


(4) 0003
+reject_checked_recovery_connection(PGconn *conn)

The same as the previous one.


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


Regards
Takayuki Tsunakawa

  97. Re: Libpq support to connect to standby server as priority

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-12-26T20:07:23Z

    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

  98. Re: Libpq support to connect to standby server as priority

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-12-26T20:15:13Z

    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

  99. Re: Libpq support to connect to standby server as priority

    Dave Cramer <pg@fastcrypt.com> — 2019-12-26T20:17:45Z

    On Thu, 26 Dec 2019 at 15:07, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

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

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

>
>

  100. Re: Libpq support to connect to standby server as priority

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-12-26T21:58:37Z

    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

  101. RE: Libpq support to connect to standby server as priority

    tsunakawa.takay@fujitsu.com <tsunakawa.takay@fujitsu.com> — 2019-12-27T01:26:03Z

    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.


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

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


Regards
Takayuki Tsunakawa

  102. Re: Libpq support to connect to standby server as priority

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-12-27T13:08:28Z

    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

  103. RE: Libpq support to connect to standby server as priority

    tsunakawa.takay@fujitsu.com <tsunakawa.takay@fujitsu.com> — 2020-01-06T01:37:46Z

    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 document read-write as a synonym for primary.

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

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


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

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

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

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


Regards
Takayuki Tsunakawa

  104. Re: Libpq support to connect to standby server as priority

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-01-09T15:25:39Z

    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 document read-write as a synonym for primary.
> 
> * When the server version is 13 or later, libpq just checks in_recovery, not checking transaction_read_only or sending SHOW transaction_read_only.
> 
> * When the server version is before 13, libpq sends SHOW transaction_read_only as before.

Yes, that sounds good to me.

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

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

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

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

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

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

Yes.

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

  105. Re: Libpq support to connect to standby server as priority

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-02-28T16:05:35Z

    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

  106. Re: Libpq support to connect to standby server as priority

    David Steele <david@pgmasters.net> — 2020-03-16T13:23:45Z

    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

  107. Re: Libpq support to connect to standby server as priority

    Greg Nancarrow <gregn4422@gmail.com> — 2020-05-18T07:33:10Z

    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

  108. Re: Libpq support to connect to standby server as priority

    Daniel Gustafsson <daniel@yesql.se> — 2020-07-02T12:12:51Z

    > 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

  109. Re: Libpq support to connect to standby server as priority

    Greg Nancarrow <gregn4422@gmail.com> — 2020-07-06T12:19:52Z

    > 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

  110. Re: Libpq support to connect to standby server as priority

    Daniel Gustafsson <daniel@yesql.se> — 2020-07-12T20:50:55Z

    > 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

  111. Re: Libpq support to connect to standby server as priority

    Greg Nancarrow <gregn4422@gmail.com> — 2020-07-20T01:15:02Z

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

  112. RE: Libpq support to connect to standby server as priority

    Smith, Peter <peters@fast.au.fujitsu.com> — 2020-08-11T01:38:08Z

    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 possible option values. 
This format is inconsistent document style with other variables in this SGML. 
Other places are using sub-lists for option values. e.g. look at "six modes" of sslmode.

====

COMMENT libpq.sgml (cut/paste parameter description)

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

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

====

COMMENT libpq.sgml (pg_is_in_recovery)

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

+       <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 that is reported by the server upon successful connection. Otherwise, if the server is prior to version 14, only a connection in which read-write transactions are not accepted by default is considered acceptable. To determine whether the server supports read-write transactions, the query SHOW transaction_read_only will be sent upon any successful connection; if it returns on, it means the server doesn't support read-write transactions.
--

====

COMMENT libpq.sgml (Oxford comma)

+       <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 width should 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 validateAndRecordTargetServerType should 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_gettext of 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 trying to 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. The code 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

  113. RE: Libpq support to connect to standby server as priority

    Smith, Peter <peters@fast.au.fujitsu.com> — 2020-08-11T07:46:39Z

    Hi Greg,

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

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

====

COMMENT - missing "any" tests

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

But this now means there are some missing test cases for

target_server_type = "any"

====

COMMENT - negative tests?

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

But I did not find any such error combination tests.

e.g. Where are these test cases?

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

--

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

e.g. Where are these test cases?

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


Kind Regards,
Peter Smith
---
Fujitsu Australia

  114. Re: Libpq support to connect to standby server as priority

    Robert Haas <robertmhaas@gmail.com> — 2020-08-13T19:48:41Z

    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

  115. RE: Libpq support to connect to standby server as priority

    tsunakawa.takay@fujitsu.com <tsunakawa.takay@fujitsu.com> — 2020-08-14T00:16:27Z

    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 replicated database (ALTER USER SET default_transaction_read_only = on), it's the primary read-only server.


Regards
Takayuki Tsunakawa

  116. Re: Libpq support to connect to standby server as priority

    Greg Nancarrow <gregn4422@gmail.com> — 2020-08-18T11:32:00Z

    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 that is reported by >the server upon successful connection. Otherwise, if the server is prior to version 14, only a connection in which read-write >transactions are not accepted by default is considered acceptable. To determine whether the server supports read-write >transactions, the query SHOW transaction_read_only will be sent upon any successful connection; if it returns on, it means the >server doesn't support read-write transactions.
>--

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 >validateAndRecordTargetServerType should 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_gettext of 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 trying to >acheive.
>...
>
>Suggestion2:
>Consider to separate out the requested_server_type cases instead of trying to hand everything in the same else block. The code >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 should be an >error like "could not make a readonly connection to server "
>
>But I did not find any such error combination tests.
>
>e.g. Where are these test cases?
>
>target_session_attrs = "standby", when no standby is available
>target_session_attrs = "secondary", when no standby is available
>target_server_type = "standby", when no standby is available
>target_server_type = "secondary", when no standby is available
>
>--
>
>And, similarly for "could not make a writable connection to server ".
>
>e.g. Where are these test cases?
>
>target_session_attrs = "primary", when no primary is available
>target_session_attrs = "read-write", when no primary is available
>target_server_type = "primary", when no primary is available
>target_server_type = "read-write", when no primary is available

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

  117. Re: Libpq support to connect to standby server as priority

    Peter Smith <smithpb2250@gmail.com> — 2020-08-20T01:36:15Z

    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

  118. Re: Libpq support to connect to standby server as priority

    Greg Nancarrow <gregn4422@gmail.com> — 2020-08-21T04:53:33Z

    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

  119. Re: Libpq support to connect to standby server as priority

    Peter Smith <smithpb2250@gmail.com> — 2020-08-21T05:24:49Z

    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

  120. Re: Libpq support to connect to standby server as priority

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-25T21:58:49Z

    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

  121. Re: Libpq support to connect to standby server as priority

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-26T18:34:19Z

    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

  122. Re: Libpq support to connect to standby server as priority

    Greg Nancarrow <gregn4422@gmail.com> — 2020-09-30T07:57:05Z

    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

  123. Re: Libpq support to connect to standby server as priority

    Anastasia Lubennikova <a.lubennikova@postgrespro.ru> — 2020-11-24T12:26:08Z

    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

  124. Re: Libpq support to connect to standby server as priority

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-11-24T21:49:19Z

    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

  125. Re: Libpq support to connect to standby server as priority

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2020-11-24T22:59:17Z

    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.

  126. Re: Libpq support to connect to standby server as priority

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-11-24T23:19:47Z

    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

  127. Re: Libpq support to connect to standby server as priority

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-11-25T01:07:14Z

    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

  128. Re: Libpq support to connect to standby server as priority

    Greg Nancarrow <gregn4422@gmail.com> — 2020-11-25T07:04:11Z

    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

  129. Re: Libpq support to connect to standby server as priority

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-11-25T16:43:27Z

    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

  130. Re: Libpq support to connect to standby server as priority

    Greg Nancarrow <gregn4422@gmail.com> — 2020-11-26T00:07:01Z

    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

  131. Re: Libpq support to connect to standby server as priority

    Greg Nancarrow <gregn4422@gmail.com> — 2020-12-01T08:09:10Z

    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

  132. Re: Libpq support to connect to standby server as priority

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-01-05T21:34:52Z

    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

  133. Re: Libpq support to connect to standby server as priority

    vignesh C <vignesh21@gmail.com> — 2021-02-08T09:16:48Z

    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

  134. Re: Libpq support to connect to standby server as priority

    Greg Nancarrow <gregn4422@gmail.com> — 2021-02-09T00:17:14Z

    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

  135. Re: Libpq support to connect to standby server as priority

    Greg Nancarrow <gregn4422@gmail.com> — 2021-02-09T08:57:06Z

    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

  136. Re: Libpq support to connect to standby server as priority

    vignesh C <vignesh21@gmail.com> — 2021-02-10T06:07:26Z

    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

  137. Re: Libpq support to connect to standby server as priority

    vignesh C <vignesh21@gmail.com> — 2021-02-10T06:09:48Z

    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

  138. Re: Libpq support to connect to standby server as priority

    Greg Nancarrow <gregn4422@gmail.com> — 2021-02-12T01:37:06Z

    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

  139. Re: Libpq support to connect to standby server as priority

    vignesh C <vignesh21@gmail.com> — 2021-02-12T03:41:49Z

    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

  140. Re: Libpq support to connect to standby server as priority

    Greg Nancarrow <gregn4422@gmail.com> — 2021-02-16T05:48:37Z

    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

  141. Re: Libpq support to connect to standby server as priority

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-03T02:07:17Z

    Greg Nancarrow <gregn4422@gmail.com> writes:
> I've marked this as "Ready for Committer".

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

			regards, tom lane

  142. Re: Libpq support to connect to standby server as priority

    vignesh C <vignesh21@gmail.com> — 2021-03-03T03:48:51Z

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

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

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

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

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

Regards,
Vignesh

  143. Re: Libpq support to connect to standby server as priority

    Greg Nancarrow <gregn4422@gmail.com> — 2021-03-03T04:05:45Z

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


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

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

The buildfarm machine crake is using Perl 5.30.3.

Regards,
Greg Nancarrow
Fujitsu Australia

  144. Re: Libpq support to connect to standby server as priority

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-03T04:39:10Z

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

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

			regards, tom lane

  145. Re: Libpq support to connect to standby server as priority

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-03T18:54:18Z

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

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

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

			regards, tom lane