From b46974f3e742879b9f031a427b96b001364cf39d Mon Sep 17 00:00:00 2001 From: Timofey Turenko Date: Thu, 23 Jan 2020 14:49:18 +0200 Subject: [PATCH 1/5] Fix Galera lib seach in mariadb_nodes.cpp --- maxscale-system-test/mariadb_nodes.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/maxscale-system-test/mariadb_nodes.cpp b/maxscale-system-test/mariadb_nodes.cpp index 66af629a5..c5fdc3361 100644 --- a/maxscale-system-test/mariadb_nodes.cpp +++ b/maxscale-system-test/mariadb_nodes.cpp @@ -437,7 +437,7 @@ int Galera_nodes::start_galera() 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/*", + "sed -i \"s|###GALERA-LIB-PATH###|$(ls /usr/lib*/galera*/*.so)|g\" /etc/my.cnf.d/* /etc/mysql/my.cnf.d/*", IP_private[i]); } From 4f1ae70765700daeceaaacc2f737a7f94ad11203 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 30 Dec 2019 10:58:01 +0200 Subject: [PATCH 2/5] Allow multiple fatal signals As long as the same thread never handles more than one fatal signal, multiple fatal signals can be processed. This should guarantee that the stacktrace is printed into the log while guaranteeing that recursion never takes place if the handling of a fatal signal causes a fatal signal to be emitted. --- server/core/gateway.cc | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/server/core/gateway.cc b/server/core/gateway.cc index 17809e353..7da83efc3 100644 --- a/server/core/gateway.cc +++ b/server/core/gateway.cc @@ -402,11 +402,18 @@ static int signal_set(int sig, void (* handler)(int)); static void sigfatal_handler(int i) { - // The same signal being handled *now* can occur in another thread (and is often likely). - // By setting the default handler here we will always get a core, but not necessarily - // the backtrace into the log file. This should be overhauled to proper signal handling - // (MXS-599). - signal_set(i, SIG_DFL); + thread_local std::thread::id current_id; + std::thread::id no_id; + + if (current_id != no_id) + { + // Fatal error when processing a fatal error. + // TODO: This should be overhauled to proper signal handling (MXS-599). + signal_set(i, SIG_DFL); + raise(i); + } + + current_id = std::this_thread::get_id(); MXS_CONFIG* cnf = config_get_global_options(); fprintf(stderr, @@ -444,6 +451,7 @@ static void sigfatal_handler(int i) /* re-raise signal to enforce core dump */ fprintf(stderr, "\n\nWriting core dump\n"); + signal_set(i, SIG_DFL); raise(i); } From c04d6748d314f8032ac88c628766b63c00c18725 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Tue, 21 Jan 2020 11:33:04 +0200 Subject: [PATCH 3/5] Fix debug assertion on inconsistent sescmd result The slave backend would be closed twice if it would both respond with a different result and be closed due to a hangup before the master responded. Added a test case that reproduced the problem. --- maxscale-system-test/CMakeLists.txt | 3 + .../maxscale.cnf.template.crash_on_bad_sescmd | 49 +++++++++++++ maxscale-system-test/crash_on_bad_sescmd.cpp | 69 +++++++++++++++++++ .../readwritesplit/rwsplit_session_cmd.cc | 2 +- 4 files changed, 122 insertions(+), 1 deletion(-) create mode 100644 maxscale-system-test/cnf/maxscale.cnf.template.crash_on_bad_sescmd create mode 100644 maxscale-system-test/crash_on_bad_sescmd.cpp diff --git a/maxscale-system-test/CMakeLists.txt b/maxscale-system-test/CMakeLists.txt index 3440f5f70..b30908f12 100644 --- a/maxscale-system-test/CMakeLists.txt +++ b/maxscale-system-test/CMakeLists.txt @@ -946,6 +946,9 @@ add_test_executable(mxs2115_version_string.cpp mxs2115_version_string replicatio # MXS-2295: COM_CHANGE_USER does not clear out session command history add_test_executable(mxs2295_change_user_loop.cpp mxs2295_change_user_loop mxs2295_change_user_loop LABELS REPL_BACKEND) +# Debug assertion due to double-closed when a slave's response differs from the master +add_test_executable(crash_on_bad_sescmd.cpp crash_on_bad_sescmd crash_on_bad_sescmd LABELS readwritesplit REPL_BACKEND) + # MXS-2300: Prune session command history add_test_executable(mxs2300_history_pruning.cpp mxs2300_history_pruning mxs2300_history_pruning LABELS readwritesplit REPL_BACKEND) diff --git a/maxscale-system-test/cnf/maxscale.cnf.template.crash_on_bad_sescmd b/maxscale-system-test/cnf/maxscale.cnf.template.crash_on_bad_sescmd new file mode 100644 index 000000000..458d371db --- /dev/null +++ b/maxscale-system-test/cnf/maxscale.cnf.template.crash_on_bad_sescmd @@ -0,0 +1,49 @@ +[maxscale] +threads=auto + +[server1] +type=server +address=###node_server_IP_1### +port=###node_server_port_1### +protocol=MySQLBackend + +[server2] +type=server +address=###node_server_IP_2### +port=###node_server_port_2### +protocol=MySQLBackend + +[server3] +type=server +address=###node_server_IP_3### +port=###node_server_port_3### +protocol=MySQLBackend + +[server4] +type=server +address=###node_server_IP_4### +port=###node_server_port_4### +protocol=MySQLBackend + +[MySQL Monitor] +type=monitor +module=mysqlmon +servers=server1,server2,server3,server4 +user=maxskysql +password=skysql +monitor_interval=1000 + +[RW Split Router] +type=service +router=readwritesplit +servers=server1,server2,server3,server4 +user=maxskysql +password=skysql +max_sescmd_history=20 +disable_sescmd_history=false + +[RW Split Listener] +type=listener +service=RW Split Router +protocol=MySQLClient +port=4006 diff --git a/maxscale-system-test/crash_on_bad_sescmd.cpp b/maxscale-system-test/crash_on_bad_sescmd.cpp new file mode 100644 index 000000000..4ee88315d --- /dev/null +++ b/maxscale-system-test/crash_on_bad_sescmd.cpp @@ -0,0 +1,69 @@ +/** + * Double-close on bad session command result + */ + +#include "testconnections.h" + +void run_test(TestConnections& test) +{ + Connection conn = test.maxscales->rwsplit(); + conn.connect(); + + for (int i = 0; i <= 300 && test.global_result == 0; i++) + { + if (conn.query("SET @a = 1") + && conn.query("USE test") + && conn.query("SET SQL_MODE=''") + && conn.query("USE test") + && conn.query("SELECT @@last_insert_id") + && conn.query("SELECT 1") + && conn.query("USE test") + && conn.query("SELECT 1") + && conn.query("SET @a = 123") + && conn.query("BEGIN") + && conn.query("SELECT @a") + && conn.query("COMMIT") + && conn.query("SET @a = 321") + && conn.query("SELECT @a") + && conn.query("SET @a = 456") + && conn.query("START TRANSACTION READ ONLY") + && conn.query("SELECT @a") + && conn.query("COMMIT") + && conn.query("PREPARE ps FROM 'SELECT 1'") + && conn.query("EXECUTE ps") + && conn.query("DEALLOCATE PREPARE ps")) + { + conn.reset_connection(); + } + else + { + break; + } + } +} + +int main(int argc, char* argv[]) +{ + TestConnections test(argc, argv); + + std::vector threads; + + for (int i = 0; i < 5; i++) + { + threads.emplace_back(run_test, std::ref(test)); + } + + for (int i = 0; i < 5; i++) + { + test.repl->stop_node(1 + i % 3); + test.repl->start_node(1 + i % 3); + sleep(1); + } + + for (auto& a : threads) + { + a.join(); + } + + return test.global_result; +} diff --git a/server/modules/routing/readwritesplit/rwsplit_session_cmd.cc b/server/modules/routing/readwritesplit/rwsplit_session_cmd.cc index fce3ad819..7726b9985 100644 --- a/server/modules/routing/readwritesplit/rwsplit_session_cmd.cc +++ b/server/modules/routing/readwritesplit/rwsplit_session_cmd.cc @@ -65,7 +65,7 @@ static void discard_if_response_differs(SRWBackend backend, uint8_t slave_response, SSessionCommand sescmd) { - if (master_response != slave_response) + if (master_response != slave_response && backend->in_use()) { uint8_t cmd = sescmd->get_command(); std::string query = sescmd->to_string(); From 6dd4a04c5d3eb27cab4197789cf34feabd7e16c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Fri, 13 Dec 2019 08:34:55 +0200 Subject: [PATCH 4/5] cherry-pick: MXS-2803: Write all buffered data Since the queued queries will never be inspected after the COM_CHANGE_USER completes, they should all be written instead of only the first packet. --- server/modules/protocol/MySQL/mariadbbackend/mysql_backend.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/modules/protocol/MySQL/mariadbbackend/mysql_backend.cc b/server/modules/protocol/MySQL/mariadbbackend/mysql_backend.cc index 87fe9b456..7b8f4259a 100644 --- a/server/modules/protocol/MySQL/mariadbbackend/mysql_backend.cc +++ b/server/modules/protocol/MySQL/mariadbbackend/mysql_backend.cc @@ -945,7 +945,7 @@ static int gw_read_and_write(DCB* dcb) if (proto->ignore_replies > 0) { /** The reply to a COM_CHANGE_USER is in packet */ - GWBUF* query = modutil_get_next_MySQL_packet(&proto->stored_query); + GWBUF* query = proto->stored_query; proto->stored_query = NULL; proto->ignore_replies--; mxb_assert(proto->ignore_replies >= 0); From 4641dc208f1f4e94c6b2962076593215a8927334 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Tue, 21 Jan 2020 14:55:14 +0200 Subject: [PATCH 5/5] Fix sescmd debug assert The assertion will not hold for COM_CHANGE_USER. This prevents the debug assertion but the actual backend code should also be changed. --- server/modules/routing/readwritesplit/rwsplit_session_cmd.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/modules/routing/readwritesplit/rwsplit_session_cmd.cc b/server/modules/routing/readwritesplit/rwsplit_session_cmd.cc index 7726b9985..2c462d2a7 100644 --- a/server/modules/routing/readwritesplit/rwsplit_session_cmd.cc +++ b/server/modules/routing/readwritesplit/rwsplit_session_cmd.cc @@ -86,7 +86,6 @@ void RWSplitSession::process_sescmd_response(SRWBackend& backend, GWBUF** ppPack { if (backend->has_session_commands()) { - mxb_assert(GWBUF_IS_COLLECTED_RESULT(*ppPacket)); uint8_t cmd; gwbuf_copy_data(*ppPacket, MYSQL_HEADER_LEN, 1, &cmd); uint8_t command = backend->next_session_command()->get_command(); @@ -95,6 +94,8 @@ void RWSplitSession::process_sescmd_response(SRWBackend& backend, GWBUF** ppPack MXS_PS_RESPONSE resp = {}; bool discard = true; + mxb_assert(GWBUF_IS_COLLECTED_RESULT(*ppPacket) || command == MXS_COM_CHANGE_USER); + if (command == MXS_COM_STMT_PREPARE && cmd != MYSQL_REPLY_ERR) { // This should never fail or the backend protocol is broken