Re: Libpq support to connect to standby server as priority

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

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

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

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

Haribabu Kommi wrote:

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

Haribabu Kommi wrote:

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

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

Even if we add a variable to cache the connection, I don't think the logic of checking
the next host for the read-only host logic may not change, but the extra connection
request to the read-write host again will be removed.

I evaluated your suggestion of caching the connection and reuse it when there is no
read only server doesn't find, but I am thinking that it will add more complexity and also
the connection to the other servers delays, the cached connection may be closed by
the server also because of timeout.

I feel the extra time during connection may be fine, if user is preferring the prefer-read
mode, instead of adding more complexity in handling the cached connection?

comments?

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

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

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

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

Yours,
Laurenz Albe

On 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

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

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

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

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

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

Haribabu Kommi wrote:

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

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

Yours,
Laurenz Albe

Laurenz Albe <laurenz.albe@cybertec.at> writes:

Haribabu Kommi wrote:

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

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

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

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

/messages/by-id/1700970.cRWpxnom9y@hammer.magicstack.net

regards, tom lane

Tom Lane wrote:

Laurenz Albe <laurenz.albe@cybertec.at> writes:

Haribabu Kommi wrote:

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

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

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

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

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

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

/messages/by-id/1700970.cRWpxnom9y@hammer.magicstack.net

That's a good point.

Yours,
Laurenz Albe

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

/messages/by-id/1700970.cRWpxnom9y@hammer.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

On Tue, Nov 13, 2018 at 05:54:17PM +1100, Haribabu Kommi wrote:

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

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

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

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

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

I also have the same opinion of Laurenz, that this option is letting the
application to connect to read-only server if possible, otherwise let it
connect to read-write server.

I feel that any of the state changes during the connection and after
connection, needs not to be reflected on the existing connection for
these type of connections.

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

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

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

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

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

On Thu, Nov 15, 2018 at 05:14:33PM +0900, Michael Paquier wrote:

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

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

Laurenz Albe <laurenz.albe@cybertec.at> writes:

Tom Lane wrote:

I don't like this patch at all: the business with keeping two connections
open seems impossibly fragile and full of race conditions.

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

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

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

regards, tom lane

Tom Lane wrote:

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

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

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

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

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

Yours,
Laurenz Albe