Skip to content

Commit

Permalink
Bug#30050452: CRASH WHEN EXECUTING "SELECT SLEEP(10)" THROUGH CURSOR …
Browse files Browse the repository at this point in the history
…(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
  • Loading branch information
Jusufadis Bakamovic committed Jun 10, 2020
1 parent 6875535 commit 6653b43
Show file tree
Hide file tree
Showing 22 changed files with 1,408 additions and 319 deletions.
24 changes: 24 additions & 0 deletions include/my_compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
2 changes: 2 additions & 0 deletions sql/handler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
40 changes: 23 additions & 17 deletions storage/temptable/include/temptable/allocator.h
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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. */
Expand Down Expand Up @@ -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() {}
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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);
Expand Down
20 changes: 19 additions & 1 deletion storage/temptable/include/temptable/constants.h
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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 */
25 changes: 10 additions & 15 deletions storage/temptable/include/temptable/handler.h
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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()`). */
Expand Down Expand Up @@ -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() {
Expand All @@ -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 */
137 changes: 137 additions & 0 deletions storage/temptable/include/temptable/kv_store.h
Original file line number Diff line number Diff line change
@@ -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 */
Loading

0 comments on commit 6653b43

Please sign in to comment.