Problem:
Item_func_conv::val_str() copied the ASCII string with the numeric base
conversion result directly to the function result string. In case of a
tricky character set (e.g. utf32) it produced an illformed string.
Fix:
Copy the base conversion result to the function result as is only if
the function character set is ASCII compatible, go through a
character set conversion otherwise.
Change history in the affected code:
- Since 10.4.8 (MDEV-20397 and MDEV-23311), functions ROUND(), CEILING(),
FLOOR() return a TIME value for a TIME input.
- Since 10.4.14 (MDEV-23525), MIN() and MAX() calculate a result for a TIME
input using val_native() rather than val_str().
Problem:
The patch for MDEV-23525 did not take into account combinations like
MIN(ROUND(time)), MAX(FLOOR(time)), etc.
MIN() and MAX() with ROUND(time), CEILING(time), FLOOR(time) as an argument
call the method val_native() of the undelying classes Item_func_round and
Item_func_int_val. However these classes implemented the method val_native()
as DBUG_ASSERT(0).
Fix:
This patch adds a TIME-specific code inside:
- Item_func_round::val_native()
- Item_func_int_val::val_native()
still with DBUG_ASSERT(0) for all other data types,
as other data types do not call val_native() of these classes.
We'll need a more generic solition eventualy, e.g.
turn Item_func_round and Item_func_int_val into Item_handled_func.
However, this change would be too risky for 10.4 at this point.
The pointer was used deep in the call path.
Resolve this by setting the pointer to NULL at the end of
the function.
Tested with gcc-13.3.1 (fc38)
The warning disable 38fe266ea9 can be reverted in 10.6+ on merge.
The existing (incorrect) overriding mechanism is:
Non-minus-one var value overrides table param overrides default value.
Before MDEV-27169, unspecified var value is -1. So if the user sets
both the var to be a value other than -1 and the table param, the var
value will prevail, which is incorrect.
After MDEV-27169, unspecified var value is default value. So if the
user does not set the var but sets the table param, the default value
will prevail, which is even more incorrect.
This patch fixes it so that table param, if specified, always
overrides var value, and the latter if not specified or set to -1,
falls back to the default value
We achieve this by replacing all such overriding in spd_param.cc with
macros that override in the correct way, and removing all the
"overriding -1" lines involving table params in
spider_set_connect_info_default() except for those table params not
defined as sysvar/thdvar in spd_params.cc
We also introduced macros for non-overriding sysvar and thdvar, so
that the code is cleaner and less error-prone
In server versions where MDEV-27169 has not been applied, we also
backport the patch, that is, replacing -1 default values with real
default values
In server versions where MDEV-28006 has not been applied, we do the
same for udf params
MDEV-31503 ALTER SEQUENCE ends up in optimistic parallel slave binlog out-of-order
The OOO error still was possible even after MDEV-31077. This time
it occured through open_table() when the sequence table was not in
the table cache *and* the table was created before the last server
restart.
In such context a internal (read-only) transaction is committed
and it was not blocked from doing a wakeup() call to subsequent
transactions.
Fixed with extending suspend_subsequent_commits() effect for the entirety
of Sql_cmd_alter_sequence::execute().
An elaborated MDEV-31077 test proves the fixes of both failure scenarios.
Also the bug condition suggests a workaround to pre-SELECT sequence
tables before START SLAVE.
Reviewed-by: Brandon Nesterenko <brandon.nesterenko@mariadb.com>
The case is statement format and mixed InnoDB/MyISAM without
binlog_direct_non_trans_update. Fix due to Brandon Nesterenko.
Reviewed-by: Andrei Elkin <andrei.elkin@mariadb.com>
Signed-off-by: Kristian Nielsen <knielsen@knielsen-hq.org>
The largest_started_sub_id needs to be set under LOCK_parallel_entry
together with testing stop_sub_id. However, in-between was the logic for
do_ftwrl_wait(), which temporarily releases the mutex. This could lead to
inconsistent stopping amongst worker threads and lost data.
Fix by moving all the stop-related logic out from unrelated do_gco_wait()
and do_ftwrl_wait() and into its own function do_stop_handling().
Reviewed-by: Andrei Elkin <andrei.elkin@mariadb.com>
Signed-off-by: Kristian Nielsen <knielsen@knielsen-hq.org>
Various test cases for the bugs around MDEV-31448.
Test cases due to Brandon Nesterenko, thanks!
Reviewed-by: Andrei Elkin <andrei.elkin@mariadb.com>
Signed-off-by: Kristian Nielsen <knielsen@knielsen-hq.org>
The problem is that when a worker thread is (user) killed in
wait_for_prior_commit, the event group may complete out-of-order since the
wait for prior commit was aborted by the kill.
This fix ensures that event groups will always complete in-order, even
in the error case. This is done in finish_event_group() by doing an
extra wait_for_prior_commit(), if necessary, that ignores kills.
This fix supersedes the fix for MDEV-30780, so the earlier fix for
that is reverted in this patch.
Also fix that an error from wait_for_prior_commit() inside
finish_event_group() would not signal the error to
wakeup_subsequent_commits().
Based on earlier work by Brandon Nesterenko and Andrei Elkin, with
some changes to simplify the semantics of wait_for_prior_commit() and
make the code more robust to future changes.
Reviewed-by: Andrei Elkin <andrei.elkin@mariadb.com>
Signed-off-by: Kristian Nielsen <knielsen@knielsen-hq.org>
The problem was an incorrect unmark_start_commit() in
signal_error_to_sql_driver_thread(). If an event group gets an error, this
unmark could run after the following GCO started, and the subsequent
re-marking could access de-allocated GCO.
The offending unmark_start_commit() looks obviously incorrect, and the fix
is to just remove it. It was introduced in the MDEV-8302 patch, the commit
message of which suggests it was added there solely to satisfy an assertion
in ha_rollback_trans(). So update this assertion instead to not trigger for
event groups that experienced an error (rgi->worker_error). When an error
occurs in an event group, all following event groups are skipped anyway, so
the unmark should never be needed in this case.
Reviewed-by: Andrei Elkin <andrei.elkin@mariadb.com>
Signed-off-by: Kristian Nielsen <knielsen@knielsen-hq.org>
At STOP SLAVE, worker threads will continue applying event groups until the
end of the current GCO before stopping. This is a left-over from when only
conservative mode was available. In optimistic and aggressive mode, often
_all_ queued event will be in the same GCO, and slave stop will be
needlessly delayed.
This patch instead records at STOP SLAVE time the latest (highest sub_id)
event group that has started. Then worker threads will continue to apply
event groups up to that event group, but skip any following. The result is
that each worker thread will complete its currently running event group, and
then the slave will stop.
If the slave is caught up, and STOP SLAVE is run in the middle of an event
group that is already executing in a worker thread, then that event group
will be rolled back and the slave stop immediately, as normal.
Reviewed-by: Andrei Elkin <andrei.elkin@mariadb.com>
Signed-off-by: Kristian Nielsen <knielsen@knielsen-hq.org>
Docker when mounting a configuration file into a Windows exposes the
file with permission 0777. These world writable files are ignored by
by MariaDB.
Add the access check such that filesystem RO or immutable file is
counted as sufficient protection on the file.
Test:
$ mkdir /tmp/src
$ vi /tmp/src/my.cnf
$ chmod 666 /tmp/src/my.cnf
$ mkdir /tmp/dst
$ sudo mount --bind /tmp/src /tmp/dst -o ro
$ ls -la /tmp/dst
total 4
drwxr-xr-x. 2 dan dan 60 Jun 15 15:12 .
drwxrwxrwt. 25 root root 660 Jun 15 15:13 ..
-rw-rw-rw-. 1 dan dan 10 Jun 15 15:12 my.cnf
$ mount | grep dst
tmpfs on /tmp/dst type tmpfs (ro,seclabel,nr_inodes=1048576,inode64)
strace client/mariadb --defaults-file=/tmp/dst/my.cnf
newfstatat(AT_FDCWD, "/tmp/dst/my.cnf", {st_mode=S_IFREG|0666, st_size=10, ...}, 0) = 0
access("/tmp/dst/my.cnf", W_OK) = -1 EROFS (Read-only file system)
openat(AT_FDCWD, "/tmp/dst/my.cnf", O_RDONLY|O_CLOEXEC) = 3
The one failing test, but this isn't a regression, just not a total fix:
$ chmod u-w /tmp/src/my.cnf
$ ls -la /tmp/src/my.cnf
-r--rw-rw-. 1 dan dan 18 Jun 16 10:22 /tmp/src/my.cnf
$ strace -fe trace=access client/mariadb --defaults-file=/tmp/dst/my.cnf
access("/etc/ld.so.preload", R_OK) = -1 ENOENT (No such file or directory)
access("/etc/system-fips", F_OK) = -1 ENOENT (No such file or directory)
access("/tmp/dst/my.cnf", W_OK) = -1 EACCES (Permission denied)
Warning: World-writable config file '/tmp/dst/my.cnf' is ignored
Windows test (Docker Desktop ~4.21) which was the important one to fix:
dan@LAPTOP-5B5P7RCK:~$ docker run --rm -v /mnt/c/Users/danie/Desktop/conf:/etc/mysql/conf.d/:ro -e MARIADB_ROOT_PASSWORD=bob quay.io/m
ariadb-foundation/mariadb-devel:10.4-MDEV-27038-ro-mounts-pkgtest ls -la /etc/mysql/conf.d
total 4
drwxrwxrwx 1 root root 512 Jun 15 13:57 .
drwxr-xr-x 4 root root 4096 Jun 15 07:32 ..
-rwxrwxrwx 1 root root 43 Jun 15 13:56 myapp.cnf
root@a59b38b45af1:/# strace -fe trace=access mariadb
access("/etc/ld.so.preload", R_OK) = -1 ENOENT (No such file or directory)
access("/etc/mysql/conf.d/myapp.cnf", W_OK) = -1 EROFS (Read-only file system)
The problematic query outlined a bug in window functions sorting
optimization. When multiple window functions are present in a query,
we sort the sorting key (as defined by PARTITION BY and ORDER BY) from
generic to specific.
SELECT RANK() OVER (ORDER BY const_col) as r1,
RANK() OVER (ORDER BY const_col, a) as r2,
RANK() OVER (PARTITION BY c) as r3,
RANK() OVER (PARTITION BY c ORDER BY b) as r4
FROM table;
For these functions, the sorting we need to do for window function
computations are: [(const_col), (const_col, a)] and [(c), (c, b)].
Instead of doing 4 different sort order, the sorts grouped within [] are
compatible and we can use the most *specific* sort to cover both window
functions.
The bug was caused by an incorrect flagging of which sort is most
specific for a compatible group of functions. In our specific test case,
instead of picking (const_col, a) as the most specific sort, it would
only sort by (const_col), which lead to wrong results for rank function.
By ensuring that we pick the last sort key before an "incompatible sort"
flag is met in our "ordered array of sorting specifications", we
guarantee correct results.
print_summary(): Skip index_ids for which index.pages is 0.
Tablespaces may contain some freed pages that used to refer to indexes
or tables that were dropped.
If procedure is changed in one connection, and other procedure has
already called the initial version of the procedure, the query to
INFORMATION_SCHEMA.PARAMETERS would use obsolete information from sp
cache for that connection. That happens because cache invalidating
method only increments cache version, and does not flush (all) the
cache(s), and changing of a procedure only invalidates cache, and
removes the procedure's cache entry from local thread cache only.
The fix adds the check if sp info obtained from the cache for forming of
results for the query to I_S, is not obsoleted, and does not use it, if
it is.
The test has been added to main.information_schema. It changes the SP in
one connection, and ensures, that the change is seen in the query to the
I_S.PARAMETERS in other connection, that already has called the
procedure before the change.
There was no actual execution of the SQL of a pushed derived table,
which caused "r_rows" to be always displayed as 0 and "r_total_time_ms"
to show inaccurate numbers.
This commit makes a derived table SQL to be executed by the storage
engine, so the server is able to calculate the number of rows returned
and measure the execution time more accurately
PROBLEM:
A deadlock was possible when a transaction tried to "upgrade" an already
held Record Lock to Next Key Lock.
SOLUTION:
This patch is based on observations that:
(1) a Next Key Lock is equivalent to Record Lock combined with Gap Lock
(2) a GAP Lock never has to wait for any other lock
In case we request a Next Key Lock, we check if we already own a Record
Lock of equal or stronger mode, and if so, then we change the requested
lock type to GAP Lock, which we either already have, or can be granted
immediately, as GAP locks don't conflict with any other lock types.
(We don't consider Insert Intention Locks a Gap Lock in above statements).
The reason of why we don't upgrage Record Lock to Next Key Lock is the
following.
Imagine a transaction which does something like this:
for each row {
request lock in LOCK_X|LOCK_REC_NOT_GAP mode
request lock in LOCK_S mode
}
If we upgraded lock from Record Lock to Next Key lock, there would be
created only two lock_t structs for each page, one for
LOCK_X|LOCK_REC_NOT_GAP mode and one for LOCK_S mode, and then used
their bitmaps to mark all records from the same page.
The situation would look like this:
request lock in LOCK_X|LOCK_REC_NOT_GAP mode on row 1:
// -> creates new lock_t for LOCK_X|LOCK_REC_NOT_GAP mode and sets bit for
// 1
request lock in LOCK_S mode on row 1:
// -> notices that we already have LOCK_X|LOCK_REC_NOT_GAP on the row 1,
// so it upgrades it to X
request lock in LOCK_X|LOCK_REC_NOT_GAP mode on row 2:
// -> creates a new lock_t for LOCK_X|LOCK_REC_NOT_GAP mode (because we
// don't have any after we've upgraded!) and sets bit for 2
request lock in LOCK_S mode on row 2:
// -> notices that we already have LOCK_X|LOCK_REC_NOT_GAP on the row 2,
// so it upgrades it to X
...etc...etc..
Each iteration of the loop creates a new lock_t struct, and in the end we
have a lot (one for each record!) of LOCK_X locks, each with single bit
set in the bitmap. Soon we run out of space for lock_t structs.
If we create LOCK_GAP instead of lock upgrading, the above scenario works
like the following:
// -> creates new lock_t for LOCK_X|LOCK_REC_NOT_GAP mode and sets bit for
// 1
request lock in LOCK_S mode on row 1:
// -> notices that we already have LOCK_X|LOCK_REC_NOT_GAP on the row 1,
// so it creates LOCK_S|LOCK_GAP only and sets bit for 1
request lock in LOCK_X|LOCK_REC_NOT_GAP mode on row 2:
// -> reuses the lock_t for LOCK_X|LOCK_REC_NOT_GAP by setting bit for 2
request lock in LOCK_S mode on row 2:
// -> notices that we already have LOCK_X|LOCK_REC_NOT_GAP on the row 2,
// so it reuses LOCK_S|LOCK_GAP setting bit for 2
In the end we have just two locks per page, one for each mode:
LOCK_X|LOCK_REC_NOT_GAP and LOCK_S|LOCK_GAP.
Another benefit of this solution is that it avoids not-entirely
const-correct, (and otherwise looking risky) "upgrading".
The fix was ported from
mysql/mysql-server@bfba840dfamysql/mysql-server@75cefdb1f7
Reviewed by: Marko Mäkelä
"mtr --view-protocol func_math" failed because of a too long
column names imlicitly generated for the underlying expressions.
With --view-protocol they were replaced to "Name_exp_1".
Adding column aliases for these expressions.
Fix that rpl_slave_state::load() was calling rpl_slave_state::update() without
holding LOCK_slave_state.
Reviewed-by: Monty <monty@mariadb.org>
Signed-off-by: Kristian Nielsen <knielsen@knielsen-hq.org>
This fixes mdev_26541.test, and the new clean_up_spider.inc will be
useful for other tests where part of deinit_spider does not apply,
e.g. those testing spider initialisation only.
'long long int'; cast to an unsigned type to negate this value ..
to itself in Item_func_mul::int_op and Item_func_round::int_op
Problems:
The code in multiple places in the following methods:
- Item_func_mul::int_op()
- longlong Item_func_int_div::val_int()
- Item_func_mod::int_op()
- Item_func_round::int_op()
did not properly check for corner values LONGLONG_MIN
and (LONGLONG_MAX+1) before doing negation.
This cuased UBSAN to complain about undefined behaviour.
Fix summary:
- Adding helper classes ULonglong, ULonglong_null, ULonglong_hybrid
(in addition to their signed couterparts in sql/sql_type_int.h).
- Moving the code performing multiplication of ulonglong numbers
from Item_func_mul::int_op() to ULonglong_hybrid::ullmul().
- Moving the code responsible for extracting absolute values
from negative numbers to Longlong::abs().
It makes sure to perform negation without undefinite behavior:
LONGLONG_MIN is handled in a special way.
- Moving negation related code to ULonglong::operator-().
It makes sure to perform negation without undefinite behavior:
(LONGLONG_MAX + 1) is handled in a special way.
- Moving signed<=>unsigned conversion code to
Longlong_hybrid::val_int() and ULonglong_hybrid::val_int().
- Reusing old and new sql_type_int.h classes in multiple
places in Item_func_xxx::int_op().
Fix details (explain how sql_type_int.h classes are reused):
- Instead of straight negation of negative "longlong" arguments
*before* performing unsigned multiplication,
Item_func_mul::int_op() now calls ULonglong_null::ullmul()
using Longlong_hybrid_null::abs() to pass arguments.
This fixes undefined behavior N1.
- Instead of straight negation of "ulonglong" result
*after* performing unsigned multiplication,
Item_func_mul::int_op() now calls ULonglong_hybrid::val_int(),
which recursively calls ULonglong::operator-().
This fixes undefined behavior N2.
- Removing duplicate negating code from Item_func_mod::int_op().
Using ULonglong_hybrid::val_int() instead.
This fixes undefinite behavior N3.
- Removing literal "longlong" negation from Item_func_round::int_op().
Using Longlong::abs() instead, which correctly handler LONGLONG_MIN.
This fixes undefinite behavior N4.
- Removing the duplicate (negation related) code from
Item_func_int_div::val_int(). Reusing class ULonglong_hybrid.
There were no undefinite behavior in here.
However, this change allowed to reveal a bug in
"-9223372036854775808 DIV 1".
The removed negation code appeared to be incorrect when
negating +9223372036854775808. It returned the "out of range" error.
ULonglong_hybrid::operator-() now handles all values correctly
and returns +9223372036854775808 as a negation for -9223372036854775808.
Re-recording wrong results for
SELECT -9223372036854775808 DIV 1;
Now instead of "out of range", it returns -9223372036854775808,
which is the smallest possible value for the expression data type
(signed) BIGINT.
- Removing "no UBSAN" branch from Item_func_splus::int_opt()
and Item_func_minus::int_opt(), as it made UBSAN happy but
in RelWithDebInfo some MTR tests started to fail.
- When foreign_key_check is disabled, allowing to modify the
column which is part of foreign key constraint can lead to
refusal of TRUNCATE TABLE, OPTIMIZE TABLE later. So it make
sense to block the column modify operation when foreign key
is involved irrespective of foreign_key_check variable.
Correct way to modify the charset of the column when fk is involved:
SET foreign_key_checks=OFF;
ALTER TABLE child DROP FOREIGN KEY fk, MODIFY m VARCHAR(200) CHARSET utf8mb4;
ALTER TABLE parent MODIFY m VARCHAR(200) CHARSET utf8mb4;
ALTER TABLE child ADD CONSTRAINT FOREIGN KEY (m) REFERENCES PARENT(m);
SET foreign_key_checks=ON;
fk_check_column_changes(): Remove the FOREIGN_KEY_CHECKS while
checking the column change for foreign key constraint. This
is the partial revert of commit 5f1f2fc0e4
and it changes the behaviour of copy alter algorithm
ha_innobase::prepare_inplace_alter_table(): Find the modified
column and check whether it is part of existing and newly
added foreign key constraint.
spider_db_mbase_util::open_item_func() is a monster function.
It is difficult to maintain while it is expected that we need to
modify it when a new SQL function or a new func_type is added.
We split the function into two distinct functions: one handles the
case of str != NULL and the other handles the case of str == NULL.
This refactoring was done in a conservative way because we do not
have comprehensive tests on the function.
It also fixes MDEV-29447 and MDEV-31338 where field items that are
arguments of a func item may be used before created / initialised.
Note this commit is adapted from a patch by Nayuta for MDEV-26285.
sql/log.cc:11101:56: runtime error: downcast of address 0x7f9dc801e9c8 which does not point to an object of type 'Gtid_list_log_event'
sql/sql_repl.cc:1429:12: runtime error: member call on address 0x7f1ca401ea48 which does not point to an object of type 'Gtid_list_log_event'
