- Change thd->backup_commit_lock from MDL_request* to MDL_ticket*,
and convert related call sites in handler.cc, log.cc, sql_class.cc,
and xa.cc to use MDL_ACQUIRE_LOCK.
- Convert reload_acl_and_cache() in sql_reload.cc, purge_master_logs()
and reset_master() in sql_repl.cc to MDL_ACQUIRE_LOCK, holding the
ticket in a local MDL_ticket* (these functions used a local
MDL_request originally and should not touch thd->backup_commit_lock).
- Convert acquire_lock() in partition_info.cc to MDL_ACQUIRE_LOCK.
- Add MDL_REQUEST_LIST_ADD() helper for enqueuing lock requests into
MDL_request_list, and convert call sites in sql_base.cc and sp.cc.
All new code of the whole pull request, including one or several files
that are either new files or modified ones, are contributed under the
BSD-new license. I am contributing on behalf of my employer Amazon Web
Services, Inc.
Add new acquire_lock() overloads on MDL_context that combine MDL_request
initialization and lock acquisition into a single call, returning
MDL_ticket* directly. Convert applicable call sites across the codebase
to use the new ACQUIRE_LOCK macro.
All new code of the whole pull request, including one or several files
that are either new files or modified ones, are contributed under the
BSD-new license. I am contributing on behalf of my employer Amazon Web
Services, Inc.
Originally two MDL_EXCLUSIVE are used for pure alias of derived table
and its schema which is empty db name. It is concurrency performance
overhead and there is no obvious reason why such MDL should exist.
MDEV-33985 changed pure alias check from `db.str` to `db.length` and
that allowed `empty_c_string` to be pure alias too. It is important
for MDEV-33985 fix with sequences. Now such kind of pure alias skips
MDL initialization and thus derived table fails falsely on BACKUP
namespace as it is default namespace for 0-filled structure (see TODO
comments).
The fix a) keeps MDL_NOT_INITIALIZED for uninitialized MDL and that
was the consequence of MDL_INTENTION_EXCLUSIVE value 0 (see TODO
comment).
And b) does not update MDL type for pure alias when view is
initialized, so it is MDL_NOT_INITIALIZED. `lock_table_names()` code
naturally skips such kind of MDL with (mdl_request.type <
MDL_SHARED_UPGRADABLE) condition when processing tables list.
MDL_context_owner was introduced in MySQL for gtest unit testing of MDL,
which MariaDB doesn't use. Its virtual methods (enter_cond, exit_cond,
is_killed, get_thd, notify_shared_lock) are not MDL-specific - they are
part of THD and were only encapsulated in MDL_context_owner as a
misleading abstraction.
This is a cleanup that:
- Removes the MDL_context_owner class entirely
- Removes THD's inheritance from MDL_context_owner
- Changes MDL to use THD* directly instead of MDL_context_owner*
- Removes the redundant MDL_context::get_owner() method
- Moves ENTER_COND/EXIT_COND macros to sql_class.h near THD_ENTER_COND
- Removed duplicate words, like "the the" and "to to"
- Removed duplicate lines (one double sort line found in mysql.cc)
- Fixed some typos found while searching for duplicate words.
Command used to find duplicate words:
egrep -rI "\s([a-zA-Z]+)\s+\1\s" | grep -v param
Thanks to Artjoms Rimdjonoks for the command and pointing out the
spelling errors.
Statements that intend to modify data have to acquire protection
against ongoing backup. Prior to backup locks, protection against
FTWRL was acquired in form of 2 shared metadata locks of GLOBAL
(global read lock) and COMMIT namespaces. These two namespaces
were separate entities, they didn't share data structures and
locking primitives. And thus they were separate contention
points.
With backup locks, introduced by 7a9dfdd, these namespaces were
combined into a single BACKUP namespace. It became a single
contention point, which doubled load on BACKUP namespace data
structures and locking primitives compared to GLOBAL and COMMIT
namespaces. In other words system throughput has halved.
MDL fast lanes solve this problem by allowing multiple contention
points for single MDL_lock. Fast lane is scalable multi-instance
registry for leightweight locks. Internally it is just a list of
granted tickets, close counter and a mutex.
Number of fast lanes (or contention points) is defined by the
metadata_locks_instances system variable. Value of 1 disables fast
lanes and lock requests are served by conventional MDL_lock data
structures.
Since fast lanes allow arbitrary number of contention points, they
outperform pre-backup locks GLOBAL and COMMIT.
Fast lanes are enabled only for BACKUP namespace. Support for other
namespaces is to be implemented separately.
Lock types are divided in 2 categories: lightweight and heavyweight.
Lightweight lock types represent DML: MDL_BACKUP_DML,
MDL_BACKUP_TRANS_DML, MDL_BACKUP_SYS_DML, MDL_BACKUP_DDL,
MDL_BACKUP_ALTER_COPY, MDL_BACKUP_COMMIT. They are fully compatible
with each other. Normally served by corresponding fast lane, which is
determined by thread_id % metadata_locks_instances.
Heavyweight lock types represent ongoing backup: MDL_BACKUP_START,
MDL_BACKUP_FLUSH, MDL_BACKUP_WAIT_FLUSH, MDL_BACKUP_WAIT_DDL,
MDL_BACKUP_WAIT_COMMIT, MDL_BACKUP_FTWRL1, MDL_BACKUP_FTWRL2,
MDL_BACKUP_BLOCK_DDL. These locks are always served by conventional
MDL_lock data structures. Whenever such lock is requested, fast
lanes are closed and all tickets registered in fast lanes are
moved to conventional MDL_lock data structures. Until such locks
are released or aborted, lightweight lock requests are served by
conventional MDL_lock data structures.
Strictly speaking moving tickets from fast lanes to conventional
MDL_lock data structures is not required. But it allows to reduce
complexity and keep intact methods like: MDL_lock::visit_subgraph(),
MDL_lock::notify_conflicting_locks(), MDL_lock::reschedule_waiters(),
MDL_lock::can_grant_lock().
It is not even required to register tickets in fast lanes. They
can be implemented basing on an atomic variable that holds two
counters: granted lightweight locks and granted/waiting heavyweight
locks. Similarly to MySQL solution, which roughly speaking has
"single atomic fast lane". However it appears to be it won't bring
any better performance, while code complexity is going to be much
higher.
Given that MDL_lock::m_strategy is only accessed by MDL_lock methods,
there is no point in having MDL_lock::needs_notification() and
MDL_lock::hog_lock_types_bitmap() getters anymore.
MDL_lock::has_pending_conflicting_lock() moved to MDL_lock class.
This is part of broader cleanup, which aims to make large part of
MDL_lock members private. It is needed to simplify further work on
MDEV-19749 - MDL scalability regression after backup locks.
Avoid accessing MDL_lock::key from outside of MDL_lock class directly,
use MDL_lock::get_key() instead.
This is part of broader cleanup, which aims to make large part of
MDL_lock members private. It is needed to simplify further work on
MDEV-19749 - MDL scalability regression after backup locks.
Removed useless MDL_ticket::create() and MDL_ticket::destroy()
indirection. It didn't serve any purpose.
Moved mysql_mdl_create() PFS call out of critical section.
Moved ticket->m_time assignment out of MDL_lock::m_rwlock protection.
It increases critical section size for no good reason. Assigning it
before critical section must be good enough for statistics purposes,
since we must not do long waits here anyway.
This is part of broader cleanup, which aims to make large part of
MDL_lock members private. It is needed to simplify further work on
MDEV-19749 - MDL scalability regression after backup locks.
Avoid accessing MDL_lock::m_granted and MDL_lock::m_rwlock from
MDL_ticket::downgrade_lock(), use MDL_lock::downgrade() instead.
This is part of broader cleanup, which aims to make large part of
MDL_lock members private. It is needed to simplify further work on
MDEV-19749 - MDL scalability regression after backup locks.
If a slave replicating an event has waited for more than
@@slave_abort_blocking_timeout for a conflicting metadata lock held by a
non-replication thread, the blocking query is killed to allow replication to
proceed and not be blocked indefinitely by a user query.
Reviewed-by: Monty <monty@mariadb.org>
Signed-off-by: Kristian Nielsen <knielsen@knielsen-hq.org>
Add a debug-only field MDL_context::lock_warrant. This field can be set
to the MDL context different from the one the current execution is done in.
The lock warrantor has to hold an MDL for at least a duration of a table
lifetime.
This is needed in the subsequent commit so that the shared MDL acquired by
the InnoDB purge_coordinator_task can be shared by purge_worker_task
that access index records that include virtual columns.
Reviewed by: Vladislav Vaintroub
don't forget to reset mdl_context.m_deadlock_overweight when
taking the THD out of the cache - the history of previous connections
should not affect the weight in deadlock victim selection
(small cleanup of the test to help the correct merge)
- Changing the global function ok_for_lower_case_names()
into a method in class Lex_ident_fs.
- Changing a few functions/methods to get the database name
as a "const LEX_CSTRING" instead of a "const char *".
All these functions/methods use ok_for_lower_case_names()
inside. This change helps to avoid new strlen() calls, and also
removes a few old strlen() calls.
This patch is the result of running
run-clang-tidy -fix -header-filter=.* -checks='-*,modernize-use-equals-default' .
Code style changes have been done on top. The result of this change
leads to the following improvements:
1. Binary size reduction.
* For a -DBUILD_CONFIG=mysql_release build, the binary size is reduced by
~400kb.
* A raw -DCMAKE_BUILD_TYPE=Release reduces the binary size by ~1.4kb.
2. Compiler can better understand the intent of the code, thus it leads
to more optimization possibilities. Additionally it enabled detecting
unused variables that had an empty default constructor but not marked
so explicitly.
Particular change required following this patch in sql/opt_range.cc
result_keys, an unused template class Bitmap now correctly issues
unused variable warnings.
Setting Bitmap template class constructor to default allows the compiler
to identify that there are no side-effects when instantiating the class.
Previously the compiler could not issue the warning as it assumed Bitmap
class (being a template) would not be performing a NO-OP for its default
constructor. This prevented the "unused variable warning".
It was possibile for a user to create an interlocked state which may go on
for a significant period of time. There is a tight loop in the FTWRL code
path that tries to repeatedly acquire a read lock. As the weight of FTWRL
lock is the smallest among others, it's always selected by the deadlock
detector, but can never be killed.
Imaging the following sequence:
connection_0 connection_1
GET_LOCK("l1", 0);
LOCK TABLES t WRITE;
FLUSH TABLES WITH READ LOCK;
GET_LOCK("l1", 1000);
The GET_LOCK statement in connection_1 triggers the deadlock detector,
which tries to select the lock in FTWRL, since its weight is 0. However,
since a loop in Global_read_lock::lock_global_read_lock() tries to always
win, it tries to acquire lock again. Which invokes the deadlock detector,
and that cycle continues until GET_LOCK in connection_1 times out.
This patch resolves the live-locking by introducing a dynamic bonus to the
deadlock weight associated with every lock. Each lock gets a bonus weight
each time it's selected by the deadlock detector. In case of a live-lock
situation, those locks that cannot be killed, get additional weight each
iteration. Eventually their weight becomes so high that the deadlock
detector shifts its attention to other lock, until it find the one that
can be killed.
The issue was:
T1, a parallel slave worker thread, is waiting for another worker thread to
commit. While waiting, it has the MDL_BACKUP_COMMIT lock.
T2, working for mariabackup, is doing BACKUP STAGE BLOCK_COMMIT and blocks
all commits.
This causes a deadlock as the thread T1 is waiting for can't commit.
Fixed by moving locking of MDL_BACKUP_COMMIT from ha_commit_trans() to
commit_one_phase_2()
Other things:
- Added a new argument to ha_comit_one_phase() to signal if the
transaction was a write transaction.
- Ensured that ha_maria::implicit_commit() is always called under
MDL_BACKUP_COMMIT. This code is not needed in 10.5
- Ensure that MDL_Request values 'type' and 'ticket' are always
initialized. This makes it easier to check the state of the MDL_Request.
- Moved thd->store_globals() earlier in handle_rpl_parallel_thread() as
thd->init_for_queries() could use a MDL that could crash if store_globals
where not called.
- Don't call ha_enable_transactions() in THD::init_for_queries() as this
is both slow (uses MDL locks) and not needed.
The MDEV-17262 commit 26432e49d3
was skipped. In Galera 4, the implementation would seem to require
changes to the streaming replication.
In the tests archive.rnd_pos main.profiling, disable_ps_protocol
for SHOW STATUS and SHOW PROFILE commands until MDEV-18974
has been fixed.
There were two newly enabled warnings:
1. cast for a function pointers. Affected sql_analyse.h, mi_write.c
and ma_write.cc, mf_iocache-t.cc, mysqlbinlog.cc, encryption.cc, etc
2. memcpy/memset of nontrivial structures. Fixed as:
* the warning disabled for InnoDB
* TABLE, TABLE_SHARE, and TABLE_LIST got a new method reset() which
does the bzero(), which is safe for these classes, but any other
bzero() will still cause a warning
* Table_scope_and_contents_source_st uses `TABLE_LIST *` (trivial)
instead of `SQL_I_List<TABLE_LIST>` (not trivial) so it's safe to
bzero now.
* added casts in debug_sync.cc and sql_select.cc (for JOIN)
* move assignment method for MDL_request instead of memcpy()
* PARTIAL_INDEX_INTERSECT_INFO::init() instead of bzero()
* remove constructor from READ_RECORD() to make it trivial
* replace some memcpy() with c++ copy assignments
