This merges the patch series that fixes a lot of missing validation
checks for read_log_event(). The merge includes a few added fixes that
are needed in new code introduced in 12.3.
Signed-off-by: Kristian Nielsen <knielsen@knielsen-hq.org>
get_clone() returns NULL on out-of-memory, but two of its callers in
prep_alter_part_table() dereferenced the result without a check, so the
server crashed:
changed INTERVAL of SYSTEM_TIME partitioning
ADD PARTITION under LOCK TABLES
get_clone() itself did not check the allocation of Vers_part_info and
dereferenced it.
handle_alter_part_error() did not check the clone either. The clone only
kept the DDL log entries after the table is closed. In the branch where
DDL log recovery fails, write_log_completed() released the entries
through lpt->part_info, which the table close may have freed, and
release_log_entries() released the same entries again through the
clone. That branch is not reachable now, as ddl_log_execute_entry()
always returns false.
The fix checks the allocation of Vers_part_info in get_clone() and the
result of get_clone() in prep_alter_part_table() like the other callers
there do. handle_alter_part_error() keeps a copy of DDL_LOG_STATE
instead of the clone and passes it to write_log_completed() and
release_log_entries(), which now take DDL_LOG_STATE. The redundant
release_log_entries() after write_log_completed() is removed.
ALTER TABLE ... PARTITION BY SYSTEM_TIME with a changed INTERVAL or
STARTS on a table already partitioned by SYSTEM_TIME took the fast
ALTER path, which only rewrites the table definition. The history rows
stayed where the old layout put them, so after the ALTER they were in
wrong partitions. An unchanged interval took the copy path instead.
MDEV-29873 replaced memcmp() of the intervals, which is true when they
differ, with interval_t::operator==, which is true when they are equal,
and lost the negation.
The copy path was also broken: it assigned the whole vers_info of the
new partitioning spec to the cloned partition info, now_part and
hist_part included. The ALTER does not list partitions, so now_part
became NULL and check_partition_info() failed with ER_VERS_WRONG_PARTS
for a table with explicit partitions (MDEV-33370 test case).
The fix negates the comparison again (now with interval_t::operator!=),
so a changed interval rebuilds the table and redistributes the history.
It copies only interval, limit and auto_hist from the new spec and keeps
now_part and hist_part of the existing partitions.
Without STARTS the start of the interval defaults to the current time
rounded down to the INTERVAL unit. So ALTER with the same INTERVAL and
no STARTS usually gets a different start and now rebuilds the table.
Vers_part_info::operator=, which copied now_part and hist_part and had
no other callers, is deleted.
Fast ALTER was also chosen when the same statement changed the table
definition, e.g. ADD COLUMN with the same INTERVAL and STARTS, or DROP
COLUMN with LIMIT. It wrote the new .frm, but the partitions kept the
old record layout. Now any change of the table definition
(alter_info->flags) takes the copy path.
CONVERT ... PARTITION passed user-specified partition names to
create_partition_name() with translate=FALSE, so names needing
filename escaping (e.g. `foo-bar`) didn't match the on-disk file.
Pass TRUE to encode them.
The ALTER TABLE.. CONVERT TABLE TO invalided the table cache of the
previously normal table that was moved and which left the query cache refering
to a removed table by the time it was invalidated after the ALTER.
Let's invalidate the query cache at the beginning of the fast path for
all variants of the ALTER TABLE (partitioning).
This prevents a use after free ASAN error that was previous there when the
query_cache_invalidate was use on the source table that was freed from the
table cache.
Alternates considered:
* A query cache invalidate early CONVERT_IN and leave the
fast_end_partition invalidate - still results in ASAN assertion.
* A fast_end_partition with an argument to skip QC clear -
seemed to treat this special ALTER TABLE too specially.
Review: Sanja Byelkin
paritioning has its own method for printing engine options,
it wasn't consistent with how table options were printed - didn't
use quotes, ignored sql_mode, so wasn't future and dump/import safe.
let's get rid of it and use the same method that prints per-table
engine options.
let's simplify the code by removing copying of part_elem->connect_string
to table->s->connect_string. Move CONNECTION support from he server into
engines, engines define TOPTION("CONNECTION") if they want to support it,
it's stored in the option_struct and handled by the option_struct fix.
Problems:
* Mroonga used CONNECTION for something but there were no tests for it.
Removed Mroonga support for CONNECTION as requested by the maintainer
in MDEV-38530
* DROP/ALTER SERVER command used to close all tables using the server
in question, avoiding the need for FLUSH TABLES. This functionality
never worked for partitioned tables anyway and is now removed. Will be
done properly in MDEV-37827.
remove copying of part_elem->option_struct to table->s->option_struct.
It meant that most of the time a handler had no access to its options
(that's why some handlers resorted to a complex process of guessing the
correct partition) and couldn't work concurrenty. Instead
* introduce handler::option_struct which will point to
table->s->option_struct or part_elem->option_struct as appropriate,
the handler can use it any time as needed. A handler should not use
table->s->option_struct unless it really knows what it's doing and has
verified that it doesn't break partitioning
* rename option_struct in table->s to option_struct_table and in
part_elem to option_struct_part to emphasize it's semantics and
prevent incorrect usage.
* fix engines accordingly.
PARTITION BY [LINEAR] KEY ALGORITHM={MYSQL51|MYSQL55|BASE31|CRC32C|XXH32|XXH3}
- The MYSQL5X algorithms are the existing algorithms, with MYSQL55
being the default
- The BASE31 algorithm uses a base-31 representation of the bytes, see
Modular hashing in https://algs4.cs.princeton.edu/34hash/. It serves
as a simple baseline that distributes better than the old mysql5x
algorithms
- CRC32C uses my_crc32c.
- XXH32 and XXH3 are xxhash algorithms - xxhash.h copied from latest
release (0.8.3) of https://github.com/Cyan4973/xxHash
For performance (esp. xxh) we use one-shot hash functions in binary
hash_sort, and streaming hash function otherwise for byte-by-byte
hashing. XXH is the only stateful hash function. The other hash
algorithms are stateless and homomorphic, so streaming and one-shot
functions are identical.
Also updated the columnstore hash due to the change of MY_HASH_ADD
signature.
The following testing commands were run at an earlier version of the
patch with the following patch that changes the default algorithm from
MYSQL55 to CRC32C, and XXH32 (changing the patch accordingly)
mtr --suite main --do-test=.*partition
mtr --suite parts
modified sql/ha_partition.cc
@@ -10336,6 +10336,8 @@ uint32 ha_partition::calculate_key_hash_value(Field **field_array)
switch ((*field_array)->table->part_info->key_algorithm)
{
case partition_info::KEY_ALGORITHM_NONE:
+ hasher.set_algorithm(HASH_ALGORITHM_CRC32C);
+ break;
case partition_info::KEY_ALGORITHM_55:
/* Hasher default to mysql55 */
break;
modified sql/partition_info.cc
@@ -2328,7 +2328,7 @@ bool partition_info::fix_parser_data(THD *thd)
if ((thd_sql_command(thd) == SQLCOM_CREATE_TABLE ||
thd_sql_command(thd) == SQLCOM_ALTER_TABLE) &&
key_algorithm == KEY_ALGORITHM_NONE)
- key_algorithm= KEY_ALGORITHM_55;
+ key_algorithm= PARTITION_INFO_DEFAULT_ALGORITHM;
}
DBUG_RETURN(FALSE);
}
@@ -2344,7 +2344,7 @@ bool partition_info::fix_parser_data(THD *thd)
if ((thd_sql_command(thd) == SQLCOM_CREATE_TABLE ||
thd_sql_command(thd) == SQLCOM_ALTER_TABLE) &&
key_algorithm == KEY_ALGORITHM_NONE)
- key_algorithm= KEY_ALGORITHM_55;
+ key_algorithm= PARTITION_INFO_DEFAULT_ALGORITHM;
}
defined_max_value= FALSE; // in case it already set (CREATE TABLE LIKE)
do
modified sql/partition_info.h
@@ -446,6 +446,8 @@ class partition_info : public DDL_LOG_STATE, public Sql_alloc
int gen_part_type(THD *thd, String *str) const;
};
+#define PARTITION_INFO_DEFAULT_ALGORITHM partition_info::KEY_ALGORITHM_CRC32C
+
void part_type_error(THD *thd, partition_info *work_part_info,
const char *part_type, partition_info *tab_part_info);
modified sql/sql_partition.cc
@@ -2471,7 +2471,7 @@ static int add_key_with_algorithm(String *str, const partition_info *part_info)
err+= str->append(STRING_WITH_LEN("KEY "));
if (part_info->key_algorithm != partition_info::KEY_ALGORITHM_NONE &&
- part_info->key_algorithm != partition_info::KEY_ALGORITHM_55)
+ part_info->key_algorithm != PARTITION_INFO_DEFAULT_ALGORITHM)
{
err+= str->append(STRING_WITH_LEN("ALGORITHM = "));
switch (part_info->key_algorithm)
@@ -2479,6 +2479,9 @@ static int add_key_with_algorithm(String *str, const partition_info *part_info)
case partition_info::KEY_ALGORITHM_51:
err+= str->append(STRING_WITH_LEN("MYSQL51"));
break;
+ case partition_info::KEY_ALGORITHM_55:
+ err+= str->append(STRING_WITH_LEN("MYSQL55"));
+ break;
case partition_info::KEY_ALGORITHM_BASE31:
err+= str->append(STRING_WITH_LEN("BASE31"));
break;
1. Fix empty part_elem->id in prep_alter_part_table().
On auto-create newly added partition has id 0. It came from
set_up_default_partitions() for new part_info
(thd->work_part_info). vers_update_el_ids() can work only with
unassigned ids (UINT_MAX32), so we assign it explicitly on pushing
into tab_part_info.
2. If range value is out of TIMESTAMP_MAX_VALUE set it to
TIMESTAMP_MAX_VALUE, but only if the history partition is the last
one, otherwise push ER_DATA_OUT_OF_RANGE. Error is to create
multiple out-of-range partitions (e.g. with PARTITIONS clause in
CREATE TABLE).
Wrong "fastpath" boundary conditions. The right boundary is open, the
left boundary is closed. The condition range_value[loc_hist_id - 1] as
left boundary must be checked with <= operator. The same with the
right boundary range_value[loc_hist_id] as the correct way is to check
it with > operator, the right boundary check was rewritten for clear
understanding:
ts < range_value[loc_hist_id]
The below code confirms closed endpoint type for left boundary:
if (range_value[loc_hist_id] <= ts)
min_hist_id= loc_hist_id + 1;
Also the endpoint type (closed for left, open for right) is confirmed
by vers_set_hist_part() for DML:
else if (vers_info->interval.is_set() &&
vers_info->hist_part->range_value <= thd->query_start())
and here (right boundary, rewritten for clarity):
if (thd->query_start() < next->range_value)
{
error= false;
break;
}
- 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.
main/statistics_json.result is updated for f8ba5ced55 (MDEV-36099)
The test uses 'delete from t1' in many places and then populates
the table again. The natural order of rows in a MyISAM table is well
defined and the test was implicitly relying on that.
before f8ba5ced55 delete was deleting rows one by one, using
ha_myisam::delete_row() because the connection was stuck in rbr mode.
This caused rows to be shown in the reverse insertion order (because of
the delete link list).
MDEV-36099 fixes this bug and the server now correctly uses
ha_myisam::delete_all_rows(). This makes rows to be shown in the
insertion order as expected.
MDEV-33658 part 1’s refactoring ecaedbe299
introduced a new function init_key_info which (in part) aims to
calculate the total key length; however, it doesn’t account for the
key already having been initialized (as happens when called via
ALTER TABLE .. CONVERT PARTITION .. TO TABLE). This leads to crashes
when this key is later iterated over, because the iterator will try
to iterate over additional key parts which don’t exist because the
length reports as longer than the actual memory owned. The crash
reported by MDEV-36906 highlights this in function key_copy.
To explain how the keys already have been initialized, init_key_info
is called multiple times. That is, init_key_info is called from
mysql_prepare_create_table, which prepares a table and its key
structures for table creation, which is in turn called by
mysql_write_frm when using flags MFRM_WRITE_SHADOW and
MFRM_WRITE_CONVERTED_TO. The
ALTER TABLE .. CONVERT PARTITION .. TO TABLE use case (see function
fast_alter_partition_table), calls mysql_write_frm multiple times with
both of these flags set (first with MFRM_WRITE_CONVERTED_TO and then
with MFRM_WRITE_SHADOW).
Raising it up a level, mysql_prepare_create_table doesn't need to be
called again after it has already been invoked when just writing frms.
Init_key_info is the only place in that function which leads to side
effects, but the rest is redundant and can be skipped on the second
call (i.e. when writing the shadow).
The patch fixes this by skipping the call to mysql_prepare_create_table
in mysql_write_frm in the MFRM_WRITE_SHADOW block when it has already
been called previously. To track whether or not it has been previously
called, we add a new flag for the mysql_write_frm function,
MFRM_ALTER_INFO_PREPARED, which is hard-coded into the function call on
the later invocation.
Test case based on work by Elena Stepanova <elenst@mariadb.com>
Reviewed By:
============
Sergei Golubchik <serg@mariadb.org>
Nikita Malyavin <nikita.malyavin@mariadb.com>
Remove not anymore used
bool ddl_log_write_execute_entry(uint first_entry,
DDL_LOG_MEMORY_ENTRY **active_entry)
Simple transformations, no logic changes.
