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.a...@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
signature.asc
Description: PGP signature
