Bug#30050452: CRASH WHEN EXECUTING "SELECT SLEEP(10)" THROUGH CURSOR (WITH THREAD-POOL)

Problem:
  * TempTable segfaults in certain scenarios when ThreadPool (TP) plugin
    is used.
  * Cursor-protocol only accentuates the crash condition more.

Analysis:
  * TempTable implements two of its critical parts through the means of
    thread-local variables, namely its data-backend (storage for tables)
    and custom memory allocator.
  * This becomes a design and functional issue as soon as MySQL server is
    run with the ThreadPool (TP) plugin because presence of that plugin
    will change the traditional thread-handling MySQL model, which is
    1-thread-per-client-connection, to the alternative model where this
    assumption no longer holds and code relying on such assumption
    automatically becomes broken.
  * In particular, TP plugin will not be allocating a new thread for each
    new connection arriving but instead it will pick a thread from
    pre-allocated pool of threads and assign one to the statement
    execution engine.
  * In practice, this means that it is possible that statements within
    one client connection are now served with a multitude of different
    threads (but not simultaneously), which with traditional
    thread-handling model, such condition was not possible (it was always
    the same thread).
  * Furthermore, cursor-protocol makes it possible that even an execution
    of a single statement within one client connection is served by
    different threads.
      * E.g. Cursor-protocol enables fetching the result set of a query
        (data) in chunks. And TP plugin makes a situation possible where
        each chunk is fetched from a different thread.
  * TL;DR thread-local-storage variables cannot really co-exist with TP
    plugin in their natural form.

Solution:
  * Replace TempTable parts which are implemented in terms of
    thread-local-storage variables with a solution which:
      * Is functionally correct in both cases, with and without TP plugin
        running.
      * Does not sacrifice the overall performance at any rate.
      * Does not lock the design but keeps the door open for future
        advancements (e.g. more control over memory consumption).
  * Basic idea is to:
    * Organize data-backend (table-storage) into shards
    * Ditto for shared-blocks (custom memory allocator external state)
    * Use unique connection identifier to map between connections and
      their corresponding shards

Addendum:
  * This patch also fixes some other issues which have been reported but
    which are directly or indirectly caused by the same limitation in
    design:
      * BUG #30050420 CRASHES WHEN CLOSING CONNECTION WITH UNCLOSED
        CURSOR (WITH THREAD-POOL)
      * BUG #31116036 TEMPTABLE WASTES 1MB FOR EACH CONNECTION IN THREAD
        CACHE
      * BUG #31471863 TEMPTABLE ENGINE USES MUCH MORE MEMORY THAN MEMORY
        ENGINE FOR TEMPORARY TABLE

Reviewed-by: Pawel Olchawa <[email protected]>
RB: 24497

Author: Jusufadis Bakamovic

include/my_compiler.h

@@ -342,4 +342,28 @@ inline bool unlikely(bool expr) { return expr; }
 #define MY_COMPILER_CLANG_WORKAROUND_REF_DOCBUG() \
   MY_COMPILER_CLANG_DIAGNOSTIC_IGNORE("-Wdocumentation")
 
+/**
+ * ignore -Wunused-variable compiler warnings for \@see \@ref
+ *
+ * @code
+ * MY_COMPILER_DIAGNOSTIC_PUSH()
+ * MY_COMPILER_CLANG_WORKAROUND_FALSE_POSITIVE_UNUSED_VARIABLE_WARNING()
+ * ...
+ * MY_COMPILER_DIAGNOSTIC_POP()
+ * @endcode
+ *
+ * @see MY_COMPILER_DIAGNOSTIC_PUSH()
+ * @see MY_COMPILER_DIAGNOSTIC_POP()
+ *
+ * allows to work around false positives -Wunused-variable warnings like:
+ *
+ * - \@sa \@ref
+ * - \@see \@ref
+ * - \@return \@ref
+ *   https://bugs.llvm.org/show_bug.cgi?id=46035
+ *
+ */
+#define MY_COMPILER_CLANG_WORKAROUND_FALSE_POSITIVE_UNUSED_VARIABLE_WARNING() \
+  MY_COMPILER_CLANG_DIAGNOSTIC_IGNORE("-Wunused-variable")
+
 #endif /* MY_COMPILER_INCLUDED */

sql/handler.cc

@@ -237,12 +237,14 @@ size_t num_hton2plugins() { return se_plugin_array.size(); }
 
 st_plugin_int *insert_hton2plugin(uint slot, st_plugin_int *plugin) {
   if (se_plugin_array.assign_at(slot, plugin)) return nullptr;
+  builtin_htons.assign_at(slot, true);
   return se_plugin_array[slot];
 }
 
 st_plugin_int *remove_hton2plugin(uint slot) {
   st_plugin_int *retval = se_plugin_array[slot];
   se_plugin_array[slot] = NULL;
+  builtin_htons.assign_at(slot, false);
   return retval;
 }
 

storage/temptable/include/temptable/allocator.h

@@ -1,4 +1,4 @@
-/* Copyright (c) 2016, 2019, Oracle and/or its affiliates. All Rights Reserved.
+/* Copyright (c) 2016, 2020, Oracle and/or its affiliates. All Rights Reserved.
 
 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, as published by the
@@ -43,9 +43,6 @@ TempTable custom allocator. */
 
 namespace temptable {
 
-/** Block that is shared between different tables within a given OS thread. */
-extern thread_local Block shared_block;
-
 /* Thin abstraction which enables logging of memory operations.
  *
  * Used by the Allocator to implement switching from RAM to MMAP-backed
@@ -214,7 +211,7 @@ class Allocator : private MemoryMonitor {
   };
 
   /** Constructor. */
-  Allocator();
+  explicit Allocator(Block *shared_block);
 
   /** Constructor from allocator of another type. The state is copied into the
    * new object. */
@@ -293,23 +290,31 @@ class Allocator : private MemoryMonitor {
     See AllocatorState for details.
    */
   std::shared_ptr<AllocatorState> m_state;
+
+  /** A block of memory which is a state external to this allocator and can be
+   * shared among different instances of the allocator (not simultaneously). In
+   * order to speed up its operations, allocator may decide to consume the
+   * memory of this shared block.
+   */
+  Block *m_shared_block;
 };
 
 /* Implementation of inlined methods. */
 
 template <class T, class AllocationScheme>
-inline Allocator<T, AllocationScheme>::Allocator()
-    : m_state(std::make_shared<AllocatorState>()) {}
+inline Allocator<T, AllocationScheme>::Allocator(Block *shared_block)
+    : m_state(std::make_shared<AllocatorState>()),
+      m_shared_block(shared_block) {}
 
 template <class T, class AllocationScheme>
 template <class U>
 inline Allocator<T, AllocationScheme>::Allocator(const Allocator<U> &other)
-    : m_state(other.m_state) {}
+    : m_state(other.m_state), m_shared_block(other.m_shared_block) {}
 
 template <class T, class AllocationScheme>
 template <class U>
 inline Allocator<T, AllocationScheme>::Allocator(Allocator<U> &&other) noexcept
-    : m_state(std::move(other.m_state)) {}
+    : m_state(std::move(other.m_state)), m_shared_block(other.m_shared_block) {}
 
 template <class T, class AllocationScheme>
 inline Allocator<T, AllocationScheme>::~Allocator() {}
@@ -342,14 +347,15 @@ inline T *Allocator<T, AllocationScheme>::allocate(size_t n_elements) {
 
   Block *block;
 
-  if (shared_block.is_empty()) {
-    shared_block = Block(AllocationScheme::block_size(m_state->number_of_blocks,
-                                                      n_bytes_requested),
-                         Source::RAM);
-    block = &shared_block;
+  if (m_shared_block && m_shared_block->is_empty()) {
+    *m_shared_block = Block(AllocationScheme::block_size(
+                                m_state->number_of_blocks, n_bytes_requested),
+                            Source::RAM);
+    block = m_shared_block;
     ++m_state->number_of_blocks;
-  } else if (shared_block.can_accommodate(n_bytes_requested)) {
-    block = &shared_block;
+  } else if (m_shared_block &&
+             m_shared_block->can_accommodate(n_bytes_requested)) {
+    block = m_shared_block;
   } else if (m_state->current_block.is_empty() ||
              !m_state->current_block.can_accommodate(n_bytes_requested)) {
     const size_t block_size = AllocationScheme::block_size(
@@ -396,7 +402,7 @@ inline void Allocator<T, AllocationScheme>::deallocate(T *chunk_data,
   const auto remaining_chunks =
       block.deallocate(Chunk(chunk_data), n_bytes_requested);
   if (remaining_chunks == 0) {
-    if (block == shared_block) {
+    if (m_shared_block && (block == *m_shared_block)) {
       // Do nothing. Keep the last block alive.
     } else {
       DBUG_ASSERT(m_state->number_of_blocks > 0);

storage/temptable/include/temptable/constants.h

@@ -1,4 +1,4 @@
-/* Copyright (c) 2017, 2017, Oracle and/or its affiliates. All Rights Reserved.
+/* Copyright (c) 2017, 2020, Oracle and/or its affiliates. All Rights Reserved.
 
 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, as published by the
@@ -26,6 +26,8 @@ TempTable constants. */
 #ifndef TEMPTABLE_CONSTANTS_H
 #define TEMPTABLE_CONSTANTS_H
 
+#include "my_config.h"
+
 namespace temptable {
 
 /** Multiply a number by 1024.
@@ -68,6 +70,22 @@ constexpr size_t STORAGE_PAGE_SIZE = 64_KiB;
 /** Number of buckets to have by default in a hash index. */
 constexpr size_t INDEX_DEFAULT_HASH_TABLE_BUCKETS = 1024;
 
+/** Store build-type information into the constexpr expression. */
+#ifndef DBUG_OFF
+constexpr bool DEBUG_BUILD = true;
+#else
+constexpr bool DEBUG_BUILD = false;
+#endif /* DBUG_OFF */
+
+/** Store L1-dcache size information into the constexpr expression. */
+constexpr size_t L1_DCACHE_SIZE = CPU_LEVEL1_DCACHE_LINESIZE;
+
+/** Number of shards in key-value store. */
+constexpr size_t KV_STORE_SHARDS_COUNT = 16 * 1024;
+
+/** Size of a pool containing shared-blocks. */
+constexpr size_t SHARED_BLOCK_POOL_SIZE = 16 * 1024;
+
 } /* namespace temptable */
 
 #endif /* TEMPTABLE_CONSTANTS_H */

storage/temptable/include/temptable/handler.h

@@ -128,17 +128,16 @@ Also called "key", "field", "subkey", "key part", "key segment" elsewhere.
 #ifndef TEMPTABLE_HANDLER_H
 #define TEMPTABLE_HANDLER_H
 
-#ifndef DBUG_OFF
-#include <thread>
-#endif /* DBUG_OFF */
-
 #include "sql/handler.h"
 #include "sql/table.h"
 #include "storage/temptable/include/temptable/storage.h"
 #include "storage/temptable/include/temptable/table.h"
 
 namespace temptable {
 
+/** Forward declarations. */
+class Block;
+
 /** Temptable engine handler. */
 class Handler : public ::handler {
  public:
@@ -524,6 +523,10 @@ class Handler : public ::handler {
   /** Currently opened table, or `nullptr` if none is opened. */
   Table *m_opened_table;
 
+  /** Pointer to the non-owned shared-block of memory to be re-used by all
+   * `Allocator` instances or copies made by `Table`. */
+  Block *m_shared_block;
+
   /** Iterator used by `rnd_init()`, `rnd_next()` and `rnd_end()` methods.
    * It points to the row that was retrieved by the last read call (e.g.
    * `rnd_next()`). */
@@ -554,17 +557,6 @@ class Handler : public ::handler {
 
   /** Number of deleted rows by this handler object. */
   size_t m_deleted_rows;
-
-#ifndef DBUG_OFF
-  /** Check if the current OS thread is the one that created this handler
-   * object.
-   * @return true if current thread == creator thread */
-  bool current_thread_is_creator() const;
-
-  /** The OS thread that created this handler object. The only thread that is
-   * supposed to use it and the tables created via it. */
-  std::thread::id m_owner;
-#endif /* DBUG_OFF */
 };
 
 inline void Handler::opened_table_validate() {
@@ -588,6 +580,9 @@ inline bool Handler::is_field_type_fixed_size(const Field &mysql_field) const {
   }
 }
 
+void kv_store_shards_debug_dump();
+void shared_block_pool_release(THD *thd);
+
 } /* namespace temptable */
 
 #endif /* TEMPTABLE_HANDLER_H */

storage/temptable/include/temptable/kv_store.h

@@ -0,0 +1,137 @@
+/* Copyright (c) 2020, Oracle and/or its affiliates. All Rights Reserved.
+
+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, as published by the
+Free Software Foundation.
+
+This program is also distributed with certain software (including but not
+limited to OpenSSL) that is licensed under separate terms, as designated in a
+particular file or component or in included license documentation. The authors
+of MySQL hereby grant you an additional permission to link the program and
+your derivative works with the separately licensed software that they have
+included with MySQL.
+
+This program is distributed in the hope that it will be useful, but WITHOUT
+ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+FOR A PARTICULAR PURPOSE. See the GNU General Public License, version 2.0,
+for more details.
+
+You should have received a copy of the GNU General Public License along with
+this program; if not, write to the Free Software Foundation, Inc.,
+51 Franklin St, Fifth Floor, Boston, MA 02110-1301  USA */
+
+/** @file storage/temptable/include/temptable/kv_store.h
+TempTable key-value store implementation. */
+
+#ifndef TEMPTABLE_KV_STORE_H
+#define TEMPTABLE_KV_STORE_H
+
+#include <shared_mutex>
+#include <string>
+#include <unordered_map>
+
+#include "storage/temptable/include/temptable/constants.h"
+#include "storage/temptable/include/temptable/kv_store_logger.h"
+
+namespace temptable {
+
+/** Forward-declarations. */
+class Table;
+
+/** Key-value store, a convenience wrapper class which models a thread-safe
+ * dictionary type.
+ *
+ * Thread-safety is accomplished by using a `Lock` which can be any of the usual
+ * mutual exclusive algorithms from C++ thread-support library or any other
+ * which satisfy C++ concurrency named requirements. E.g. mutex, timed_mutex,
+ * recursive_mutex, recursive_timed_mutex, shared_mutex, shared_timed_mutex, ...
+ * User code can opt-in for any of those. Also, whether the actual
+ * implementation will use shared-locks or exclusive-locks (for read-only
+ * operations) is handled automagically by this class.
+ *
+ * Furthermore, user code can similarly opt-in for different key-value
+ * implementation but whose interface is compatible with the one of
+ * std::unordered_map.
+ * */
+template <typename Lock,
+          template <typename...> class KeyValueImpl = std::unordered_map>
+class Key_value_store
+    : public Key_value_store_logger<Key_value_store<Lock, KeyValueImpl>,
+                                    DEBUG_BUILD> {
+  /** Do not break encapsulation when using CRTP. */
+  friend struct Key_value_store_logger<Key_value_store<Lock, KeyValueImpl>,
+                                       DEBUG_BUILD>;
+
+  /** Help ADL to bring debugging/logging symbols into the scope. */
+  using Key_value_store_logger<Key_value_store<Lock, KeyValueImpl>,
+                               DEBUG_BUILD>::dbug_print;
+  using Key_value_store_logger<Key_value_store<Lock, KeyValueImpl>,
+                               DEBUG_BUILD>::log;
+
+  /** Check whether we can use shared locks (which enable multiple concurrent
+   * readers) or must we rather fallback to exclusive locks. Shared-locks will
+   * be used only during read-only operations.
+   * */
+  using Exclusive_or_shared_lock =
+      std::conditional_t<std::is_same<Lock, std::shared_timed_mutex>::value,
+                         std::shared_lock<Lock>, std::lock_guard<Lock>>;
+
+  /** Alias for our key-value store implementation. */
+  using Key_value_store_impl = KeyValueImpl<std::string, Table>;
+
+  /** Container holding (table-name, Table) tuples. */
+  Key_value_store_impl m_kv_store;
+
+  /** Lock type. */
+  Lock m_lock;
+
+ public:
+  /** Inserts a new table into the container constructed in-place with the
+   * given args if there is no table with the key in the container.
+   *
+   * [in] args Arguments to forward to the constructor of the table.
+   * @return Returns a pair consisting of an iterator to the inserted table,
+   * or the already-existing table if no insertion happened, and a bool
+   * denoting whether the insertion took place (true if insertion happened,
+   * false if it did not).
+   * */
+  template <class... Args>
+  std::pair<typename Key_value_store_impl::iterator, bool> emplace(
+      Args &&... args) {
+    std::lock_guard<Lock> lock(m_lock);
+    dbug_print();
+    log(Key_value_store_stats::Event::EMPLACE);
+    return m_kv_store.emplace(args...);
+  }
+
+  /** Searches for a table with given name (key).
+   *
+   * [in] key Name of a table to search for.
+   * @return Pointer to table if found, nullptr otherwise.
+   * */
+  Table *find(const std::string &key) {
+    Exclusive_or_shared_lock lock(m_lock);
+    auto iter = m_kv_store.find(key);
+    if (iter != m_kv_store.end()) {
+      return &iter->second;
+    } else {
+      return nullptr;
+    }
+  }
+
+  /** Removes the table (if one exists) with the given name (key).
+   *
+   * [in] key Name of a table to remove..
+   * @return Number of elements removed.
+   * */
+  typename Key_value_store_impl::size_type erase(const std::string &key) {
+    std::lock_guard<Lock> lock(m_lock);
+    dbug_print();
+    log(Key_value_store_stats::Event::ERASE);
+    return m_kv_store.erase(key);
+  }
+};
+
+}  // namespace temptable
+
+#endif /* TEMPTABLE_KV_STORE_H */