From 630ba196bc9f5bc6c116ca7d5134e91ff4015cb1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Fri, 20 Dec 2024 12:50:05 +0200 Subject: [PATCH 1/3] Fix write_high_water and writeq_low_water defaults The documented default values were wrong. --- Documentation/Getting-Started/Configuration-Guide.md | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/Documentation/Getting-Started/Configuration-Guide.md b/Documentation/Getting-Started/Configuration-Guide.md index 52d406e22c..ac69771b08 100644 --- a/Documentation/Getting-Started/Configuration-Guide.md +++ b/Documentation/Getting-Started/Configuration-Guide.md @@ -1390,13 +1390,12 @@ the MaxScale log. - **Type**: [size](#size) - **Mandatory**: No - **Dynamic**: Yes -- **Default**: `16Mi` +- **Default**: `65536` High water mark for network write buffer. When the size of the outbound network buffer in MaxScale for a single connection exceeds this value, network traffic -throtting for that connection is started. The parameter accepts -[size type values](#sizes). The default value is 65536 bytes (was 16777216 bytes -before 22.08.4). +throtting for that connection is started. The parameter accepts [size type +values](#sizes). The default value was 16777216 bytes before 22.08.4. More specifically, if the client side write queue is above this value, it will block traffic coming from backend servers. If the backend side write queue is @@ -1416,12 +1415,12 @@ to 0. - **Type**: [size](#size) - **Mandatory**: No - **Dynamic**: Yes -- **Default**: `8Ki` +- **Default**: `1024` Low water mark for network write buffer. Once the traffic throttling is enabled, it will only be disabled when the network write buffer is below `writeq_low_water` bytes. The parameter accepts [size type values](#sizes). The -default value is 1024 bytes (was 8192 bytes before 22.08.4). +default value was 8192 bytes before 22.08.4. The value of `writeq_high_water` must always be greater than the value of `writeq_low_water`. From 5e16083743865d800265e9ce28a610caaeddb2e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 30 Dec 2024 12:50:31 +0200 Subject: [PATCH 2/3] MXS-5439: Check for EAGAIN in connect() According to the manpage, this is what's returned for UNIX domain sockets. --- server/core/dcb.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/server/core/dcb.cc b/server/core/dcb.cc index 45befc99a1..0f733f6691 100644 --- a/server/core/dcb.cc +++ b/server/core/dcb.cc @@ -105,8 +105,9 @@ int connect_socket(const char* host, int port) struct sockaddr_storage addr = {}; int so; size_t sz; + bool is_unix_socket = host[0] == '/'; - if (host[0] == '/') + if (is_unix_socket) { so = open_unix_socket(MXS_SOCKET_NETWORK, (struct sockaddr_un*)&addr, host); sz = sizeof(sockaddr_un); @@ -119,7 +120,8 @@ int connect_socket(const char* host, int port) if (so != -1) { - if (::connect(so, (struct sockaddr*)&addr, sz) == -1 && errno != EINPROGRESS) + if (::connect(so, (struct sockaddr*)&addr, sz) == -1 + && ((is_unix_socket && errno != EAGAIN) || (!is_unix_socket && errno != EINPROGRESS))) { MXS_ERROR("Failed to connect backend server [%s]:%d due to: %d, %s.", host, port, errno, mxb_strerror(errno)); From 56a205538636f2de72f41cf9b16bfffb8b453728 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Tue, 31 Dec 2024 07:30:12 +0200 Subject: [PATCH 3/3] MXS-5432: Never discard queued packets This is not safe to do as the upper layer still expects responses for the commands, even if they are going to be ignored. A potential solution to this kind of a situation would be to replace the COM_STMT_PREPARE with a COM_QUERY that generates a response but does nothing, for example DO 1, but this would be more complex than the current approach of just discarding connections that lag behind too much. Added a test case that reproduces the problem and verifies that removing the packet discarding fixes the problem. --- .../protocol/MariaDB/mariadb_backend.cc | 20 --------- system-test/CMakeLists.txt | 3 ++ .../maxscale.cnf.template.mxs5432_queued_ps | 25 +++++++++++ system-test/mxs5432_queued_ps.cc | 44 +++++++++++++++++++ 4 files changed, 72 insertions(+), 20 deletions(-) create mode 100644 system-test/cnf/maxscale.cnf.template.mxs5432_queued_ps create mode 100644 system-test/mxs5432_queued_ps.cc diff --git a/server/modules/protocol/MariaDB/mariadb_backend.cc b/server/modules/protocol/MariaDB/mariadb_backend.cc index d78ff0b0d3..be13ac7c30 100644 --- a/server/modules/protocol/MariaDB/mariadb_backend.cc +++ b/server/modules/protocol/MariaDB/mariadb_backend.cc @@ -2949,26 +2949,6 @@ MariaDBBackendConnection::StateMachineRes MariaDBBackendConnection::authenticate void MariaDBBackendConnection::store_delayed_packet(mxs::Buffer buffer) { uint8_t cmd = mxs_mysql_get_command(buffer.get()); - - if (cmd == MXS_COM_STMT_CLOSE && !m_delayed_packets.empty()) - { - // This'll ignore MARIADB_PS_DIRECT_EXEC_ID which could also be handled by popping - // the last command if it's a COM_STMT_PREPARE. This is unlikely to happen as no - // connector is known to behave like this. - uint32_t ps_id = mxs_mysql_extract_ps_id(buffer.get()); - auto it = std::find_if(m_delayed_packets.begin(), m_delayed_packets.end(), [&](const auto& buffer){ - return buffer.id() == ps_id; - }); - - if (it != m_delayed_packets.end()) - { - MXB_INFO("COM_STMT_CLOSE refers to COM_STMT_PREPARE with ID %u that is " - "queued for execution, removing both from the queue.", ps_id); - m_delayed_packets.erase(it); - return; - } - } - m_delayed_packets.emplace_back(std::move(buffer)); MXB_INFO("Storing %s while in state '%s', %lu packet(s) queued: %s", diff --git a/system-test/CMakeLists.txt b/system-test/CMakeLists.txt index b7b8ef9238..bcf5b4ec9b 100644 --- a/system-test/CMakeLists.txt +++ b/system-test/CMakeLists.txt @@ -942,6 +942,9 @@ add_test_executable(mxs5302_ps_overflow.cc mxs5302_ps_overflow mxs5302_ps_overfl # MXS-5415: retry_failed_reads is not affected by delayed_retry_timeout add_test_executable(mxs5415_retry_failed_reads_timeout.cc mxs5415_retry_failed_reads_timeout mxs5415_retry_failed_reads_timeout LABELS REPL_BACKEND readwritesplit) +# MXS-5432: Executing prepared statements too fast may stall query execution +add_test_executable(mxs5432_queued_ps.cc mxs5432_queued_ps mxs5432_queued_ps LABELS REPL_BACKEND readwritesplit) + ############################################ # END: Normal tests # ############################################ diff --git a/system-test/cnf/maxscale.cnf.template.mxs5432_queued_ps b/system-test/cnf/maxscale.cnf.template.mxs5432_queued_ps new file mode 100644 index 0000000000..c6918e191d --- /dev/null +++ b/system-test/cnf/maxscale.cnf.template.mxs5432_queued_ps @@ -0,0 +1,25 @@ +[maxscale] +threads=###threads### +log_info=1 + +###server### + +[Monitor] +type=monitor +module=mariadbmon +servers=server1,server2 +user=maxskysql +password=skysql +monitor_interval=1s + +[Router] +type=service +router=readwritesplit +servers=server1,server2 +user=maxservice +password=maxservice + +[Listener] +type=listener +service=Router +port=4006 diff --git a/system-test/mxs5432_queued_ps.cc b/system-test/mxs5432_queued_ps.cc new file mode 100644 index 0000000000..c5ce9f3dac --- /dev/null +++ b/system-test/mxs5432_queued_ps.cc @@ -0,0 +1,44 @@ +/* + * Copyright (c) 2024 MariaDB plc + * + * Use of this software is governed by the Business Source License included + * in the LICENSE.TXT file and at www.mariadb.com/bsl11. + * + * Change Date: 2027-04-10 + * + * On the date above, in accordance with the Business Source License, use + * of this software will be governed by version 2 or later of the General + * Public License. + */ + +#include + +void test_main(TestConnections& test) +{ + auto c = test.maxscale->rwsplit(); + test.expect(c.connect(), "Failed to connect: %s", c.error()); + auto master_id = c.field("SELECT @@server_id, @@last_insert_id"); + + for (int i = 0; i < 10; i++) + { + std::string slow_query = "SET @a=(SELECT SLEEP(CASE @@server_id WHEN " + master_id + + " THEN 0 ELSE 1 END))"; + test.expect(c.query(slow_query), "Failed to execute SET: %s", c.error()); + + for (int j = 0; j < 10; j++) + { + MYSQL_STMT* stmt = c.stmt(); + std::string ps_query = "SELECT 1"; + test.expect(mysql_stmt_prepare(stmt, ps_query.c_str(), ps_query.length()) == 0, + "Prepare of '%s' failed: %s", ps_query.c_str(), mysql_stmt_error(stmt)); + mysql_stmt_close(stmt); + } + + test.expect(c.query("SELECT @@server_id"), "Failed to execute SELECT: %s", c.error()); + } +} + +int main(int argc, char** argv) +{ + return TestConnections().run_test(argc, argv, test_main); +}