diff --git a/CMakeLists.txt b/CMakeLists.txt index 7c7e88931..d8d9ea931 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -50,6 +50,7 @@ find_package(ASAN) find_package(TSAN) find_package(CURL) find_package(PAM) +find_package(UBSAN) # Build PCRE2 so we always know the version # Read BuildPCRE2 for details about how to add pcre2 as a dependency to a target @@ -155,6 +156,9 @@ if (WITH_ASAN AND ASAN_FOUND) elseif (WITH_TSAN AND TSAN_FOUND) set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -fsanitize=thread") set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=thread") +elseif (WITH_UBSAN AND UBSAN_FOUND) + set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -fsanitize=undefined") + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=undefined") endif() set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -std=c99 -D_GNU_SOURCE=1 ${FLAGS}") diff --git a/Documentation/Authenticators/PAM-Authenticator.md b/Documentation/Authenticators/PAM-Authenticator.md index 7aee7a794..4e918413b 100644 --- a/Documentation/Authenticators/PAM-Authenticator.md +++ b/Documentation/Authenticators/PAM-Authenticator.md @@ -92,7 +92,7 @@ generating the Unix user and password should be enough. ## Implementation details and limitations -The PAM general authentication scheme is difficult for a proxy such as MaxScale. +The general PAM authentication scheme is difficult for a proxy such as MaxScale. An application using the PAM interface needs to define a *conversation function* to allow the OS PAM modules to communicate with the client, possibly exchanging multiple messages. This works when a client logs in to a normal server, but not @@ -101,25 +101,22 @@ MaxScale to successfully log into the servers, the messages and answers need to be predefined. This requirement denies the use of more exotic schemes such as one-time passwords or two-factor authentication. -The current version of the MaxScale PAM authentication module only supports a +The MaxScale PAM authentication module only supports a simple password exchange. On the client side, the authentication begins with MaxScale sending an AuthSwitchRequest packet. In addition to the command, the packet contains the client plugin name `dialog`, a message type byte `4` and the message `Password: `. In the next packet, the client should send the password, which MaxScale will forward to the PAM API running on the local machine. If the -password is correct, an OK packet is sent to the client. No additional -PAM-related messaging is allowed, as this would indicate a more complicated -authentication scheme. +password is correct, an OK packet is sent to the client. If the local PAM API asks +for additional credentials as is typical in two-factor authentication schemes, +authentication fails. Informational messages such as password expiration +notifications are allowed. These are simply printed to the log. On the backend side, MaxScale expects the servers to act as MaxScale did towards the client. The servers should send an AuthSwitchRequest packet as defined above, MaxScale responds with the password received by the client authenticator -and finally backend replies with OK. - -## SSL support - -PAM Authenticator supports SSL connections from client to MaxScale, but not from -MaxScale to backends. +and finally backend replies with OK. Informational messages from backends are +only printed to the info-log. ## Building the module diff --git a/Documentation/Getting-Started/Configuration-Guide.md b/Documentation/Getting-Started/Configuration-Guide.md index dd8a47854..fa6565b7d 100644 --- a/Documentation/Getting-Started/Configuration-Guide.md +++ b/Documentation/Getting-Started/Configuration-Guide.md @@ -1364,7 +1364,7 @@ server. serversize=10 ``` -The service would then have the parameter `weightby=serversize`. If there are 4 +The service would then have the parameter `weightby=serv_weight`. If there are 4 servers defined in the service (serverA, serverB, serverC and serverD) with the serversize set as shown in the table below, the connections would balanced using the percentages in this table. diff --git a/Documentation/Monitors/MariaDB-Monitor.md b/Documentation/Monitors/MariaDB-Monitor.md index a1b59a6ba..7afc24221 100644 --- a/Documentation/Monitors/MariaDB-Monitor.md +++ b/Documentation/Monitors/MariaDB-Monitor.md @@ -360,10 +360,11 @@ following: 1. Prepare the old master for demotion: 1. Stop any external replication. - 2. Enable the *read\_only*-flag to stop writes. - 3. Disable scheduled server events (if event handling is on). - 4. Run the commands in `demotion_sql_file`. - 5. Flush the binary log (FLUSH LOGS) so that all events are on disk. + 2. Kill connections from super-users since *read\_only* does not affect them. + 3. Enable the *read\_only*-flag to stop writes. + 4. Disable scheduled server events (if event handling is on). + 5. Run the commands in `demotion_sql_file`. + 6. Flush the binary log (FLUSH LOGS) so that all events are on disk. 2. Wait for the new master to catch up with the old master. 3. Promote new master and redirect slaves as in failover steps 3 and 4. Also redirect the demoted old master. diff --git a/cmake/FindUBSAN.cmake b/cmake/FindUBSAN.cmake new file mode 100644 index 000000000..f0afc71c7 --- /dev/null +++ b/cmake/FindUBSAN.cmake @@ -0,0 +1,14 @@ +# Find UBSan libraries +# +# The following variables are set: +# UBSAN_FOUND - If UBSan was found +# UBSAN_LIBRARIES - Path to the libubsan library + +find_library(UBSAN_LIBRARIES NAMES libubsan.so.0 libubsan.so.1 libubsan.so.2 libubsan.so.3 libubsan.so.4) + +if (UBSAN_LIBRARIES) + message(STATUS "Found UBSan libraries: ${UBSAN_LIBRARIES}") + set(UBSAN_FOUND TRUE CACHE INTERNAL "") +else() + message(STATUS "Could not find UBSan") +endif() diff --git a/include/maxscale/buffer.hh b/include/maxscale/buffer.hh index 845861a42..4c60454fd 100644 --- a/include/maxscale/buffer.hh +++ b/include/maxscale/buffer.hh @@ -735,6 +735,7 @@ public: Buffer(GWBUF* pBuffer) : m_pBuffer(pBuffer) { + mxb_assert(pBuffer); } /** diff --git a/include/maxscale/protocol/mysql.hh b/include/maxscale/protocol/mysql.hh index c90a3b99d..2678a60cf 100644 --- a/include/maxscale/protocol/mysql.hh +++ b/include/maxscale/protocol/mysql.hh @@ -305,11 +305,10 @@ typedef enum MXS_COM_RESET_CONNECTION = 31, MXS_COM_STMT_BULK_EXECUTE = 0xfa, MXS_COM_MULTI = 0xfe, - MXS_COM_END + MXS_COM_END, + MXS_COM_UNDEFINED = -1 } mxs_mysql_cmd_t; -static const mxs_mysql_cmd_t MXS_COM_UNDEFINED = (mxs_mysql_cmd_t) -1; - /** * A GWBUF property with this name will contain the latest GTID in string form. * This information is only available in OK packets. @@ -378,6 +377,7 @@ static inline uint32_t MYSQL_GET_PAYLOAD_LEN(const uint8_t* header) static inline uint32_t MYSQL_GET_PACKET_LEN(const GWBUF* buffer) { + mxb_assert(buffer); return MYSQL_GET_PAYLOAD_LEN(GWBUF_DATA(buffer)) + MYSQL_HEADER_LEN; } @@ -628,6 +628,7 @@ void mxs_mysql_set_current_db(MXS_SESSION* session, const char* db); */ static inline uint8_t mxs_mysql_get_command(GWBUF* buffer) { + mxb_assert(buffer); if (GWBUF_LENGTH(buffer) > MYSQL_HEADER_LEN) { return GWBUF_DATA(buffer)[4]; @@ -651,6 +652,7 @@ static inline uint8_t mxs_mysql_get_command(GWBUF* buffer) */ static inline uint32_t mxs_mysql_get_packet_len(GWBUF* buffer) { + mxb_assert(buffer); // The first three bytes of the packet header contain its length uint8_t buf[3]; gwbuf_copy_data(buffer, 0, 3, buf); diff --git a/maxscale-system-test/CMakeLists.txt b/maxscale-system-test/CMakeLists.txt index a66020183..38f6be498 100644 --- a/maxscale-system-test/CMakeLists.txt +++ b/maxscale-system-test/CMakeLists.txt @@ -199,6 +199,9 @@ add_test_executable(maxctrl_basic.cpp maxctrl_basic maxctrl_basic LABELS maxctrl # MXS-2167: Monitors should be able to use extra_port add_test_executable(mxs2167_extra_port.cpp mxs2167_extra_port mxs2167_extra_port LABELS REPL_BACKEND) +# Test KILL QUERY functionality +add_test_executable(kill_query.cpp kill_query replication LABELS REPL_BACKEND) + ############################################ # BEGIN: Tests that require GTID # ############################################ @@ -964,6 +967,12 @@ add_test_executable(pam_authentication.cpp pam_authentication pam_authentication # MXS-2350: On-demand connection creation add_test_executable(mxs2350_lazy_connect.cpp mxs2350_lazy_connect mxs2350_lazy_connect LABELS REPL_BACKEND) +# MXS-2520: Allow master reconnection on reads +add_test_executable(mxs2520_master_read_reconnect.cpp mxs2520_master_read_reconnect mxs2520_master_read_reconnect LABELS REPL_BACKEND readwritesplit) + +# MXS-2464: Crash in route_stored_query with ReadWriteSplit +add_test_executable(mxs2464_sescmd_reconnect.cpp mxs2464_sescmd_reconnect mxs2464_sescmd_reconnect LABELS REPL_BACKEND readwritesplit) + ############################################ # BEGIN: binlogrouter and avrorouter tests # ############################################ diff --git a/maxscale-system-test/cnf/maxscale.cnf.template.mxs2464_sescmd_reconnect b/maxscale-system-test/cnf/maxscale.cnf.template.mxs2464_sescmd_reconnect new file mode 100644 index 000000000..db6bd2478 --- /dev/null +++ b/maxscale-system-test/cnf/maxscale.cnf.template.mxs2464_sescmd_reconnect @@ -0,0 +1,29 @@ +[maxscale] +threads=###threads### + +[MySQL-Monitor] +type=monitor +module=mysqlmon +servers=###server_line### +user=maxskysql +password=skysql +monitor_interval=1000 +backend_read_timeout=1 +backend_connect_timeout=1 + +[RW-Split-Router] +type=service +router=readwritesplit +servers=###server_line### +user=maxskysql +password=skysql +delayed_retry_timeout=1 +transaction_replay=true + +[RW-Split-Listener] +type=listener +service=RW-Split-Router +protocol=MySQLClient +port=4006 + +###server### diff --git a/maxscale-system-test/cnf/maxscale.cnf.template.mxs2520_master_read_reconnect b/maxscale-system-test/cnf/maxscale.cnf.template.mxs2520_master_read_reconnect new file mode 100644 index 000000000..cd66ced60 --- /dev/null +++ b/maxscale-system-test/cnf/maxscale.cnf.template.mxs2520_master_read_reconnect @@ -0,0 +1,30 @@ +[maxscale] +threads=###threads### +log_info=1 + +[MySQL-Monitor] +type=monitor +module=mysqlmon +servers=server1 +user=maxskysql +password=skysql +monitor_interval=1000 +backend_read_timeout=1 +backend_connect_timeout=1 + +[RW-Split-Router] +type=service +router=readwritesplit +servers=server1 +user=maxskysql +password=skysql +delayed_retry_timeout=45 +transaction_replay=true + +[RW-Split-Listener] +type=listener +service=RW-Split-Router +protocol=MySQLClient +port=4006 + +###server### diff --git a/maxscale-system-test/kill_query.cpp b/maxscale-system-test/kill_query.cpp new file mode 100644 index 000000000..d03c37b09 --- /dev/null +++ b/maxscale-system-test/kill_query.cpp @@ -0,0 +1,50 @@ +/** + * Test KILL QUERY functionality + */ + +#include "testconnections.h" + +int main(int argc, char* argv[]) +{ + TestConnections test(argc, argv); + + auto conn = test.maxscales->rwsplit(); + conn.connect(); + conn.query("CREATE OR REPLACE TABLE test.t1 (id LONGTEXT)"); + + for (int x = 0; x < 10; x++) + { + conn.query("INSERT INTO test.t1 VALUES (REPEAT('a', 5000000))"); + } + + for (int i = 0; i < 3; i++) + { + auto a = test.maxscales->rwsplit(); + auto b = test.maxscales->rwsplit(); + test.expect(a.connect() && b.connect(), "Connections should work"); + + auto id = a.thread_id(); + + test.set_timeout(15); + std::thread thr( + [&]() { + // The ALTER should take longer than 15 seconds to complete so that a KILL is required to + // interrupt it. + test.expect(!a.query("ALTER TABLE test.t1 FORCE"), "ALTER should fail"); + + const char* expected = "Query execution was interrupted"; + test.expect(strstr(a.error(), expected), + "Alter should fail with '%s' but it failed with '%s'", + expected, a.error()); + }); + + test.expect(b.query("KILL QUERY " + std::to_string(id)), "KILL QUERY failed: %s", b.error()); + thr.join(); + + test.stop_timeout(); + } + + conn.query("DROP TABLE test.t1"); + + return test.global_result; +} diff --git a/maxscale-system-test/mariadb_func.h b/maxscale-system-test/mariadb_func.h index 0fac77f74..86fb5fc69 100644 --- a/maxscale-system-test/mariadb_func.h +++ b/maxscale-system-test/mariadb_func.h @@ -348,6 +348,11 @@ public: m_pw = pw; } + uint32_t thread_id() const + { + return mysql_thread_id(m_conn); + } + private: std::string m_host; int m_port; diff --git a/maxscale-system-test/mxs2464_sescmd_reconnect.cpp b/maxscale-system-test/mxs2464_sescmd_reconnect.cpp new file mode 100644 index 000000000..989e62560 --- /dev/null +++ b/maxscale-system-test/mxs2464_sescmd_reconnect.cpp @@ -0,0 +1,35 @@ +/** + * MXS-2464: Crash in route_stored_query with ReadWriteSplit + * https://jira.mariadb.org/browse/MXS-2464 + */ + +#include "testconnections.h" + +int main(int argc, char* argv[]) +{ + TestConnections test(argc, argv); + + test.maxscales->connect(); + std::thread thr([&]() { + sleep(5); + test.tprintf("block node 0"); + test.repl->block_node(0); + test.tprintf("wait for monitor"); + test.maxscales->wait_for_monitor(2); + test.tprintf("unblock node 0"); + test.repl->unblock_node(0); + }); + + constexpr const char* query = "SET @a = (SELECT SLEEP(10))"; + test.set_timeout(60); + test.tprintf("%s", query); + test.try_query(test.maxscales->conn_rwsplit[0], query); + test.stop_timeout(); + + test.tprintf("disconnect"); + test.maxscales->disconnect(); + test.tprintf("join"); + thr.join(); + + return test.global_result; +} diff --git a/maxscale-system-test/mxs2520_master_read_reconnect.cpp b/maxscale-system-test/mxs2520_master_read_reconnect.cpp new file mode 100644 index 000000000..5a0955884 --- /dev/null +++ b/maxscale-system-test/mxs2520_master_read_reconnect.cpp @@ -0,0 +1,34 @@ +/** + * MXS-2520: Allow master reconnection on reads + * https://jira.mariadb.org/browse/MXS-2520 + */ + +#include "testconnections.h" + +int main(int argc, char* argv[]) +{ + TestConnections test(argc, argv); + + test.maxscales->connect(); + std::thread thr([&]() { + sleep(5); + test.tprintf("block node 0"); + test.repl->block_node(0); + test.tprintf("wait for monitor"); + test.maxscales->wait_for_monitor(2); + test.tprintf("unblock node 0"); + test.repl->unblock_node(0); + }); + + test.set_timeout(60); + test.tprintf("SELECT SLEEP(10)"); + test.try_query(test.maxscales->conn_rwsplit[0], "SELECT SLEEP(10)"); + test.stop_timeout(); + + test.tprintf("disconnect"); + test.maxscales->disconnect(); + test.tprintf("join"); + thr.join(); + + return test.global_result; +} diff --git a/maxscale-system-test/nodes.cpp b/maxscale-system-test/nodes.cpp index 68fda7ce6..7c23011a9 100644 --- a/maxscale-system-test/nodes.cpp +++ b/maxscale-system-test/nodes.cpp @@ -322,11 +322,11 @@ int Nodes::read_basic_env() { // reading IPs sprintf(env_name, "%s_%03d_network", prefix, i); - IP[i] = get_nc_item((char*) env_name); + IP[i] = strdup(get_nc_item(env_name).c_str()); // reading private IPs sprintf(env_name, "%s_%03d_private_ip", prefix, i); - IP_private[i] = get_nc_item((char*) env_name); + IP_private[i] = strdup(get_nc_item(env_name).c_str()); if (IP_private[i] == NULL) { IP_private[i] = IP[i]; @@ -335,7 +335,7 @@ int Nodes::read_basic_env() // reading IPv6 sprintf(env_name, "%s_%03d_network6", prefix, i); - IP6[i] = get_nc_item((char*) env_name); + IP6[i] = strdup(get_nc_item(env_name).c_str()); if (IP6[i] == NULL) { IP6[i] = IP[i]; @@ -344,11 +344,11 @@ int Nodes::read_basic_env() //reading sshkey sprintf(env_name, "%s_%03d_keyfile", prefix, i); - sshkey[i] = get_nc_item((char*) env_name); + sshkey[i] = strdup(get_nc_item(env_name).c_str()); sprintf(env_name, "%s_%03d_whoami", prefix, i); - access_user[i] = get_nc_item((char*) env_name); + access_user[i] = strdup(get_nc_item(env_name).c_str()); if (access_user[i] == NULL) { access_user[i] = (char *) "vagrant"; @@ -369,7 +369,7 @@ int Nodes::read_basic_env() } sprintf(env_name, "%s_%03d_hostname", prefix, i); - hostname[i] = get_nc_item((char*) env_name); + hostname[i] = strdup(get_nc_item(env_name).c_str()); if (hostname[i] == NULL) { hostname[i] = IP[i]; @@ -396,13 +396,14 @@ const char* Nodes::ip(int i) const return use_ipv6 ? IP6[i] : IP[i]; } -char * Nodes::get_nc_item(char * item_name) +std::string Nodes::get_nc_item(const char* item_name) { size_t start = network_config.find(item_name); if (start == std::string::npos) { - return NULL; + return ""; } + size_t end = network_config.find("\n", start); size_t equal = network_config.find("=", start); if (end == std::string::npos) @@ -411,14 +412,14 @@ char * Nodes::get_nc_item(char * item_name) } if (equal == std::string::npos) { - return NULL; + return ""; } - char * cstr = new char [end - equal + 1]; - strcpy(cstr, network_config.substr(equal + 1, end - equal - 1).c_str()); - setenv(item_name, cstr, 1); + std::string str = network_config.substr(equal + 1, end - equal - 1); - return (cstr); + setenv(item_name, str.c_str(), 1); + + return str; } int Nodes::get_N() diff --git a/maxscale-system-test/nodes.h b/maxscale-system-test/nodes.h index ba2b26192..5dc45583d 100644 --- a/maxscale-system-test/nodes.h +++ b/maxscale-system-test/nodes.h @@ -181,9 +181,9 @@ public: /** * @brief get_nc_item Find variable in the MDBCI network_config file * @param item_name Name of the variable - * @return value of variable + * @return value of variable or empty value if not found */ - char *get_nc_item(char * item_name); + std::string get_nc_item(const char* item_name); /** * @brief get_N Calculate the number of nodes discribed in the _netoek_config file diff --git a/server/core/CMakeLists.txt b/server/core/CMakeLists.txt index c0b9df974..2d2b85095 100644 --- a/server/core/CMakeLists.txt +++ b/server/core/CMakeLists.txt @@ -74,6 +74,8 @@ target_link_libraries(maxscale-common if(WITH_ASAN AND ASAN_FOUND) target_link_libraries(maxscale-common ${ASAN_LIBRARIES}) +elseif(WITH_UBSAN AND UBSAN_FOUND) + target_link_libraries(maxscale-common ${UBSAN_LIBRARIES}) endif() find_library(HAVE_LIBDL NAMES dl) diff --git a/server/core/backend.cc b/server/core/backend.cc index d060fb66d..21a7e6d0a 100644 --- a/server/core/backend.cc +++ b/server/core/backend.cc @@ -53,6 +53,7 @@ void Backend::close(close_type type) { m_closed = true; m_closed_at = time(NULL); + m_session_commands.clear(); if (in_use()) { diff --git a/server/modules/authenticator/MariaDBAuth/mysql_auth.cc b/server/modules/authenticator/MariaDBAuth/mysql_auth.cc index 9dfc89c60..d6f613741 100644 --- a/server/modules/authenticator/MariaDBAuth/mysql_auth.cc +++ b/server/modules/authenticator/MariaDBAuth/mysql_auth.cc @@ -623,6 +623,10 @@ static bool mysql_auth_set_client_data(MYSQL_session* client_data, } } } + else + { + client_data->correct_authenticator = true; + } } } } diff --git a/server/modules/filter/dbfwfilter/dbfwfilter.hh b/server/modules/filter/dbfwfilter/dbfwfilter.hh index 7db83f678..5f22a7581 100644 --- a/server/modules/filter/dbfwfilter/dbfwfilter.hh +++ b/server/modules/filter/dbfwfilter/dbfwfilter.hh @@ -305,7 +305,7 @@ class Rule; typedef std::shared_ptr SRule; /** Helper function for strdup'ing in printf style */ -char* create_error(const char* format, ...); +char* create_error(const char* format, ...) __attribute__ ((nonnull)); /** * Check if a rule matches diff --git a/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc b/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc index 952d38285..386821d43 100644 --- a/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc +++ b/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc @@ -229,6 +229,53 @@ std::string get_version_string(SERVICE* service) return rval; } +uint8_t get_charset(SERVER_REF* servers) +{ + uint8_t rval = 0; + + for (SERVER_REF* s = servers; s; s = s->next) + { + if (server_ref_is_active(s)) + { + if (s->server->is_master()) + { + // Master found, stop searching + rval = s->server->charset; + break; + } + else if (s->server->is_slave() || (s->server->is_running() && rval == 0)) + { + // Slaves precede Running servers + rval = s->server->charset; + } + } + } + + if (rval == 0) + { + // Charset 8 is latin1, the server default + rval = 8; + } + + return rval; +} + +bool supports_extended_caps(SERVER_REF* servers) +{ + bool rval = false; + + for (SERVER_REF* s = servers; s; s = s->next) + { + if (s->active && s->server->is_active && s->server->version().total >= 100200) + { + rval = true; + break; + } + } + + return rval; +} + /** * @brief Check whether a DCB requires SSL. * @@ -280,25 +327,13 @@ int MySQLSendHandshake(DCB* dcb) uint8_t mysql_plugin_data[13] = ""; uint8_t mysql_server_capabilities_one[2]; uint8_t mysql_server_capabilities_two[2]; - uint8_t mysql_server_language = 8; + uint8_t mysql_server_language = get_charset(dcb->service->dbref); uint8_t mysql_server_status[2]; uint8_t mysql_scramble_len = 21; uint8_t mysql_filler_ten[10] = {}; /* uint8_t mysql_last_byte = 0x00; not needed */ char server_scramble[GW_MYSQL_SCRAMBLE_SIZE + 1] = ""; - - bool is_maria = false; - - if (dcb->service->dbref) - { - mysql_server_language = dcb->service->dbref->server->charset; - - if (dcb->service->dbref->server->version().total >= 100200) - { - /** The backend servers support the extended capabilities */ - is_maria = true; - } - } + bool is_maria = supports_extended_caps(dcb->service->dbref); MySQLProtocol* protocol = DCB_PROTOCOL(dcb, MySQLProtocol); GWBUF* buf; @@ -2062,11 +2097,12 @@ spec_com_res_t handle_query_kill(DCB* dcb, querybuf[copied_len] = '\0'; kill_type_t kt = KT_CONNECTION; uint64_t thread_id = 0; - rval = RES_END; std::string user; if (parse_kill_query(querybuf, &thread_id, &kt, &user)) { + rval = RES_END; + if (thread_id > 0) { mxs_mysql_execute_kill(dcb->session, thread_id, kt); diff --git a/server/modules/routing/readwritesplit/rwsplit_select_backends.cc b/server/modules/routing/readwritesplit/rwsplit_select_backends.cc index 579b65c38..ff1a4093c 100644 --- a/server/modules/routing/readwritesplit/rwsplit_select_backends.cc +++ b/server/modules/routing/readwritesplit/rwsplit_select_backends.cc @@ -282,16 +282,20 @@ RWBackend* RWSplitSession::get_slave_backend(int max_rlag) auto counts = get_slave_counts(m_raw_backends, m_current_master); int best_priority {INT_MAX}; auto current_rank = get_current_rank(); + // Slaves can be taken into use if we need more slave connections + bool need_slaves = counts.second < m_router->max_slave_count(); // Create a list of backends valid for read operations for (auto& backend : m_raw_backends) { - bool can_take_slave_into_use = !backend->in_use() && can_recover_servers() - && backend->can_connect() && counts.second < m_router->max_slave_count() - && (backend->is_slave() || backend->is_master()); - + // We can take the current master back into use even for reads + bool my_master = backend == m_current_master; + bool can_take_into_use = !backend->in_use() && can_recover_servers() && backend->can_connect(); bool master_or_slave = backend->is_master() || backend->is_slave(); - bool is_usable = backend->in_use() || can_take_slave_into_use; + + // The server is usable if it's already in use or it can be taken into use and we need either more + // slaves or a master. + bool is_usable = backend->in_use() || (can_take_into_use && (need_slaves || my_master)); bool rlag_ok = rpl_lag_is_ok(backend, max_rlag); int priority = get_backend_priority(backend, m_config.master_accept_reads); auto rank = backend->server()->rank(); diff --git a/server/modules/routing/readwritesplit/rwsplitsession.cc b/server/modules/routing/readwritesplit/rwsplitsession.cc index d151fd138..4c6a46ea5 100644 --- a/server/modules/routing/readwritesplit/rwsplitsession.cc +++ b/server/modules/routing/readwritesplit/rwsplitsession.cc @@ -155,6 +155,7 @@ int32_t RWSplitSession::routeQuery(GWBUF* querybuf) m_query_queue.emplace_back(querybuf); querybuf = NULL; rval = 1; + mxb_assert(m_expected_responses != 0); if (m_expected_responses == 0 && !route_stored_query()) { @@ -806,6 +807,7 @@ void RWSplitSession::clientReply(GWBUF* writebuf, DCB* backend_dcb) // Backend is still in use and has more session commands to execute if (backend->execute_session_command() && backend->is_waiting_result()) { + MXS_INFO("%lu session commands left on '%s'", backend->session_command_count(), backend->name()); m_expected_responses++; } } @@ -948,6 +950,44 @@ bool RWSplitSession::start_trx_replay() return rval; } +bool RWSplitSession::retry_master_query(RWBackend* backend) +{ + bool can_continue = false; + + if (backend->has_session_commands()) + { + // Try to route the session command again. If the master is not available, the response will be + // returned from one of the slaves. + + mxb_assert(!m_current_query.get()); + mxb_assert(!m_sescmd_list.empty()); + mxb_assert(m_sescmd_count >= 2); + MXS_INFO("Retrying session command due to master failure: %s", + backend->next_session_command()->to_string().c_str()); + + // Before routing it, pop the failed session command off the list and decrement the number of + // executed session commands. This "overwrites" the existing command and prevents history duplication. + m_sescmd_list.pop_back(); + --m_sescmd_count; + + retry_query(backend->next_session_command()->deep_copy_buffer()); + can_continue = true; + } + else if (m_current_query.get()) + { + retry_query(m_current_query.release()); + can_continue = true; + } + else + { + // This should never happen + mxb_assert_message(!true, "m_current_query is empty and no session commands being executed"); + MXS_ERROR("Current query unexpectedly empty when trying to retry query on master"); + } + + return can_continue; +} + /** * @brief Router error handling routine * @@ -1014,8 +1054,7 @@ void RWSplitSession::handleError(GWBUF* errmsgbuf, if (can_retry_query()) { - can_continue = true; - retry_query(m_current_query.release()); + can_continue = retry_master_query(backend); } else if (m_config.master_failure_mode == RW_ERROR_ON_WRITE) { @@ -1151,6 +1190,7 @@ bool RWSplitSession::handle_error_new_connection(DCB* backend_dcb, GWBUF* errmsg if (stored && m_config.retry_failed_reads) { + mxb_assert(m_expected_responses == 0); MXS_INFO("Re-routing failed read after server '%s' failed", backend->name()); retry_query(stored, 0); } diff --git a/server/modules/routing/readwritesplit/rwsplitsession.hh b/server/modules/routing/readwritesplit/rwsplitsession.hh index 070d8a04f..50987f2a9 100644 --- a/server/modules/routing/readwritesplit/rwsplitsession.hh +++ b/server/modules/routing/readwritesplit/rwsplitsession.hh @@ -186,6 +186,7 @@ private: int get_max_replication_lag(); mxs::RWBackend* get_backend_from_dcb(DCB* dcb); + bool retry_master_query(mxs::RWBackend* backend); void handle_error_reply_client(DCB* backend_dcb, GWBUF* errmsg); bool handle_error_new_connection(DCB* backend_dcb, GWBUF* errmsg); void manage_transactions(mxs::RWBackend* backend, GWBUF* writebuf);