From 021d48f94c347ba1e6df6185bf836a3d343d5281 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 14 Jan 2019 13:14:20 +0200 Subject: [PATCH 01/11] Log low-level reason and idle time on master failure If the connection to the master is lost, knowing what type of an error caused the call to handleError helps deduce what was the real reason for it. Logging the idle time of the connection helps detect when the wait_timeout of a connection is exceeded. --- .../protocol/MySQL/mariadbbackend/mysql_backend.cc | 3 ++- server/modules/routing/readwritesplit/readwritesplit.hh | 9 +++++++++ .../routing/readwritesplit/rwsplit_session_cmd.cc | 2 +- server/modules/routing/readwritesplit/rwsplitsession.cc | 5 ++++- 4 files changed, 16 insertions(+), 3 deletions(-) diff --git a/server/modules/protocol/MySQL/mariadbbackend/mysql_backend.cc b/server/modules/protocol/MySQL/mariadbbackend/mysql_backend.cc index 3e4f5da2d..8d36489ff 100644 --- a/server/modules/protocol/MySQL/mariadbbackend/mysql_backend.cc +++ b/server/modules/protocol/MySQL/mariadbbackend/mysql_backend.cc @@ -1465,7 +1465,8 @@ static int backend_write_delayqueue(DCB* dcb, GWBUF* buffer) if (rc == 0) { - do_handle_error(dcb, ERRACT_NEW_CONNECTION, "Lost connection to backend server."); + do_handle_error(dcb, ERRACT_NEW_CONNECTION, + "Lost connection to backend server while writing delay queue."); } return rc; diff --git a/server/modules/routing/readwritesplit/readwritesplit.hh b/server/modules/routing/readwritesplit/readwritesplit.hh index f4364311f..01163d7d7 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.hh +++ b/server/modules/routing/readwritesplit/readwritesplit.hh @@ -424,3 +424,12 @@ SRWBackendVector::iterator find_best_backend(SRWBackendVector& backends, * The following are implemented in rwsplit_tmp_table_multi.c */ void close_all_connections(mxs::SRWBackendList& backends); + +/** + * Utility function for extracting error messages from buffers + * + * @param buffer Buffer containing an error + * + * @return String representation of the error + */ +std::string extract_error(GWBUF* buffer); diff --git a/server/modules/routing/readwritesplit/rwsplit_session_cmd.cc b/server/modules/routing/readwritesplit/rwsplit_session_cmd.cc index 43aea773a..bc53221dc 100644 --- a/server/modules/routing/readwritesplit/rwsplit_session_cmd.cc +++ b/server/modules/routing/readwritesplit/rwsplit_session_cmd.cc @@ -29,7 +29,7 @@ using namespace maxscale; */ -static std::string extract_error(GWBUF* buffer) +std::string extract_error(GWBUF* buffer) { std::string rval; diff --git a/server/modules/routing/readwritesplit/rwsplitsession.cc b/server/modules/routing/readwritesplit/rwsplitsession.cc index 1c7136962..74da0cf81 100644 --- a/server/modules/routing/readwritesplit/rwsplitsession.cc +++ b/server/modules/routing/readwritesplit/rwsplitsession.cc @@ -943,7 +943,10 @@ void RWSplitSession::handleError(GWBUF* errmsgbuf, } else { - MXS_ERROR("Lost connection to the master server, closing session.%s", errmsg.c_str()); + int64_t idle = mxs_clock() - backend->dcb()->last_read; + MXS_ERROR("Lost connection to the master server, closing session.%s " + "Connection has been idle for %.1f seconds. Error caused by: %s", + errmsg.c_str(), (float)idle / 10.f, extract_error(errmsgbuf).c_str()); } } From 57fe5ff56ad0e4a2b6524de073e2fbd342cd90d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 14 Jan 2019 13:39:02 +0200 Subject: [PATCH 02/11] Fix error packet stringification function The code read past the stack buffer. --- .../routing/readwritesplit/rwsplit_session_cmd.cc | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/server/modules/routing/readwritesplit/rwsplit_session_cmd.cc b/server/modules/routing/readwritesplit/rwsplit_session_cmd.cc index bc53221dc..cd775f9f0 100644 --- a/server/modules/routing/readwritesplit/rwsplit_session_cmd.cc +++ b/server/modules/routing/readwritesplit/rwsplit_session_cmd.cc @@ -35,13 +35,18 @@ std::string extract_error(GWBUF* buffer) if (MYSQL_IS_ERROR_PACKET(((uint8_t*)GWBUF_DATA(buffer)))) { - size_t replylen = MYSQL_GET_PAYLOAD_LEN(GWBUF_DATA(buffer)); + size_t replylen = MYSQL_GET_PAYLOAD_LEN(GWBUF_DATA(buffer)) + MYSQL_HEADER_LEN; char replybuf[replylen]; gwbuf_copy_data(buffer, 0, sizeof(replybuf), (uint8_t*)replybuf); std::string err; std::string msg; - err.append(replybuf + 8, 5); - msg.append(replybuf + 13, replylen - 4 - 5); + + /** + * The payload starts with a one byte command followed by a two byte error code, a six byte state and + * a human-readable string that spans the rest of the packet. + */ + err.append(replybuf + MYSQL_HEADER_LEN + 3, 6); + msg.append(replybuf + MYSQL_HEADER_LEN + 3 + 6, replylen - MYSQL_HEADER_LEN - 3 - 6); rval = err + ": " + msg; } From db98ecbaa1182ee07c2eab35bc5345ac9fb1ed30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 14 Jan 2019 21:43:09 +0200 Subject: [PATCH 03/11] MXS-2259: Fix double addition of DCBs to worker list When poll_add_dcb was called for a DCB that once was polling system but was subsequently removed, the DCB would appear twice in the worker's list of DCBs. This caused a hang when the DCB was the last one in the worker's list and dcb_foreach_local would be called. To prevent the aforementioned problem, the DCBs are now added and removed directly to and from the workers instead of indirectly via poll_add_dcb and poll_remove_dcb. --- server/core/dcb.cc | 47 +++++++++++++++++++++++++++++++++++----------- 1 file changed, 36 insertions(+), 11 deletions(-) diff --git a/server/core/dcb.cc b/server/core/dcb.cc index 49b8e67b7..c592dec8c 100644 --- a/server/core/dcb.cc +++ b/server/core/dcb.cc @@ -65,6 +65,12 @@ using maxbase::Worker; #define DCB_EH_NOTICE(s, p) #endif +#ifdef EPOLLRDHUP +constexpr uint32_t poll_events = EPOLLIN | EPOLLOUT | EPOLLRDHUP | EPOLLHUP | EPOLLET; +#else +constexpr uint32_t poll_events = EPOLLIN | EPOLLOUT | EPOLLHUP | EPOLLET; +#endif + namespace { @@ -2826,6 +2832,7 @@ static void dcb_add_to_list(DCB* dcb) } else { + mxb_assert(this_unit.all_dcbs[id]->thread.tail->thread.next != dcb); this_unit.all_dcbs[id]->thread.tail->thread.next = dcb; this_unit.all_dcbs[id]->thread.tail = dcb; } @@ -2989,6 +2996,8 @@ void dcb_foreach_local(bool (* func)(DCB* dcb, void* data), void* data) for (DCB* dcb = this_unit.all_dcbs[thread_id]; dcb; dcb = dcb->thread.next) { + mxb_assert(dcb->thread.next != dcb); + if (!func(dcb, data)) { break; @@ -3568,13 +3577,7 @@ int poll_add_dcb(DCB* dcb) { dcb_sanity_check(dcb); - uint32_t events = 0; - -#ifdef EPOLLRDHUP - events = EPOLLIN | EPOLLOUT | EPOLLRDHUP | EPOLLHUP | EPOLLET; -#else - events = EPOLLIN | EPOLLOUT | EPOLLHUP | EPOLLET; -#endif + uint32_t events = poll_events; /** Choose new state and worker thread ID according to the role of DCB. */ dcb_state_t new_state; @@ -3726,13 +3729,23 @@ DCB* dcb_get_current() static int upstream_throttle_callback(DCB* dcb, DCB_REASON reason, void* userdata) { DCB* client_dcb = dcb->session->client_dcb; + mxb::Worker* worker = static_cast(client_dcb->poll.owner); + + // The fd is removed manually here due to the fact that poll_add_dcb causes the DCB to be added to the + // worker's list of DCBs but poll_remove_dcb doesn't remove it from it. This is due to the fact that the + // DCBs are only removed from the list when they are closed. if (reason == DCB_REASON_HIGH_WATER) { - poll_remove_dcb(client_dcb); + MXS_INFO("High water mark hit for '%s'@'%s', not reading data until low water mark is hit", + client_dcb->user, client_dcb->remote); + worker->remove_fd(client_dcb->fd); + client_dcb->state = DCB_STATE_NOPOLLING; } else if (reason == DCB_REASON_LOW_WATER) { - poll_add_dcb(client_dcb); + MXS_INFO("Low water mark hit for '%s'@'%s', accepting new data", client_dcb->user, client_dcb->remote); + worker->add_fd(client_dcb->fd, poll_events, (MXB_POLL_DATA*)client_dcb); + client_dcb->state = DCB_STATE_POLLING; } return 0; @@ -3744,7 +3757,13 @@ bool backend_dcb_remove_func(DCB* dcb, void* data) if (dcb->session == session && dcb->dcb_role == DCB_ROLE_BACKEND_HANDLER) { - poll_remove_dcb(dcb); + DCB* client_dcb = dcb->session->client_dcb; + MXS_INFO("High water mark hit for connection to '%s' from %s'@'%s', not reading data until low water " + "mark is hit", dcb->server->name, client_dcb->user, client_dcb->remote); + + mxb::Worker* worker = static_cast(dcb->poll.owner); + worker->remove_fd(dcb->fd); + dcb->state = DCB_STATE_NOPOLLING; } return true; @@ -3756,7 +3775,13 @@ bool backend_dcb_add_func(DCB* dcb, void* data) if (dcb->session == session && dcb->dcb_role == DCB_ROLE_BACKEND_HANDLER) { - poll_add_dcb(dcb); + DCB* client_dcb = dcb->session->client_dcb; + MXS_INFO("Low water mark hit for connection to '%s' from '%s'@'%s', accepting new data", + dcb->server->name, client_dcb->user, client_dcb->remote); + + mxb::Worker* worker = static_cast(dcb->poll.owner); + worker->add_fd(dcb->fd, poll_events, (MXB_POLL_DATA*)dcb); + dcb->state = DCB_STATE_POLLING; } return true; From 511f01a28dea250dc8fb9bbbe2cbe7e91a10f646 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Tue, 15 Jan 2019 08:50:04 +0200 Subject: [PATCH 04/11] MXS-2252: Add test case Added a test case that verifies the correct user is used. --- maxscale-system-test/mariadb_nodes.h | 8 ++++++++ maxscale-system-test/proxy_protocol.cpp | 14 ++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/maxscale-system-test/mariadb_nodes.h b/maxscale-system-test/mariadb_nodes.h index 34460627b..5875c0a47 100644 --- a/maxscale-system-test/mariadb_nodes.h +++ b/maxscale-system-test/mariadb_nodes.h @@ -153,6 +153,14 @@ public: int connect(int i, const std::string& db = "test"); int connect(const std::string& db = "test"); + /** + * Get a Connection to a node + */ + Connection get_connection(int i, const std::string& db = "test") + { + return Connection(IP[i], port[i], user_name, password, db, ssl); + } + /** * Repeatedly try to connect with one second sleep in between attempts * diff --git a/maxscale-system-test/proxy_protocol.cpp b/maxscale-system-test/proxy_protocol.cpp index 9b991e007..a288ed6eb 100644 --- a/maxscale-system-test/proxy_protocol.cpp +++ b/maxscale-system-test/proxy_protocol.cpp @@ -167,6 +167,20 @@ int main(int argc, char *argv[]) } } + + /** + * MXS-2252: Proxy Protocol not displaying originating IP address in SHOW PROCESSLIST + * https://jira.mariadb.org/browse/MXS-2252 + */ + Connection direct = test.repl->get_connection(0); + Connection rwsplit = test.maxscales->rwsplit(0); + direct.connect(); + rwsplit.connect(); + auto d = direct.field("SELECT USER()"); + auto r = rwsplit.field("SELECT USER()"); + test.tprintf("Direct: %s Readwritesplit: %s", d.c_str(), r.c_str()); + test.expect(d == r, "Both connections should return the same user: %s != %s", d.c_str(), r.c_str()); + if (server_proxy_setting) { // Restore server settings. From f5f6a12484578572bce1bfc6b918d504f647ce1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Wed, 16 Jan 2019 10:01:58 +0200 Subject: [PATCH 05/11] Start Galera correctly The Galera documentation tells us to use the galera_new_cluster command to start a new Galera cluster. This should prevent the problems of nodes failing to join the cluster either on the initial startup or after a node goes down. --- maxscale-system-test/mariadb_nodes.cpp | 113 +++++++++---------------- 1 file changed, 40 insertions(+), 73 deletions(-) diff --git a/maxscale-system-test/mariadb_nodes.cpp b/maxscale-system-test/mariadb_nodes.cpp index 649944e60..676e6ed84 100644 --- a/maxscale-system-test/mariadb_nodes.cpp +++ b/maxscale-system-test/mariadb_nodes.cpp @@ -456,93 +456,60 @@ int Mariadb_nodes::start_replication() int Galera_nodes::start_galera() { bool old_verbose = verbose; - char str[1024]; - char sys1[1024]; int local_result = 0; local_result += stop_nodes(); - // Remove the grastate.dat file - ssh_node(0, "rm -f /var/lib/mysql/grastate.dat", true); + std::stringstream ss; + + for (int i = 0; i < N; i++) + { + ss << (i == 0 ? "" : ",") << IP_private[i]; + } + + auto gcomm = ss.str(); + + for (int i = 0; i < N; i++) + { + // Remove the grastate.dat file + ssh_node(i, "rm -f /var/lib/mysql/grastate.dat", true); + + ssh_node(i, "echo [mysqld] > cluster_address.cnf", true); + ssh_node_f(i, true, "echo wsrep_cluster_address=gcomm://%s >> cluster_address.cnf", gcomm.c_str()); + ssh_node(i, "cp cluster_address.cnf /etc/my.cnf.d/", true); + + ssh_node_f(i, + true, + "sed -i 's/###NODE-ADDRESS###/%s/' /etc/my.cnf.d/* /etc/mysql/my.cnf.d/*;" + "sed -i \"s|###GALERA-LIB-PATH###|$(ls /usr/lib*/galera/*.so)|g\" /etc/my.cnf.d/* /etc/mysql/my.cnf.d/*", + IP[i]); + } printf("Starting new Galera cluster\n"); fflush(stdout); - ssh_node(0, "echo [mysqld] > cluster_address.cnf", false); - ssh_node(0, "echo wsrep_cluster_address=gcomm:// >> cluster_address.cnf", false); - ssh_node(0, "cp cluster_address.cnf /etc/my.cnf.d/", true); + // Start the first node that also starts a new cluster + ssh_node_f(0, true, "galera_new_cluster"); - ssh_node_f(0, - true, - "sed -i 's/###NODE-ADDRESS###/%s/' /etc/my.cnf.d/* /etc/mysql/my.cnf.d/*;" - "sed -i \"s|###GALERA-LIB-PATH###|$(ls /usr/lib*/galera/*.so)|g\" /etc/my.cnf.d/* /etc/mysql/my.cnf.d/*", - IP[0]); - - - if (start_node(0, (char*) " --wsrep-cluster-address=gcomm://") != 0) + for (int i = 0; i < N; i++) { - cout << "Failed to start first node, trying to prepare it again" << endl; - cout << "---------- BEGIN LOGS ----------" << endl; - verbose = true; - ssh_node_f(0, true, "sudo journalctl -u mariadb | tail -n 50"); - cout << "----------- END LOGS -----------" << endl; - prepare_server(0); - local_result += start_node(0, (char*) " --wsrep-cluster-address=gcomm://"); + if (start_node(i, "") != 0) + { + cout << "Failed to start node" << i << endl; + cout << "---------- BEGIN LOGS ----------" << endl; + verbose = true; + ssh_node_f(0, true, "sudo journalctl -u mariadb | tail -n 50"); + cout << "----------- END LOGS -----------" << endl; + } } + char str[1024]; sprintf(str, "%s/create_user_galera.sh", test_dir); copy_to_node_legacy(str, "~/", 0); - sprintf(str, - "export galera_user=\"%s\"; export galera_password=\"%s\"; ./create_user_galera.sh %s", - user_name, - password, - socket_cmd[0]); - ssh_node(0, str, false); - - std::vector threads; - std::mutex lock; - - for (int i = 1; i < N; i++) - { - auto func = [&, i]() { - printf("Starting node %d\n", i); - fflush(stdout); - ssh_node(i, "echo [mysqld] > cluster_address.cnf", true); - sprintf(str, "echo wsrep_cluster_address=gcomm://%s >> cluster_address.cnf", IP_private[0]); - ssh_node(i, str, true); - ssh_node(i, "cp cluster_address.cnf /etc/my.cnf.d/", true); - ssh_node_f(i, - true, - "sed -i 's/###NODE-ADDRESS###/%s/' /etc/my.cnf.d/* /etc/mysql/my.cnf.d/*;" - "sed -i \"s|###GALERA-LIB-PATH###|$(ls /usr/lib*/galera/*.so)|g\" /etc/my.cnf.d/* /etc/mysql/my.cnf.d/*", - IP[i]); - - sprintf(&sys1[0], " --wsrep-cluster-address=gcomm://%s", IP_private[0]); - if (this->verbose) - { - printf("%s\n", sys1); - fflush(stdout); - } - fflush(stdout); - - if (start_node(i, sys1)) - { - std::lock_guard guard(lock); - cout << "Failed to start node " << i << endl; - cout << "---------- BEGIN LOGS ----------" << endl; - verbose = true; - ssh_node_f(i, true, "sudo journalctl -u mariadb | tail -n 50"); - cout << "----------- END LOGS -----------" << endl; - local_result++; - } - }; - threads.emplace_back(func); - } - - for (auto& a : threads) - { - a.join(); - } + ssh_node_f(0, false, "export galera_user=\"%s\"; export galera_password=\"%s\"; ./create_user_galera.sh %s", + user_name, + password, + socket_cmd[0]); local_result += robust_connect(5) ? 0 : 1; local_result += execute_query(nodes[0], "%s", create_repl_user); From ba40916d4abfc55feffd802b904815f2e27a819d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Wed, 16 Jan 2019 11:22:26 +0200 Subject: [PATCH 06/11] MXS-2266: Close prepared statements with internal ID The ID used to store the prepared statements uses the internal ID and using the external ID caused unwanted memory use and a false warning. --- server/core/queryclassifier.cc | 14 ++++++++++++-- .../routing/readwritesplit/rwsplit_route_stmt.cc | 1 + 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/server/core/queryclassifier.cc b/server/core/queryclassifier.cc index 802a2fe5f..18d87d929 100644 --- a/server/core/queryclassifier.cc +++ b/server/core/queryclassifier.cc @@ -390,8 +390,18 @@ uint32_t QueryClassifier::ps_get_type(std::string id) const void QueryClassifier::ps_erase(GWBUF* buffer) { - m_ps_handles.erase(qc_mysql_extract_ps_id(buffer)); - m_sPs_manager->erase(buffer); + if (qc_mysql_is_ps_command(mxs_mysql_get_command(buffer))) + { + // Erase the type of the statement stored with the internal ID + m_sPs_manager->erase(ps_id_internal_get(buffer)); + // ... and then erase the external to internal ID mapping + m_ps_handles.erase(qc_mysql_extract_ps_id(buffer)); + } + else + { + // Not a PS command, we don't need the ID mapping + m_sPs_manager->erase(buffer); + } } bool QueryClassifier::query_type_is_read_only(uint32_t qtype) const diff --git a/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc b/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc index 65cf6092f..56fc1c2fb 100644 --- a/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc +++ b/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc @@ -451,6 +451,7 @@ bool RWSplitSession::route_session_write(GWBUF* querybuf, uint8_t command, uint3 } else if (qc_query_is_type(type, QUERY_TYPE_DEALLOC_PREPARE)) { + mxb_assert(!mxs_mysql_is_ps_command(m_qc.current_route_info().command())); m_qc.ps_erase(querybuf); } From 7181ea408328a590c2acf9b26a223cb1011bacab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Wed, 16 Jan 2019 11:30:52 +0200 Subject: [PATCH 07/11] MXS-2266: Add test case Extended the existing tests to cover MXS-2266. --- maxscale-system-test/CMakeLists.txt | 2 +- maxscale-system-test/binary_ps.cpp | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/maxscale-system-test/CMakeLists.txt b/maxscale-system-test/CMakeLists.txt index da04de531..cfce1f6e3 100644 --- a/maxscale-system-test/CMakeLists.txt +++ b/maxscale-system-test/CMakeLists.txt @@ -455,7 +455,7 @@ add_test_executable(mxs359_read_only.cpp mxs359_read_only mxs359_read_only LABEL # Test master_failure_mode=error_on_write and master replacement add_test_executable(mxs359_error_on_write.cpp mxs359_error_on_write mxs359_error_on_write LABELS readwritesplit REPL_BACKEND) -# Binary protocol prepared statement tests +# Binary protocol prepared statement tests (also tests MXS-2266) add_test_executable(binary_ps.cpp binary_ps replication LABELS readwritesplit LIGHT REPL_BACKEND) add_test_executable(binary_ps_cursor.cpp binary_ps_cursor replication LABELS readwritesplit LIGHT REPL_BACKEND) diff --git a/maxscale-system-test/binary_ps.cpp b/maxscale-system-test/binary_ps.cpp index 137f903f9..bd513263e 100644 --- a/maxscale-system-test/binary_ps.cpp +++ b/maxscale-system-test/binary_ps.cpp @@ -46,6 +46,7 @@ int main(int argc, char** argv) test.add_result(strcmp(buffer, server_id[0]), "Expected server_id '%s', got '%s'", server_id[0], buffer); mysql_stmt_close(stmt); + stmt = mysql_stmt_init(test.maxscales->conn_rwsplit[0]); // Execute read, should return a slave server ID @@ -66,5 +67,8 @@ int main(int argc, char** argv) test.maxscales->close_maxscale_connections(0); + // MXS-2266: COM_STMT_CLOSE causes a warning to be logged + test.log_excludes(0, "Closing unknown prepared statement"); + return test.global_result; } From 39dc83f13c0db369d7ec7561350b4d39d9a3423b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Wed, 16 Jan 2019 16:00:32 +0200 Subject: [PATCH 08/11] Don't call python3 directly Calling it directly from the script adds a dependency on it and we don't what that. --- script/maxscale_generate_support_info.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/script/maxscale_generate_support_info.py b/script/maxscale_generate_support_info.py index a99ca1523..3eb73d22c 100755 --- a/script/maxscale_generate_support_info.py +++ b/script/maxscale_generate_support_info.py @@ -1,4 +1,4 @@ -#!/usr/bin/python3 +#!/usr/bin/env python3 # Copyright (c) 2019 MariaDB Corporation Ab # From 317166540f3c90058680aca9a6fc70599cf7e590 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Wed, 16 Jan 2019 11:22:26 +0200 Subject: [PATCH 09/11] MXS-2266: Close prepared statements with internal ID The ID used to store the prepared statements uses the internal ID and using the external ID caused unwanted memory use and a false warning. --- server/core/queryclassifier.cc | 14 ++++++++++++-- .../routing/readwritesplit/rwsplit_route_stmt.cc | 1 + 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/server/core/queryclassifier.cc b/server/core/queryclassifier.cc index 802a2fe5f..18d87d929 100644 --- a/server/core/queryclassifier.cc +++ b/server/core/queryclassifier.cc @@ -390,8 +390,18 @@ uint32_t QueryClassifier::ps_get_type(std::string id) const void QueryClassifier::ps_erase(GWBUF* buffer) { - m_ps_handles.erase(qc_mysql_extract_ps_id(buffer)); - m_sPs_manager->erase(buffer); + if (qc_mysql_is_ps_command(mxs_mysql_get_command(buffer))) + { + // Erase the type of the statement stored with the internal ID + m_sPs_manager->erase(ps_id_internal_get(buffer)); + // ... and then erase the external to internal ID mapping + m_ps_handles.erase(qc_mysql_extract_ps_id(buffer)); + } + else + { + // Not a PS command, we don't need the ID mapping + m_sPs_manager->erase(buffer); + } } bool QueryClassifier::query_type_is_read_only(uint32_t qtype) const diff --git a/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc b/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc index 65cf6092f..56fc1c2fb 100644 --- a/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc +++ b/server/modules/routing/readwritesplit/rwsplit_route_stmt.cc @@ -451,6 +451,7 @@ bool RWSplitSession::route_session_write(GWBUF* querybuf, uint8_t command, uint3 } else if (qc_query_is_type(type, QUERY_TYPE_DEALLOC_PREPARE)) { + mxb_assert(!mxs_mysql_is_ps_command(m_qc.current_route_info().command())); m_qc.ps_erase(querybuf); } From 68d33a3ae4b5c45d3ccda7b90b9b6d1908f74482 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Wed, 16 Jan 2019 11:30:52 +0200 Subject: [PATCH 10/11] MXS-2266: Add test case Extended the existing tests to cover MXS-2266. --- maxscale-system-test/CMakeLists.txt | 2 +- maxscale-system-test/binary_ps.cpp | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/maxscale-system-test/CMakeLists.txt b/maxscale-system-test/CMakeLists.txt index da04de531..cfce1f6e3 100644 --- a/maxscale-system-test/CMakeLists.txt +++ b/maxscale-system-test/CMakeLists.txt @@ -455,7 +455,7 @@ add_test_executable(mxs359_read_only.cpp mxs359_read_only mxs359_read_only LABEL # Test master_failure_mode=error_on_write and master replacement add_test_executable(mxs359_error_on_write.cpp mxs359_error_on_write mxs359_error_on_write LABELS readwritesplit REPL_BACKEND) -# Binary protocol prepared statement tests +# Binary protocol prepared statement tests (also tests MXS-2266) add_test_executable(binary_ps.cpp binary_ps replication LABELS readwritesplit LIGHT REPL_BACKEND) add_test_executable(binary_ps_cursor.cpp binary_ps_cursor replication LABELS readwritesplit LIGHT REPL_BACKEND) diff --git a/maxscale-system-test/binary_ps.cpp b/maxscale-system-test/binary_ps.cpp index 137f903f9..bd513263e 100644 --- a/maxscale-system-test/binary_ps.cpp +++ b/maxscale-system-test/binary_ps.cpp @@ -46,6 +46,7 @@ int main(int argc, char** argv) test.add_result(strcmp(buffer, server_id[0]), "Expected server_id '%s', got '%s'", server_id[0], buffer); mysql_stmt_close(stmt); + stmt = mysql_stmt_init(test.maxscales->conn_rwsplit[0]); // Execute read, should return a slave server ID @@ -66,5 +67,8 @@ int main(int argc, char** argv) test.maxscales->close_maxscale_connections(0); + // MXS-2266: COM_STMT_CLOSE causes a warning to be logged + test.log_excludes(0, "Closing unknown prepared statement"); + return test.global_result; } From 519cd7d889225c2de5e8dec9ffdfa5e2902040f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Wed, 16 Jan 2019 16:00:32 +0200 Subject: [PATCH 11/11] Don't call python3 directly Calling it directly from the script adds a dependency on it and we don't what that. --- script/maxscale_generate_support_info.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/script/maxscale_generate_support_info.py b/script/maxscale_generate_support_info.py index a99ca1523..3eb73d22c 100755 --- a/script/maxscale_generate_support_info.py +++ b/script/maxscale_generate_support_info.py @@ -1,4 +1,4 @@ -#!/usr/bin/python3 +#!/usr/bin/env python3 # Copyright (c) 2019 MariaDB Corporation Ab #