From 7270ff7ac0202c11c91dbe8165c89d9c410f4c7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Thu, 4 Jul 2019 08:22:57 +0300 Subject: [PATCH 1/8] Use 10.3 for REST API and MaxCtrl tests --- test/docker-compose.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/docker-compose.yml b/test/docker-compose.yml index da79ca1cf..bd2f4c92b 100644 --- a/test/docker-compose.yml +++ b/test/docker-compose.yml @@ -1,7 +1,7 @@ version: '2' services: server1: - image: mariadb:10.2 + image: mariadb:10.3 network_mode: "host" container_name: server1 environment: @@ -11,7 +11,7 @@ services: command: mysqld --log-bin=binlog --binlog-format=ROW --server-id=3000 --port=3000 --log-slave-updates server2: - image: mariadb:10.2 + image: mariadb:10.3 container_name: server2 network_mode: "host" environment: @@ -21,7 +21,7 @@ services: command: mysqld --log-bin=binlog --binlog-format=ROW --server-id=3001 --port=3001 --log-slave-updates server3: - image: mariadb:10.2 + image: mariadb:10.3 container_name: server3 network_mode: "host" environment: @@ -31,7 +31,7 @@ services: command: mysqld --log-bin=binlog --binlog-format=ROW --server-id=3002 --port=3002 --log-slave-updates server4: - image: mariadb:10.2 + image: mariadb:10.3 container_name: server4 network_mode: "host" environment: From e516c11ac57f3afce28bd5d8a90a02bf82eba2e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Thu, 4 Jul 2019 08:38:31 +0300 Subject: [PATCH 2/8] MXS-2587: Never route stored queries in routeQuery This could end up in infinite mutual recursion if no responses are expected. Although this does not happen now that MXS-2587 is fixed, the code should not even be there. --- .../routing/readwritesplit/rwsplitsession.cc | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/server/modules/routing/readwritesplit/rwsplitsession.cc b/server/modules/routing/readwritesplit/rwsplitsession.cc index 2c49c068e..e92d89db1 100644 --- a/server/modules/routing/readwritesplit/rwsplitsession.cc +++ b/server/modules/routing/readwritesplit/rwsplitsession.cc @@ -175,24 +175,15 @@ int32_t RWSplitSession::routeQuery(GWBUF* querybuf) } else { - /** - * We are already processing a request from the client. Store the - * new query and wait for the previous one to complete. - */ - mxb_assert(m_expected_responses > 0 || !m_query_queue.empty()); + // We are already processing a request from the client. Store the new query and wait for the previous + // one to complete. MXS_INFO("Storing query (len: %d cmd: %0x), expecting %d replies to current command", - gwbuf_length(querybuf), - GWBUF_DATA(querybuf)[4], - m_expected_responses); + gwbuf_length(querybuf), GWBUF_DATA(querybuf)[4], m_expected_responses); + mxb_assert(m_expected_responses > 0 || !m_query_queue.empty()); m_query_queue.emplace_back(querybuf); querybuf = NULL; rval = 1; mxb_assert(m_expected_responses != 0); - - if (m_expected_responses == 0 && !route_stored_query()) - { - rval = 0; - } } if (querybuf != NULL) From 2238faa9132154163fe55be992721412fa591e03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Sun, 7 Jul 2019 21:20:28 +0300 Subject: [PATCH 3/8] Make response time lock server-specific There's no global data being modified inside the method call so a instance level lock is sufficient. --- server/core/server.cc | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/server/core/server.cc b/server/core/server.cc index 6b7ff6678..43fd5d4e4 100644 --- a/server/core/server.cc +++ b/server/core/server.cc @@ -1550,18 +1550,9 @@ bool server_set_disk_space_threshold(SERVER* server, const char* disk_space_thre return rv; } -namespace -{ -std::mutex ave_write_mutex; -} - void server_add_response_average(SERVER* srv, double ave, int num_samples) { - Server* server = static_cast(srv); - - - std::lock_guard guard(ave_write_mutex); - server->response_time_add(ave, num_samples); + static_cast(srv)->response_time_add(ave, num_samples); } int server_response_time_num_samples(const SERVER* srv) @@ -1589,9 +1580,11 @@ double server_response_time_average(const SERVER* srv) void Server::response_time_add(double ave, int num_samples) { constexpr double drift {1.1}; - int current_max = m_response_time.sample_max(); int new_max {0}; + std::lock_guard guard(m_lock); + int current_max = m_response_time.sample_max(); + // This server handles more samples than EMA max. // Increasing max allows all servers to be fairly compared. if (num_samples >= current_max) From 1c96dc9769e0c6873ec6dab83a53cc66af7de423 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 8 Jul 2019 08:17:48 +0300 Subject: [PATCH 4/8] Update download link --- Documentation/Release-Notes/generate_release_notes.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/Release-Notes/generate_release_notes.sh b/Documentation/Release-Notes/generate_release_notes.sh index a2b14160b..7c92ce782 100755 --- a/Documentation/Release-Notes/generate_release_notes.sh +++ b/Documentation/Release-Notes/generate_release_notes.sh @@ -31,7 +31,7 @@ For more information, please refer to the [Limitations](../About/Limitations.md) RPM and Debian packages are provided for supported the Linux distributions. -Packages can be downloaded [here](https://mariadb.com/downloads/mariadb-tx/maxscale). +Packages can be downloaded [here](https://mariadb.com/downloads/#mariadb_platform-mariadb_maxscale). ## Source Code From e658dca4feaa9aa8493f4ba4db73a0efcd640cd6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 8 Jul 2019 11:31:09 +0300 Subject: [PATCH 5/8] Allow rapid reloading of users on startup This fixes the test failures that stem from users being created right after maxscale has started. This also should make startups a bit smoother now that the default value of users_refresh_time has been fixed. --- server/core/service.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/server/core/service.cc b/server/core/service.cc index ad8556e80..e41b40e2f 100644 --- a/server/core/service.cc +++ b/server/core/service.cc @@ -1433,8 +1433,10 @@ bool Service::refresh_users() MXS_CONFIG* config = config_get_global_options(); - /* Check if refresh rate limit has been exceeded */ - if (now < m_rate_limits[self].last + config->users_refresh_time) + /* Check if refresh rate limit has been exceeded. Also check whether we are in the middle of starting up. + * If so, allow repeated reloading of users. */ + if (now > maxscale_started() + config->users_refresh_time + && now < m_rate_limits[self].last + config->users_refresh_time) { if (!m_rate_limits[self].warned) { From 1b69e659dbdc4fdb417a46144bc3fbbaed7a8946 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 8 Jul 2019 12:18:22 +0300 Subject: [PATCH 6/8] Assert DCB ownership in dcb_drain_writeq This will cause EPOLLOUT events for maxscaled to trigger the assert immediately if the buffer was placed into the queue by another worker. --- server/core/dcb.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/server/core/dcb.cc b/server/core/dcb.cc index a75184fe6..c9af9a270 100644 --- a/server/core/dcb.cc +++ b/server/core/dcb.cc @@ -1006,6 +1006,8 @@ static void dcb_log_write_failure(DCB* dcb, GWBUF* queue, int eno) */ int dcb_drain_writeq(DCB* dcb) { + mxb_assert(dcb->poll.owner == RoutingWorker::get_current()); + if (dcb->ssl_read_want_write) { /** The SSL library needs to write more data */ From 5aa9daaeea57eebd890941aa866ada17e580ba36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Tue, 9 Jul 2019 10:44:03 +0300 Subject: [PATCH 7/8] Improve stack trace printing The stacktrace now removes leading paths from object files and common source code prefixes. It also skips the first five frames which are always inside the stacktrace printing code and the signal handler. --- maxutils/maxbase/src/stacktrace.cc | 41 +++++++++++++++++++++++++++--- 1 file changed, 37 insertions(+), 4 deletions(-) diff --git a/maxutils/maxbase/src/stacktrace.cc b/maxutils/maxbase/src/stacktrace.cc index 2afc3e477..8f52709a3 100644 --- a/maxutils/maxbase/src/stacktrace.cc +++ b/maxutils/maxbase/src/stacktrace.cc @@ -56,7 +56,7 @@ static void get_command_output(char* output, size_t size, const char* format, .. } } -static void extract_file_and_line(const char* symbols, char* cmd, size_t size) +static void extract_file_and_line(char* symbols, char* cmd, size_t size) { const char* filename_end = strchr(symbols, '('); const char* symname_end = strchr(symbols, ')'); @@ -116,6 +116,30 @@ static void extract_file_and_line(const char* symbols, char* cmd, size_t size) snprintf(symname, sizeof(symname), "%.*s", (int)(symname_end - symname_start), symname_start); get_command_output(cmd, size, "addr2line -e %s %s", filename, symname); } + + const char prefix[] = "MaxScale/"; + + // Remove common source prefix + if (char* str = strstr(cmd, prefix)) + { + str += sizeof(prefix) - 1; + memmove(cmd, str, strlen(cmd) - (str - cmd) + 1); + } + + // Strip the directory name from the symbols (could this be useful?) + if (char* str = strrchr(symbols, '/')) + { + ++str; + memmove(symbols, str, strlen(symbols) - (str - symbols) + 1); + } + + // Remove the address where the symbol is in memory (i.e. the [0xdeadbeef] that follows the + // (main+0xa1) part), we're only interested where it is in the library. + if (char* str = strchr(symbols, '[')) + { + str--; + *str = '\0'; + } } } } @@ -129,12 +153,21 @@ void dump_stacktrace(std::function handler) int count = backtrace(addrs, 128); char** symbols = backtrace_symbols(addrs, count); + int rc = system("/bin/test -f /bin/nm -a -f /bin/addr2line"); + bool do_extract = WIFEXITED(rc) && WEXITSTATUS(rc) == 0; + if (symbols) { - for (int n = 0; n < count; n++) + // Skip first five frames, they are inside the stacktrace printing function and signal handlers + for (int n = 4; n < count; n++) { - char cmd[PATH_MAX + 1024] = ""; - extract_file_and_line(symbols[n], cmd, sizeof(cmd)); + char cmd[PATH_MAX + 1024] = ""; + + if (do_extract) + { + extract_file_and_line(symbols[n], cmd, sizeof(cmd)); + } + handler(symbols[n], cmd); } free(symbols); From 8a176d64aa94454ef50cbb52dd9719fd577d3037 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Tue, 9 Jul 2019 14:59:52 +0300 Subject: [PATCH 8/8] MXS-2490: Add direct execution support Certain MariaDB connectors will use the direct execution for batching COM_STMT_PREPARE and COM_STMT_EXECUTE execution without waiting for the COM_STMT_PREPARE to complete. In these cases the COM_STMT_EXECUTE (and other COM_STMT commands as well) will use the special ID 0xffffffff. When this is detected, it should be substituted with the ID of the latest statement that was prepared. --- include/maxscale/queryclassifier.hh | 5 +++- maxscale-system-test/CMakeLists.txt | 3 +++ .../mxs2490_ps_execute_direct.cpp | 26 +++++++++++++++++++ server/core/queryclassifier.cc | 8 ++++++ 4 files changed, 41 insertions(+), 1 deletion(-) create mode 100644 maxscale-system-test/mxs2490_ps_execute_direct.cpp diff --git a/include/maxscale/queryclassifier.hh b/include/maxscale/queryclassifier.hh index 78ea4fed6..c4a6426b5 100644 --- a/include/maxscale/queryclassifier.hh +++ b/include/maxscale/queryclassifier.hh @@ -363,7 +363,7 @@ private: */ bool query_type_is_read_only(uint32_t qtype) const; - void process_routing_hints(HINT* pHints, uint32_t* target); + void process_routing_hints(HINT* pHints, uint32_t* target); uint32_t get_route_target(uint8_t command, uint32_t qtype); MXS_SESSION* session() const @@ -415,5 +415,8 @@ private: RouteInfo m_route_info; bool m_trx_is_read_only; bool m_ps_continuation; + + uint32_t m_prev_ps_id = 0; /**< For direct PS execution, storest latest prepared PS ID. + * https://mariadb.com/kb/en/library/com_stmt_execute/#statement-id **/ }; } diff --git a/maxscale-system-test/CMakeLists.txt b/maxscale-system-test/CMakeLists.txt index a8ded287f..e07a028ef 100644 --- a/maxscale-system-test/CMakeLists.txt +++ b/maxscale-system-test/CMakeLists.txt @@ -969,6 +969,9 @@ add_test_executable(mxs2563_concurrent_slave_failure.cpp mxs2563_concurrent_slav # MXS-2521: COM_STMT_EXECUTE maybe return empty result add_test_executable(mxs2521_double_exec.cpp mxs2521_double_exec mxs2521_double_exec LABELS REPL_BACKEND readwritesplit) +# MXS-2490: Direct execution doesn't work with MaxScale +add_test_executable(mxs2490_ps_execute_direct.cpp mxs2490_ps_execute_direct replication LABELS REPL_BACKEND readwritesplit) + ############################################ # BEGIN: binlogrouter and avrorouter tests # ############################################ diff --git a/maxscale-system-test/mxs2490_ps_execute_direct.cpp b/maxscale-system-test/mxs2490_ps_execute_direct.cpp new file mode 100644 index 000000000..6e5735ae5 --- /dev/null +++ b/maxscale-system-test/mxs2490_ps_execute_direct.cpp @@ -0,0 +1,26 @@ +/** + * MXS-2490: Unknown prepared statement handler (0) given to mysqld_stmt_execute + * + * See: + * + * https://mariadb.com/kb/en/library/mariadb_stmt_execute_direct/ + * https://mariadb.com/kb/en/library/com_stmt_execute/#statement-id + */ + +#include "testconnections.h" + +int main(int argc, char** argv) +{ + TestConnections test(argc, argv); + + test.set_timeout(30); + test.maxscales->connect(); + + MYSQL_STMT* stmt = mysql_stmt_init(test.maxscales->conn_rwsplit[0]); + std::string query = "SELECT user FROM mysql.user"; + test.expect(mariadb_stmt_execute_direct(stmt, query.c_str(), query.length()) == 0, + "execute_direct should work: %s", mysql_stmt_error(stmt)); + mysql_stmt_close(stmt); + + return test.global_result; +} diff --git a/server/core/queryclassifier.cc b/server/core/queryclassifier.cc index 918db76fa..b8beb4481 100644 --- a/server/core/queryclassifier.cc +++ b/server/core/queryclassifier.cc @@ -644,6 +644,13 @@ uint32_t QueryClassifier::ps_id_internal_get(GWBUF* pBuffer) // All COM_STMT type statements store the ID in the same place uint32_t external_id = mysql_extract_ps_id(pBuffer); + + if (external_id == 0xffffffff) + { + // "Direct execution" that refers to the latest prepared statement + external_id = m_prev_ps_id; + } + auto it = m_ps_handles.find(external_id); if (it != m_ps_handles.end()) @@ -663,6 +670,7 @@ uint32_t QueryClassifier::ps_id_internal_get(GWBUF* pBuffer) void QueryClassifier::ps_store_response(uint32_t internal_id, GWBUF* buffer) { auto external_id = qc_mysql_extract_ps_id(buffer); + m_prev_ps_id = external_id; m_ps_handles[external_id] = internal_id; if (auto param_count = qc_extract_ps_param_count(buffer))