BUG#35356006 dba.rebootclusterfromcompleteoutage failed with default group_concat_max_len values

joaosigma · joaosigma · commit 244e5d010e8e · 2023-06-04T11:24:27.000+01:00
The dba.rebootclusterfromcompleteoutage needs to query the complete set
of GTIDs of a specific channel, and the SQL query that does that uses
GROUP_CONCAT function to aggregate results. However, the query can fail
if the limit set by the group_concat_max_len var is reached.

This patch created a version of the query that doesn't need to use the
GROUP_CONCAT function, which removes the issue completely. For scenarios
where it's still needed, the group_concat_max_len limit was raised to
1GB.

Change-Id: I397ddd55aece286cd7a67663ca1705306ead864a
diff --git a/modules/adminapi/common/instance_pool.cc b/modules/adminapi/common/instance_pool.cc
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2019, 2022, Oracle and/or its affiliates.
+ * Copyright (c) 2019, 2023, Oracle and/or its affiliates.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License, version 2.0,
@@ -152,6 +152,12 @@ void Instance::prepare_session() {
     set_sysvar("collation_connection", std::string("utf8mb4_general_ci"),
                mysqlshdk::mysql::Var_qualifier::SESSION);
 
+  // Make sure that the limit for 'group_concat_max_len' is increased from the
+  // default 1024 to 1GB. There's no "correct" value here, hence no constant for
+  // the variable.
+  set_sysvar("group_concat_max_len", static_cast<int64_t>(1024 * 1024 * 1024),
+             mysqlshdk::mysql::Var_qualifier::SESSION);
+
   // Cache the hostname, port, and UUID to avoid errors accessing this data if
   // the instance fails during an operation.
   get_canonical_address();
diff --git a/modules/adminapi/dba/reboot_cluster_from_complete_outage.cc b/modules/adminapi/dba/reboot_cluster_from_complete_outage.cc
@@ -382,33 +382,36 @@ Reboot_cluster_from_complete_outage::retrieve_instances(
  */
 std::shared_ptr<Instance>
 Reboot_cluster_from_complete_outage::pick_best_instance_gtid(
-    const std::vector<std::shared_ptr<Instance>> &instances, bool force,
+    const std::vector<std::shared_ptr<Instance>> &instances,
+    bool is_cluster_set_member, bool force,
     std::string_view intended_instance) {
   std::vector<Instance_gtid_info> instance_gtids;
   {
     // list of replication channel names that must be considered when comparing
     // GTID sets. With ClusterSets, the async channel for secondaries must be
     // added here.
-    const std::vector<std::string> k_known_channel_names = {
-        "group_replication_applier"};
-
     auto current_session_options = m_target_instance->get_connection_options();
 
+    std::vector<std::string> channels;
+    channels.push_back(mysqlshdk::gr::k_gr_applier_channel);
+    if (is_cluster_set_member)
+      channels.push_back(k_clusterset_async_channel_name);
+
     {
       Instance_gtid_info info;
       info.server = m_target_instance->get_canonical_address();
-      info.gtid_executed = mysqlshdk::mysql::get_total_gtid_set(
-          *m_target_instance, k_known_channel_names);
+      info.gtid_executed =
+          mysqlshdk::mysql::get_total_gtid_set(*m_target_instance, channels);
       instance_gtids.push_back(std::move(info));
     }
 
     for (const auto &i : instances) {
       Instance_gtid_info info;
       info.server = i->get_canonical_address();
-      info.gtid_executed =
-          mysqlshdk::mysql::get_total_gtid_set(*i, k_known_channel_names);
+      info.gtid_executed = mysqlshdk::mysql::get_total_gtid_set(*i, channels);
       instance_gtids.push_back(std::move(info));
     }
+
     assert(instance_gtids.size() == (instances.size() + 1));
   }
 
@@ -1178,8 +1181,9 @@ std::shared_ptr<Cluster> Reboot_cluster_from_complete_outage::do_run() {
   // pick the seed instance
   std::shared_ptr<Instance> best_instance_gtid;
   {
-    best_instance_gtid = pick_best_instance_gtid(
-        instances, m_options.get_force(), m_options.get_primary());
+    best_instance_gtid =
+        pick_best_instance_gtid(instances, m_cs_info.is_member,
+                                m_options.get_force(), m_options.get_primary());
 
     // check if the other instances are compatible in regards to GTID with the
     // (new) seed
diff --git a/modules/adminapi/dba/reboot_cluster_from_complete_outage.h b/modules/adminapi/dba/reboot_cluster_from_complete_outage.h
@@ -77,7 +77,8 @@ class Reboot_cluster_from_complete_outage {
    * explicit instance and the force option.
    */
   std::shared_ptr<Instance> pick_best_instance_gtid(
-      const std::vector<std::shared_ptr<Instance>> &instances, bool force,
+      const std::vector<std::shared_ptr<Instance>> &instances,
+      bool is_cluster_set_member, bool force,
       std::string_view intended_instance);
 
   void check_instance_configuration();
diff --git a/mysqlshdk/include/shellcore/base_session.h b/mysqlshdk/include/shellcore/base_session.h
@@ -30,6 +30,7 @@
 #include <functional>
 #include <memory>
 #include <string>
+#include <string_view>
 #include <unordered_map>
 
 #include "mysqlshdk/include/scripting/types.h"
@@ -172,7 +173,7 @@ class SHCORE_PUBLIC ShellBaseSession : public shcore::Cpp_object_bridge {
   std::function<void(const std::string &, bool exists)> update_schema_cache;
 
  protected:
-  std::string get_quoted_name(const std::string &name);
+  std::string get_quoted_name(std::string_view name);
   // TODO(rennox): Note that these are now stored on the low level session
   // object too, they should be removed from here
   mysqlshdk::db::Connection_options _connection_options;
diff --git a/mysqlshdk/libs/db/session.cc b/mysqlshdk/libs/db/session.cc
@@ -29,9 +29,9 @@
 namespace mysqlshdk {
 namespace db {
 
-Query_attribute::Query_attribute(const std::string &n,
-                                 std::unique_ptr<IQuery_attribute_value> v)
-    : name(n), value(std::move(v)) {}
+Query_attribute::Query_attribute(
+    std::string n, std::unique_ptr<IQuery_attribute_value> v) noexcept
+    : name(std::move(n)), value(std::move(v)) {}
 
 void ISession::connect(const mysqlshdk::db::Connection_options &data) {
   do_connect(data);
diff --git a/mysqlshdk/libs/db/session.h b/mysqlshdk/libs/db/session.h
@@ -79,8 +79,8 @@ struct IQuery_attribute_value {
  * IQuery_attribute_value interface is used at this level.
  */
 struct Query_attribute {
-  Query_attribute(const std::string &n,
-                  std::unique_ptr<IQuery_attribute_value> v);
+  Query_attribute(std::string n,
+                  std::unique_ptr<IQuery_attribute_value> v) noexcept;
 
   std::string name;
   std::unique_ptr<IQuery_attribute_value> value;
diff --git a/mysqlshdk/libs/mysql/replication.cc b/mysqlshdk/libs/mysql/replication.cc
@@ -392,7 +392,7 @@ std::vector<std::string> get_incoming_channel_names(
 }
 
 bool get_channel_status(const mysqlshdk::mysql::IInstance &instance,
-                        const std::string &channel_name,
+                        std::string_view channel_name,
                         Replication_channel *out_channel) {
   auto result = instance.queryf(
       std::string(k_base_channel_query) + "WHERE c.channel_name = ?",
@@ -414,7 +414,7 @@ bool get_channel_status(const mysqlshdk::mysql::IInstance &instance,
 }
 
 bool get_channel_state(const mysqlshdk::mysql::IInstance &instance,
-                       const std::string &channel_name,
+                       std::string_view channel_name,
                        Replication_channel::Receiver::State *out_io_state,
                        Replication_channel::Applier::State *out_sql_state) {
   auto result =
@@ -450,7 +450,7 @@ bool get_channel_state(const mysqlshdk::mysql::IInstance &instance,
 }
 
 bool get_channel_state(const mysqlshdk::mysql::IInstance &instance,
-                       const std::string &channel_name,
+                       std::string_view channel_name,
                        Replication_channel::Receiver::State *out_io_state,
                        Replication_channel::Error *out_io_error,
                        Replication_channel::Applier::State *out_sql_state,
@@ -803,42 +803,63 @@ std::string get_purged_gtid_set(const mysqlshdk::mysql::IInstance &server) {
 }
 
 std::string get_received_gtid_set(const mysqlshdk::mysql::IInstance &server,
-                                  const std::string &channel_name) {
+                                  std::string_view channel_name) {
+  // In the query, GTID_SUBTRACT is used, not to subtract GTIDs from the set,
+  // but to actually clean the set, by removing duplicates, etc.
   return server.queryf_one_string(
       0, "",
-      "SELECT GTID_SUBTRACT(GROUP_CONCAT(received_transaction_set), '')"
-      "   FROM performance_schema.replication_connection_status"
-      "   WHERE channel_name = ?",
+      "SELECT received_transaction_set FROM "
+      "performance_schema.replication_connection_status WHERE channel_name = ?",
       channel_name);
 }
 
-std::string get_total_gtid_set(
-    const mysqlshdk::mysql::IInstance &server,
-    const std::vector<std::string> &known_channel_names) {
+std::string get_total_gtid_set(const mysqlshdk::mysql::IInstance &server,
+                               std::string_view channel_name) {
+  // In the query, GTID_SUBTRACT is used, not to subtract GTIDs from the set,
+  // but to actually clean the set, by removing duplicates, etc.
   return server.queryf_one_string(
       0, "",
-      "SELECT GTID_SUBTRACT(CONCAT(@@GLOBAL.GTID_EXECUTED, ',', ("
-      "   SELECT GROUP_CONCAT(received_transaction_set)"
-      "       FROM performance_schema.replication_connection_status"
-      "       WHERE channel_name IN (" +
-          shcore::str_join(known_channel_names, ",", shcore::quote_sql_string) +
-          "))), '')");
+      "SELECT GTID_SUBTRACT(CONCAT(@@GLOBAL.GTID_EXECUTED, ',', (SELECT "
+      "received_transaction_set FROM "
+      "performance_schema.replication_connection_status WHERE channel_name = "
+      "?)), '')",
+      channel_name);
+}
+
+std::string get_total_gtid_set(const mysqlshdk::mysql::IInstance &server,
+                               const std::vector<std::string> &channel_names) {
+  if (channel_names.empty()) return {};
+  if (channel_names.size() == 1)
+    return get_total_gtid_set(server, channel_names.front());
+
+  // In the query, GTID_SUBTRACT is used, not to subtract GTIDs from the set,
+  // but to actually clean the set, by removing duplicates, etc.
+  return server.queryf_one_string(
+      0, "",
+      shcore::str_format(
+          "SELECT GTID_SUBTRACT(CONCAT(@@GLOBAL.GTID_EXECUTED, ',', (SELECT "
+          "GROUP_CONCAT(received_transaction_set) FROM "
+          "performance_schema.replication_connection_status WHERE channel_name "
+          "IN (%s))), '')",
+          shcore::str_join(channel_names, ",", shcore::quote_sql_string)
+              .c_str()));
 }
 
 Gtid_set_relation compare_gtid_sets(const mysqlshdk::mysql::IInstance &server,
-                                    const std::string &gtidset_a,
-                                    const std::string &gtidset_b,
+                                    std::string_view gtidset_a,
+                                    std::string_view gtidset_b,
                                     std::string *out_missing_from_a,
                                     std::string *out_missing_from_b) {
-  if (out_missing_from_a) *out_missing_from_a = gtidset_b;
-  if (out_missing_from_b) *out_missing_from_b = gtidset_a;
+  if (gtidset_a.empty() || gtidset_b.empty()) {
+    if (out_missing_from_a) *out_missing_from_a = std::string{gtidset_b};
+    if (out_missing_from_b) *out_missing_from_b = std::string{gtidset_a};
 
-  if (gtidset_a.empty()) {
-    if (gtidset_b.empty())
-      return Gtid_set_relation::EQUAL;
-    else
-      return Gtid_set_relation::CONTAINED;
-  } else if (gtidset_b.empty()) {
+    if (gtidset_a.empty()) {
+      return gtidset_b.empty() ? Gtid_set_relation::EQUAL
+                               : Gtid_set_relation::CONTAINED;
+    }
+
+    assert(gtidset_b.empty());
     return Gtid_set_relation::CONTAINS;
   }
 
diff --git a/mysqlshdk/libs/mysql/replication.h b/mysqlshdk/libs/mysql/replication.h
@@ -26,6 +26,7 @@
 
 #include <memory>
 #include <string>
+#include <string_view>
 #include <vector>
 
 #include "mysqlshdk/libs/db/session.h"
@@ -187,7 +188,7 @@ std::string format_status(const Replication_channel &channel,
  * @return false if the channel does not exist.
  */
 bool get_channel_status(const mysqlshdk::mysql::IInstance &instance,
-                        const std::string &channel_name,
+                        std::string_view channel_name,
                         Replication_channel *out_channel);
 
 /**
@@ -200,12 +201,12 @@ bool get_channel_status(const mysqlshdk::mysql::IInstance &instance,
  * @return               [TODO]
  */
 bool get_channel_state(const mysqlshdk::mysql::IInstance &instance,
-                       const std::string &channel_name,
+                       std::string_view channel_name,
                        Replication_channel::Receiver::State *out_io_state,
                        Replication_channel::Applier::State *out_sql_state);
 
 bool get_channel_state(const mysqlshdk::mysql::IInstance &instance,
-                       const std::string &channel_name,
+                       std::string_view channel_name,
                        Replication_channel::Receiver::State *out_io_state,
                        Replication_channel::Error *out_io_error,
                        Replication_channel::Applier::State *out_sql_state,
@@ -280,19 +281,28 @@ std::string get_purged_gtid_set(const mysqlshdk::mysql::IInstance &server);
  * Returns GTID set received by a specific channel.
  */
 std::string get_received_gtid_set(const mysqlshdk::mysql::IInstance &server,
-                                  const std::string &channel_name);
+                                  std::string_view channel_name);
 
 /**
  * Returns total GTID set from the server, including received but not yet
  * applied.
  *
- * known_channel_names must contain the list of all channels to consider when
+ * channel_name the channel to consider when checking for
+ * received_transaction_set (usually group_replication_applier)
+ */
+std::string get_total_gtid_set(const mysqlshdk::mysql::IInstance &server,
+                               std::string_view channel_name);
+
+/**
+ * Returns total GTID set from the server, including received but not yet
+ * applied.
+ *
+ * channel_names must contain the list of all channels to consider when
  * checking for received_transaction_set (usually just
  * group_replication_applier)
  */
-std::string get_total_gtid_set(
-    const mysqlshdk::mysql::IInstance &server,
-    const std::vector<std::string> &known_channel_names);
+std::string get_total_gtid_set(const mysqlshdk::mysql::IInstance &server,
+                               const std::vector<std::string> &channel_names);
 
 /**
  * Returns an upper bound of the number of transactions in the given GTID set.
@@ -324,8 +334,8 @@ enum class Gtid_set_relation {
 };
 
 Gtid_set_relation compare_gtid_sets(const mysqlshdk::mysql::IInstance &server,
-                                    const std::string &gtidset_a,
-                                    const std::string &gtidset_b,
+                                    std::string_view gtidset_a,
+                                    std::string_view gtidset_b,
                                     std::string *out_missing_from_a = nullptr,
                                     std::string *out_missing_from_b = nullptr);
 
diff --git a/mysqlshdk/shellcore/base_session.cc b/mysqlshdk/shellcore/base_session.cc
@@ -169,6 +169,7 @@ Query_attribute_store::get_query_attributes(
   assert(translator_cb);
 
   std::vector<mysqlshdk::db::Query_attribute> attributes;
+  attributes.reserve(m_order.size());
 
   for (const auto &name : m_order) {
     attributes.emplace_back(name, translator_cb(m_store.at(std::string{name})));
@@ -207,10 +208,13 @@ ShellBaseSession::~ShellBaseSession() { DEBUG_OBJ_DEALLOC(ShellBaseSession); }
 std::string &ShellBaseSession::append_descr(std::string &s_out, int /*indent*/,
                                             int /*quote_strings*/) const {
   if (!is_open())
-    s_out.append("<" + class_name() + ":disconnected>");
+    s_out.append("<").append(class_name()).append(":disconnected>");
   else
-    s_out.append("<" + class_name() + ":" +
-                 uri(mysqlshdk::db::uri::formats::user_transport()) + ">");
+    s_out.append("<")
+        .append(class_name())
+        .append(":")
+        .append(uri(mysqlshdk::db::uri::formats::user_transport()))
+        .append(">");
   return s_out;
 }
 
@@ -283,16 +287,20 @@ void ShellBaseSession::enable_sql_mode_tracking() {
   log_info("Now tracking 'sql_mode' system variable.");
 }
 
-std::string ShellBaseSession::get_quoted_name(const std::string &name) {
-  size_t index = 0;
-  std::string quoted_name(name);
+std::string ShellBaseSession::get_quoted_name(std::string_view name) {
+  std::string quoted_name;
+  quoted_name.reserve(name.size() + 2);
 
-  while ((index = quoted_name.find("`", index)) != std::string::npos) {
+  size_t index = 1;  // ignore the first '`'
+  quoted_name.append("`");
+  quoted_name.append(name);
+
+  while ((index = quoted_name.find('`', index)) != std::string::npos) {
     quoted_name.replace(index, 1, "``");
     index += 2;
   }
 
-  quoted_name = "`" + quoted_name + "`";
+  quoted_name.append("`");
 
   return quoted_name;
 }
diff --git a/unittest/mysqlshdk/libs/mysql/instance_t.cc b/unittest/mysqlshdk/libs/mysql/instance_t.cc
@@ -1512,4 +1512,21 @@ TEST_F(Instance_test, generate_uuid) {
   instance.generate_uuid();
 }
 
+TEST_F(Instance_test, session_limits) {
+  mysqlshdk::mysql::Instance instance(_session);
+  session
+      .expect_query(
+          "show SESSION variables "
+          "where `variable_name` in ('group_concat_max_len')")
+      .then_return({{"show SESSION variables "
+                     "where `variable_name` in ('group_concat_max_len')",
+                     {"Variable_name", "Value"},
+                     {Type::String, Type::Integer},
+                     {{"group_concat_max_len", "1073741824"}}}});
+  auto group_concat_max_len = instance.get_sysvar_int(
+      "group_concat_max_len", mysqlshdk::mysql::Var_qualifier::SESSION);
+  EXPECT_TRUE(group_concat_max_len);
+  EXPECT_EQ(1024 * 1024 * 1024, *group_concat_max_len);
+}
+
 }  // namespace testing
diff --git a/unittest/mysqlshdk/libs/mysql/replication_t.cc b/unittest/mysqlshdk/libs/mysql/replication_t.cc
diff --git a/unittest/scripts/auto/js_adminapi/scripts/dba_createcluster_crash.js b/unittest/scripts/auto/js_adminapi/scripts/dba_createcluster_crash.js
diff --git a/unittest/scripts/auto/js_adminapi_clusterset/scripts/reboot_cluster_gtids.js b/unittest/scripts/auto/js_adminapi_clusterset/scripts/reboot_cluster_gtids.js
