BUG#34819861: Gtid_set::Interval_chunk mem incorrectly tracked on THD_applier_module_receiver

Anibal Pinto · Anibal Pinto · commit 7db174a7342e · 2024-09-12T10:19:04.000+02:00
In a GR cluster setup with a primary and two secondaries, it was observed
that the primary was not releasing all allocated memory related to the
"memory/sql/Gtid_set::Interval_chunk" event name.

The memory used by GR associated to the the key
`memory/sql/Gtid_set::Interval_chunk` is allocated by one thread:
`thread/group_rpl/THD_applier_module_receiver`, but it is released by a
different thread: `thread/group_rpl/THD_certifier_broadcast`.
This was causing a incorrect memory usage relative to
`thread/group_rpl/THD_applier_module_receiver` since only memory
allocations were accounted, making the reported memory always
increasing.
The global memory reported on
performance_schema.memory_summary_global_by_event_name` was not
affected.

In order to avoid the incorrect memory usage, the used memory ownership
is transferred from `thread/group_rpl/THD_applier_module_receiver` when
it becomes out of scope into `thread/group_rpl/THD_certifier_broadcast`
when it becomes part of the scope.

Change-Id: Ieed76a6fd338ef693f8cf812c5e11680494b8829
diff --git a/include/prealloced_array.h b/include/prealloced_array.h
@@ -310,6 +310,15 @@ class Prealloced_array {
     return false;
   }
 
+  /**
+    Claim memory ownership.
+
+    @param  claim take ownership of memory
+  */
+  void claim_memory_ownership(bool claim) {
+    if (!using_inline_buffer()) my_claim(m_ext.m_array_ptr, claim);
+  }
+
   /**
     Copies an element into the back of the array.
     Complexity: Constant (amortized time, reallocation may happen).
diff --git a/mysql-test/suite/group_replication/r/gr_internal_chunk_memory_release.result b/mysql-test/suite/group_replication/r/gr_internal_chunk_memory_release.result
@@ -0,0 +1,36 @@
+include/group_replication.inc
+Warnings:
+Note	####	Sending passwords in plain text without SSL/TLS is extremely insecure.
+Note	####	Storing MySQL user name or password information in the connection metadata repository is not secure and is therefore not recommended. Please consider using the USER and PASSWORD connection options for START REPLICA; see the 'START REPLICA Syntax' in the MySQL Manual for more information.
+[connection server1]
+include/start_and_bootstrap_group_replication.inc
+
+############################################################
+# 1. Populate server with data to alloc memory using
+#    Gtid_set::Interval_chunk key.
+CREATE TABLE t1 (c1 INT NOT NULL AUTO_INCREMENT PRIMARY KEY, c2 INT) ENGINE=InnoDB;
+BEGIN;
+INSERT INTO t1 (c2) values (2);
+INSERT INTO t1 (c2) values (2);
+INSERT INTO t1 (c2) values (2);
+INSERT INTO t1 (c2) values (2);
+INSERT INTO t1 (c2) values (2);
+COMMIT;
+
+############################################################
+# 2. Assert values on thread and global counters before
+#    running garbage collect.
+include/assert.inc [Global counters shall been higher than sum of applier and certifier threads.]
+
+############################################################
+# 3. Wait for execution of another garbage collection
+
+############################################################
+# 4. Assert values on thread and global counters after
+#    running garbage collect.
+include/assert.inc [Global counters shall been higher than sum of applier and certifier threads.]
+
+############################################################
+# 5. Cleanup
+DROP TABLE t1;
+include/group_replication_end.inc
diff --git a/mysql-test/suite/group_replication/t/gr_internal_chunk_memory_release.test b/mysql-test/suite/group_replication/t/gr_internal_chunk_memory_release.test
@@ -0,0 +1,89 @@
+################################################################################
+# Validate that primary releases all allocated memory related to
+# "memory/sql/Gtid_set::Interval_chunk" event name.
+#
+# Test:
+# 0. The test requires one server: M1.
+# 1. Populate server with data to alloc memory using
+#    Gtid_set::Interval_chunk key.
+# 2. Assert values on thread and global counters before
+#    running garbage collect.
+# 3. Wait for execution of another garbage collection
+# 4. Assert values on thread and global counters after
+#    running garbage collect.
+# 5. Cleanup
+################################################################################
+
+--source include/have_nodebug.inc
+--source include/not_valgrind.inc
+--source include/not_asan.inc
+--source include/big_test.inc
+--source include/have_group_replication_plugin.inc
+--let $rpl_skip_group_replication_start= 1
+--source include/group_replication.inc
+
+--source include/start_and_bootstrap_group_replication.inc
+
+
+--echo
+--echo ############################################################
+--echo # 1. Populate server with data to alloc memory using
+--echo #    Gtid_set::Interval_chunk key.
+
+CREATE TABLE t1 (c1 INT NOT NULL AUTO_INCREMENT PRIMARY KEY, c2 INT) ENGINE=InnoDB;
+
+BEGIN;
+INSERT INTO t1 (c2) values (2);
+INSERT INTO t1 (c2) values (2);
+INSERT INTO t1 (c2) values (2);
+INSERT INTO t1 (c2) values (2);
+INSERT INTO t1 (c2) values (2);
+COMMIT;
+
+--exec $MYSQL_SLAP --create-schema=test --delimiter=";" --iterations=100 --query="INSERT INTO t1 (c2) SELECT c2 FROM t1 LIMIT 5" --concurrency=1 --silent
+
+
+--echo
+--echo ############################################################
+--echo # 2. Assert values on thread and global counters before
+--echo #    running garbage collect.
+
+--let $applier_bytes_used= `SELECT CURRENT_NUMBER_OF_BYTES_USED FROM performance_schema.memory_summary_by_thread_by_event_name JOIN performance_schema.threads USING(thread_id) WHERE event_name = "memory/sql/Gtid_set::Interval_chunk" AND name = "thread/group_rpl/THD_applier_module_receiver"`
+--let $certifier_bytes_used= `SELECT CURRENT_NUMBER_OF_BYTES_USED FROM performance_schema.memory_summary_by_thread_by_event_name JOIN performance_schema.threads USING(thread_id) WHERE event_name = "memory/sql/Gtid_set::Interval_chunk" AND name = "thread/group_rpl/THD_certifier_broadcast"`
+--let $global_bytes_used= `SELECT CURRENT_NUMBER_OF_BYTES_USED FROM performance_schema.memory_summary_global_by_event_name WHERE EVENT_NAME = "memory/sql/Gtid_set::Interval_chunk"`
+
+--let $assert_text= Global counters shall been higher than sum of applier and certifier threads.
+--let $assert_cond= [SELECT $applier_bytes_used + $certifier_bytes_used] <= $global_bytes_used
+--source include/assert.inc
+
+
+--echo
+--echo ############################################################
+--echo # 3. Wait for execution of another garbage collection
+
+--let $wait_timeout= 150
+--let $wait_condition= SELECT Count_transactions_rows_validating=5 FROM performance_schema.replication_group_member_stats WHERE member_id IN (SELECT @@server_uuid)
+--source include/wait_condition.inc
+
+
+--echo
+--echo ############################################################
+--echo # 4. Assert values on thread and global counters after
+--echo #    running garbage collect.
+
+--let $applier_bytes_used= `SELECT CURRENT_NUMBER_OF_BYTES_USED FROM performance_schema.memory_summary_by_thread_by_event_name JOIN performance_schema.threads USING(thread_id) WHERE event_name = "memory/sql/Gtid_set::Interval_chunk" AND name = "thread/group_rpl/THD_applier_module_receiver"`
+--let $certifier_bytes_used= `SELECT CURRENT_NUMBER_OF_BYTES_USED FROM performance_schema.memory_summary_by_thread_by_event_name JOIN performance_schema.threads USING(thread_id) WHERE event_name = "memory/sql/Gtid_set::Interval_chunk" AND name = "thread/group_rpl/THD_certifier_broadcast"`
+--let $global_bytes_used= `SELECT CURRENT_NUMBER_OF_BYTES_USED FROM performance_schema.memory_summary_global_by_event_name WHERE EVENT_NAME = "memory/sql/Gtid_set::Interval_chunk"`
+
+--let $assert_text= Global counters shall been higher than sum of applier and certifier threads.
+--let $assert_cond= [SELECT $applier_bytes_used + $certifier_bytes_used] <= $global_bytes_used
+--source include/assert.inc
+
+
+--echo
+--echo ############################################################
+--echo # 5. Cleanup
+
+DROP TABLE t1;
+
+--source include/group_replication_end.inc
diff --git a/plugin/group_replication/src/certifier.cc b/plugin/group_replication/src/certifier.cc
@@ -577,7 +577,15 @@ void Certifier::clear_certification_info() {
   for (Certification_info::iterator it = certification_info.begin();
        it != certification_info.end(); ++it) {
     // We can only delete the last reference.
-    if (it->second->unlink() == 0) delete it->second;
+    if (it->second->unlink() == 0) {
+      /*
+        Claim Gtid_set_ref used memory to
+        `thread/group_rpl/THD_certifier_broadcast` thread, since this is thread
+        that does release the memory.
+      */
+      it->second->claim_memory_ownership(true);
+      delete it->second;
+    }
   }
 
   certification_info.clear();
@@ -904,6 +912,14 @@ rpl_gno Certifier::certify(Gtid_set *snapshot_version,
           item_previous_sequence_number != parallel_applier_sequence_number)
         transaction_last_committed = item_previous_sequence_number;
     }
+    /*
+      The memory used by Gtid_set_ref is allocated by
+      `thread/group_rpl/THD_applier_module_receiver`, though it will be released
+      by `thread/group_rpl/THD_certifier_broadcast` thread.  To avoid untracked
+      memory release on `thread/group_rpl/THD_applier_module_receiver` we do
+      dissociate this used memory from this thread.
+    */
+    snapshot_version_value->claim_memory_ownership(false);
   }
 
   /*
@@ -1154,7 +1170,15 @@ bool Certifier::add_item(const char *item, Gtid_set_ref *snapshot_version,
     *item_previous_sequence_number =
         it->second->get_parallel_applier_sequence_number();
 
-    if (it->second->unlink() == 0) delete it->second;
+    if (it->second->unlink() == 0) {
+      /*
+        Claim Gtid_set_ref used memory to
+        `thread/group_rpl/THD_certifier_broadcast` thread, since this is thread
+        that does release the memory.
+      */
+      it->second->claim_memory_ownership(true);
+      delete it->second;
+    }
 
     it->second = snapshot_version;
     error = false;
@@ -1277,7 +1301,15 @@ void Certifier::garbage_collect() {
   stable_gtid_set_lock->wrlock();
   while (it != certification_info.end()) {
     if (it->second->is_subset_not_equals(stable_gtid_set)) {
-      if (it->second->unlink() == 0) delete it->second;
+      if (it->second->unlink() == 0) {
+        /*
+          Claim Gtid_set_ref used memory to
+          `thread/group_rpl/THD_certifier_broadcast` thread, since this is
+          thread that does release the memory.
+        */
+        it->second->claim_memory_ownership(true);
+        delete it->second;
+      }
       certification_info.erase(it++);
     } else
       ++it;
@@ -1662,6 +1694,14 @@ int Certifier::set_certification_info(
     value->link();
     certification_info.insert(
         std::pair<std::string, Gtid_set_ref *>(key, value));
+    /*
+      The memory used by Gtid_set_ref is allocated by
+      `thread/group_rpl/THD_applier_module_receiver`, though it will be released
+      by `thread/group_rpl/THD_certifier_broadcast` thread.  To avoid untracked
+      memory release on `thread/group_rpl/THD_applier_module_receiver` we do
+      dissociate this used memory from this thread.
+    */
+    value->claim_memory_ownership(false);
   }
 
   if (initialize_server_gtid_set()) {
diff --git a/sql/rpl_gtid.h b/sql/rpl_gtid.h
@@ -1491,6 +1491,14 @@ class Gtid_set {
  public:
   /// Destroy this Gtid_set.
   ~Gtid_set();
+
+  /**
+    Claim ownership of memory.
+
+    @param claim  claim ownership of memory.
+  */
+  void claim_memory_ownership(bool claim);
+
   /**
     Removes all gtids from this Gtid_set.
 
diff --git a/sql/rpl_gtid_set.cc b/sql/rpl_gtid_set.cc
@@ -99,6 +99,17 @@ Gtid_set::Gtid_set(Sid_map *_sid_map, const char *text,
   *status = add_gtid_text(text);
 }
 
+void Gtid_set::claim_memory_ownership(bool claim) {
+  m_intervals.claim_memory_ownership(claim);
+
+  Interval_chunk *chunk = chunks;
+  while (chunk != nullptr) {
+    Interval_chunk *next_chunk = chunk->next;
+    my_claim(chunk, claim);
+    chunk = next_chunk;
+  }
+}
+
 void Gtid_set::init() {
   DBUG_TRACE;
   has_cached_string_length = false;
