In cmake -DWITH_UBSAN=ON builds with clang but not with GCC,
-fsanitize=undefined will flag several runtime errors on
function pointer mismatch related to the lock-free hash table LF_HASH.
Let us use matching function signatures and remove function pointer
casts in order to avoid potential bugs due to undefined behaviour.
These errors could be caught at compilation time by
-Wcast-function-type-strict, which is available starting with clang-16,
but not available in any version of GCC as of now. The old GCC flag
-Wcast-function-type is enabled as part of -Wextra, but it specifically
does not catch these errors.
Reviewed by: Vladislav Vaintroub
A few different incorrect function type UBSAN issues have been
grouped into this patch.
The only real potentially undefined behavior is an error about
show_func_mutex_instances_lost, which when invoked in
sql_show.cc::show_status_array(), puts 5 arguments onto the stack;
however, the implementing function only actually has 3 parameters (so
only 3 would be popped). This was fixed by adding in the remaining
parameters to satisfy the type mysql_show_var_func.
The rest of the findings are pointer type mismatches that wouldn't
lead to actual undefined behavior. The lf_hash_initializer function
type definition is
typedef void (*lf_hash_initializer)(LF_HASH *hash, void *dst, const void *src);
but the MDL_lock and table cache's implementations of this function
do not have that signature. The MDL_lock has specific MDL object
parameters:
static void lf_hash_initializer(LF_HASH *hash __attribute__((unused)),
MDL_lock *lock, MDL_key *key_arg)
and the table cache has specific TDC parameters:
static void tdc_hash_initializer(LF_HASH *,
TDC_element *element, LEX_STRING *key)
leading to UBSAN runtime errors when invoking these functions.
This patch fixes these type mis-matches by changing the
implementing functions to use void * and const void * for their
respective parameters, and later casting them to their expected
type in the function body.
Note too the functions tdc_hash_key and tc_purge_callback had
a similar problem to tdc_hash_initializer and was fixed
similarly.
Reviewed By:
============
Sergei Golubchik <serg@mariadb.com>
Add wait_condition between debug sync SIGNAL points and other
expected state conditions and refactor actual sync point for
easier to use in test case.
Signed-off-by: Julius Goryavsky <julius.goryavsky@mariadb.com>
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".
There are separate flags DBUG_OFF for disabling the DBUG facility
and ENABLED_DEBUG_SYNC for enabling the DEBUG_SYNC facility.
Let us allow debug builds without DEBUG_SYNC.
Note: For CMAKE_BUILD_TYPE=Debug, CMakeLists.txt will continue to
define ENABLED_DEBUG_SYNC.
In commit 28325b0863
a compile-time option was introduced to disable the macros
DBUG_ENTER and DBUG_RETURN or DBUG_VOID_RETURN.
The parameter name WITH_DBUG_TRACE would hint that it also
covers DBUG_PRINT statements. Let us do that: WITH_DBUG_TRACE=OFF
shall disable DBUG_PRINT() as well.
A few InnoDB recovery tests used to check that some output from
DBUG_PRINT("ib_log", ...) is present. We can live without those checks.
Reviewed by: Vladislav Vaintroub
Making changes to wsrep_mysqld.h causes large parts of server code to
be recompiled. The reason is that wsrep_mysqld.h is included by
sql_class.h, even tough very little of wsrep_mysqld.h is needed in
sql_class.h. This commit introduces a new header file, wsrep_on.h,
which is meant to be included from sql_class.h, and contains only
macros and variable declarations used to determine whether wsrep is
enabled.
Also, header wsrep.h should only contain definitions that are also
used outside of sql/. Therefore, move WSREP_TO_ISOLATION* and
WSREP_SYNC_WAIT macros to wsrep_mysqld.h.
Reviewed-by: Jan Lindström <jan.lindstrom@mariadb.com>
Log MDL state transitions. Trace-friendly message
format. DBUG_LOCK_FILE replaced by thread-local storage.
Logged states legend:
Seized lock was acquired without waiting
Waiting lock is waiting
Acquired lock was acquired after waiting
Released lock was released
Deadlock lock was aborted due to deadlock
Timeout lock was aborted due to timeout >0
Nowait lock was aborted due to zero timeout
Killed lock was aborted due to kill message
OOM can not acquire because out of memory
Usage:
mtr --mysqld=--debug=d,mdl,query:i:o,/tmp/mdl.log
Cleanup from garbage messages:
sed -i -re \
'/(mysql|performance_schema|sys|mtr)\// d; /MDL_BACKUP_/ d' \
/tmp/mdl.log
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.
Some DML operations on tables having unique secondary keys cause scanning
in the secondary index, for instance to find potential unique key violations
in the seconday index. This scanning may involve GAP locking in the index.
As this locking happens also when applying replication events in high priority
applier threads, there is a probabality for lock conflicts between two wsrep
high priority threads.
This PR avoids lock conflicts of high priority wsrep threads, which do
secondary index scanning e.g. for duplicate key detection.
The actual fix is the patch in sql_class.cc:thd_need_ordering_with(), where
we allow relaxed GAP locking protocol between wsrep high priority threads.
wsrep high priority threads (replication appliers, replayers and TOI processors)
are ordered by the replication provider, and they will not need serializability
support gained by secondary index GAP locks.
PR contains also a mtr test, which exercises a scenario where two replication
applier threads have a false positive conflict in GAP of unique secondary index.
The conflicting local committing transaction has to replay, and the test verifies
also that the replaying phase will not conflict with the latter repllication applier.
Commit also contains new test scenario for galera.galera_UK_conflict.test,
where replayer starts applying after a slave applier thread, with later seqno,
has advanced to commit phase. The applier and replayer have false positive GAP
lock conflict on secondary unique index, and replayer should ignore this.
This test scenario caused crash with earlier version in this PR, and to fix this,
the secondary index uniquenes checking has been relaxed even further.
Now innodb trx_t structure has new member: bool wsrep_UK_scan, which is set to
true, when high priority thread is performing unique secondary index scanning.
The member trx_t::wsrep_UK_scan is defined inside WITH_WSREP directive, to make
it possible to prepare a MariaDB build where this additional trx_t member is
not present and is not used in the code base. trx->wsrep_UK_scan is set to true
only for the duration of function call for: lock_rec_lock() trx->wsrep_UK_scan
is used only in lock_rec_has_to_wait() function to relax the need to wait if
wsrep_UK_scan is set and conflicting transaction is also high priority.
Reviewed-by: Jan Lindström <jan.lindstrom@mariadb.com>
The reason for the failure is that
thd->mdl_context.release_transactional_locks()
was called after commit & rollback even in cases where the current
transaction is still active.
For 10.2, 10.3 and 10.4 the fix is simple:
- Replace all calls to thd->mdl_context.release_transactional_locks() with
thd->release_transactional_locks(). The thd function will only call
the mdl_context function if there are no active transactional locks.
In 10.6 we will better fix where we will change the return value for
some trans_xxx() functions to indicate if transaction did close the
transaction or not. This will avoid the need of the indirect call.
Other things:
- trans_xa_commit() and trans_xa_rollback() will automatically
call release_transactional_locks() if the transaction is closed.
- We can't do that for the other functions as the caller of many of these
are doing additional work (like close_thread_tables) before calling
release_transactional_locks().
- Added missing abort_result_set() and missing DBUG_RETURN in
select_create::send_eof()
- Fixed wrong indentation in injector::transaction::commit()
MDL_lock::Ticket_list::remove_ticket(): reduce algoritmic
complexity from O(N) to O(1)
MDL_lock::Ticket_list::clear_bit_if_not_in_list(): removed
MDL_lock::Ticket_list::m_type_counters: a map of ticket type
to count. Initialization is memset(0) which takes time.
