- developers - lists.mariadb.org

Re: 0b403e03939: MDEV-35661 Assertion `!cache_list ||...' fails in online_alter_close_connection
by Sergei Golubchik 22 Jan '25

22 Jan '25

Hi, Nikita, On Jan 19, Nikita Malyavin wrote: > revision-id: 0b403e03939 (mariadb-11.7.1-16-g0b403e03939) > parent(s): 40e502eca4c > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2025-01-03 00:54:52 +0100 > message: > > MDEV-35661 Assertion `!cache_list ||...' fails in > online_alter_close_connection > > After MDEV-35182 (88adcbf3) fix, online_alter still wasn't fully ready > for `XA PREPARE` to be skipped. > > In fact, according to XA standard, even xa_commit() call may happen > without preceding xa_prepare(), so this stage is completely optional. > > This commit removes any relying on xa_prepare() call. The only thing > that will be done there is an equivalent of "statement-level commit", > which basically just flushes a pending rows event to an online alter > cache. > > This is done by always storing an online alter cache in the XA_data > object whenever XA is active, rather than in THD::ha_data. Why? I thought it'd be simpler to keep the cache in the THD and take it off it in commit or rollback. That is, almost as now, but commit/rollback won't rely on prepare being called before. Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 1

Re: 33e36944232: MDEV-30111 InnoDB: Failing assertion: update->n_fields == 0 in row_ins_sec_index_entry_by_modify
by Sergei Golubchik 22 Jan '25

22 Jan '25

Hi, Alexander, On Jan 22, Alexander Barkov wrote: > revision-id: 33e36944232 (mariadb-10.6.20-19-g33e36944232) > parent(s): 25be7da2024 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2024-11-14 10:19:24 +0400 > message: > > MDEV-30111 InnoDB: Failing assertion: update->n_fields == 0 in row_ins_sec_index_entry_by_modify > > Also fixes: > > MDEV-32190 Index corruption with unique key and nopad collation (without DESC or HASH keys) > > The code in strings/strcoll.inl when comparing an empty string > to a string like 0x0001050001 did not take into account > that the leftmost weight in the latter can be zero, while > there are some more weights can follow the zero weight. > > Rewriting the code to treat the shorter string as smaller than > a longer string. > > --- a/strings/strcoll.inl > +++ b/strings/strcoll.inl > @@ -192,10 +192,10 @@ MY_FUNCTION_NAME(strnncoll)(CHARSET_INFO *cs __attribute__((unused)), > >0 >0 Two weights were scanned, check weight difference. > */ > if (!a_wlen) > - return b_wlen ? -b_weight : 0; > + return b_wlen ? -1 : 0; > > if (!b_wlen) > - return b_is_prefix ? 0 : a_weight; > + return b_is_prefix ? 0 : +1; > > if ((res= (a_weight - b_weight))) > return res; Sorry, I don't understand it. The function returns >0, 0, or <0. It shouldn't matter whether it returns -b_weight or -1, a_weight or +1. Except if the weight can be <= 0 itself. But in that case, if ((res= (a_weight - b_weight))) return res; is rather suspicious too. Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

On the question on producing bintars going forward
by Kristian Nielsen 22 Jan '25

22 Jan '25

Hi Vicentiu, I read through your proposal for how to handle MariaDB bintars. I know a lot of the old history on this (like from working in the MySQL build team back in 2005), I also know first-hand of the tough spot between what people think is "easily possible" and the practicalities of packaging for real linux systems. So I hope my input can be useful in some way: 1. To me, providing bintars is not important. Nor is providing any binaries for that matter. MariaDB is open source, the most important thing is it is easy to compile. Others can provide binaries as needed, and in fact distros are already doing so very well. So I personally am fine with not releasing bintars going forward. I know binaries are considered important by others. 2. I agree a clear definition is lacking of exactly what binaries should be/provide. My opinion for bintars is that they are a way to use MariaDB easily similar to compiling the source, but without the need for having installed a compiler toolchain. So bintars should be the result of a mostly default compile of the source, on a stable/oldstable linux distro like Debian or some redhat with minimal optional dependencies installed. Most work should go into making default compile do the right thing, as opposed to endless tweaking in build scripts or Buildbot for how bintars are compiled. 3. In your proposal, I think there is too much focus (out of good intentions, I'm sure) about solving all possible problems. No install method is perfect for all use cases. Bintars provide very simple install or even easy-of-use without installing, but they have other limitations. Let us not try to make bintars "perfect". Rather make them simple to produce, and simple to use in the limited scenarious where they can work well. 4. Let us firmly give up on building static binaries. Static binaries do not work well, and have not done so in many years. Here is a reference for a problem with linking libc statically: https://sourceware.org/glibc/wiki/FAQ#Even_statically_linked_programs_need_… And mixing static and dynamic linking creates some very serious problems, see MDEV-32251 and MDEV-25633. We should just build a default `cmake` binary with dynamic linking against the required library dependencies, do the build on a minimal linux where most optional dependencies are not installed. This will then work well on user's systems that have compatible libraries. On systems that have not, there are other options for running MariaDB. 5. I think we should _definitely_ not maintain an infrastructure for building our own static versions of various system libraries / 3rd-party libraries, see also (4). 6. I agree with concerns about too much effort needed to maintain bintars for many years. Let us be pragmatic, make things simple, and only do what can be done with small amount of effort. Build the bintars on stable linux systems for compatibility, and use default installs as much as possible for simplicity. When those systems become too old to easily continue building bintars, just stop building them. Existing users can easily transition to compiling themselves if they really need, thanks to (2); or use another of the different options available for running MariaDB. And finally Vicentiu, let me commend you for taking responsibility of the task of producing bintars and working so constructively on getting to a working result. It is a task the difficulty of which is easily underappreciated. So these are my opinions and suggestions, hope this helps. - Kristian.

2 1

Re: d47b1ebc06d: MDEV-35326: Memory Leak in init_io_cache_ext upon SHUTDOWN
by Sergei Golubchik 10 Jan '25

10 Jan '25

Hi, Oleksandr, On Jan 09, Oleksandr Byelkin wrote: > revision-id: d47b1ebc06d (mariadb-10.5.27-143-gd47b1ebc06d) > parent(s): 3bbbeae7924 > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2025-01-08 14:14:10 +0100 > message: > > MDEV-35326: Memory Leak in init_io_cache_ext upon SHUTDOWN > > The problems were that: > 1) resources was freed "asimetric" normal execution in send_eof, > in case of error in destructor. > 2) destructor was not called in case of SP for result objects. > (so if the last SP execution ended with error resorces was not > freeded on reinit before execution (cleanup() called before next > execution) and destructor also was not called due to lack of > delete call for the object) > > All result method revised and freeing resources made "symetric". > > Destructor of result object called for SP. > > Added skipped invalidation in case of error in insert. > > Removed misleading naming of reset(thd) (could be mixed with > with reset()) > > diff --git a/sql/sp_head.cc b/sql/sp_head.cc > --- a/sql/sp_head.cc > +++ b/sql/sp_head.cc > @@ -3665,6 +3665,18 @@ int sp_lex_keeper::cursor_reset_lex_and_exec_core(THD *thd, uint *nextp, > return res; > } > > +sp_lex_keeper::~sp_lex_keeper() > +{ > + if (m_lex_resp) > + { > + /* Prevent endless recursion. */ > + m_lex->sphead= NULL; > + if (m_lex->result) you don't need this if(), the standard explicitly says that delete of a NULL pointer is valid. > + delete m_lex->result; > + lex_end(m_lex); > + delete m_lex; > + } > +} > > /* > sp_instr class functions > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -3192,10 +3193,22 @@ bool select_send::send_eof() > Handling writing to file > ************************************************************************/ > > +bool select_to_file::free_recources() > +{ > + if (file >= 0) > + { > + (void) end_io_cache(&cache); > + bool error= mysql_file_close(file, MYF(MY_WME)); > + file= -1; > + return error; > + } > + return FALSE; > +} > + > bool select_to_file::send_eof() > { > int error= MY_TEST(end_io_cache(&cache)); > - if (unlikely(mysql_file_close(file, MYF(MY_WME))) || > + if (unlikely(free_recources()) || free_recources() incluses end_io_cache(), you don't need to call it separately above. > unlikely(thd->is_error())) > error= true; > > @@ -5934,7 +5935,15 @@ class select_to_file :public select_result_interceptor { > { path[0]=0; } > ~select_to_file(); > bool send_eof() override; > + void abort_result_set() override; > void cleanup() override; > + /* > + It is separated from cleanup() becaouse: 1. typo 2. if you rename cleanup() to reset_for_next_ps_execution() then you won't need that comment, because, obviously, "freeing resources" is distinct from "resetting for next ps execution" but it's not obvious that "freeing resources" is distinct from "cleanup" > + 1. cleanup can not be called (conventional execution) > + 2. cleanup not only free recources but reset counters (some sends > + counter updates in destructor) > + */ > + bool free_recources(); > }; > > > diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc > --- a/sql/sql_delete.cc > +++ b/sql/sql_delete.cc > @@ -1422,6 +1422,13 @@ void multi_delete::abort_result_set() > { > DBUG_ENTER("multi_delete::abort_result_set"); > > + /**************************************************************************** > + > + NOTE: if you change here be aware that almost the same code is in > + multi_delete::send_eof(). > + > + ***************************************************************************/ Let's talk about it separately. > + > /* the error was handled or nothing deleted and no side effects return */ > if (error_handled || > (!thd->transaction->stmt.modified_non_trans_table && !deleted)) > diff --git a/sql/sql_update.cc b/sql/sql_update.cc > --- a/sql/sql_update.cc > +++ b/sql/sql_update.cc > @@ -2743,6 +2743,13 @@ void multi_update::abort_result_set() > (!thd->transaction->stmt.modified_non_trans_table && !updated))) > return; > > + /**************************************************************************** > + > + NOTE: if you change here be aware that almost the same code is in > + multi_delete::send_eof(). multi_update::send_eof() > + > + ***************************************************************************/ > + > /* Something already updated so we have to invalidate cache */ > if (updated) > query_cache_invalidate3(thd, update_tables, 1); Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: 331430a6a73: MDEV-33658 2/2 Cannot add a foreign key on a table with a matching long UNIQUE
by Sergei Golubchik 09 Jan '25

09 Jan '25

Hi, Nikita, On Jan 01, Nikita Malyavin wrote: > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index de688b4cb31..2ffceb3932b 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -217,6 +218,16 @@ Foreign_key::Foreign_key(const Foreign_key &rhs, MEM_ROOT *mem_root) > > bool is_foreign_key_prefix(Key *a, Key *b) > { > + ha_key_alg a_alg= a->key_create_info.algorithm; > + ha_key_alg b_alg= b->key_create_info.algorithm; > + > + // The real algorithm will be BTREE is none was given by user. Technically, this is not correct, the algorithm will be whatever the engine wants. For MEMORY engine it'll be HASH. But only InnoDB can do foreign keys now, and for InnoDB th3 default key algorithm is BTREE. Let's reflect it in the comment, like, for example (also fixed a typo "is" -> "if") // The real algorithm in InnoDB will be BTREE if none was given by user > + a_alg= a_alg == HA_KEY_ALG_UNDEF ? HA_KEY_ALG_BTREE : a_alg; > + b_alg= b_alg == HA_KEY_ALG_UNDEF ? HA_KEY_ALG_BTREE : b_alg; > + > + if (a_alg != b_alg) > + return false; > + > /* Ensure that 'a' is the generated key */ > if (a->generated) > { > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 3f035defbd3..d7aeb5d7cbf 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -3556,6 +3556,303 @@ key_add_part_check_null(const handler *file, KEY *key_info, > } > > > +static > +my_bool key_check_without_overlaps(THD *thd, HA_CREATE_INFO *create_info, > + Alter_info *alter_info, > + Key &key) > +{ > + DBUG_ENTER("key_check_without_overlaps"); > + if (!key.without_overlaps) > + DBUG_RETURN(FALSE); > + // append_system_key_parts is already called, so we should check all the > + // columns except the last two. > + const auto &period_start= create_info->period_info.period.start; > + const auto &period_end= create_info->period_info.period.end; > + List_iterator<Key_part_spec> part_it_forwarded(key.columns); > + List_iterator<Key_part_spec> part_it(key.columns); > + part_it_forwarded++; > + part_it_forwarded++; > + while (part_it_forwarded++) > + { > + Key_part_spec *key_part= part_it++; > + > + if (period_start.streq(key_part->field_name) > + || period_end.streq(key_part->field_name)) > + { > + my_error(ER_KEY_CONTAINS_PERIOD_FIELDS, MYF(0), key.name.str, > + key_part->field_name.str); > + DBUG_RETURN(TRUE); > + } > + } It was simpler to have this check inside append_system_key_parts(), why did you move it? > + > + if (key.key_create_info.algorithm == HA_KEY_ALG_HASH || > + key.key_create_info.algorithm == HA_KEY_ALG_LONG_HASH) > + > + { > + my_error(ER_KEY_CANT_HAVE_WITHOUT_OVERLAPS, MYF(0), key.name.str); > + DBUG_RETURN(TRUE); > + } <...> > + KEY_CREATE_INFO *key_cinfo= &key.key_create_info; > + > + if (is_hash_field_needed) > + { > + if (key_cinfo->algorithm == HA_KEY_ALG_UNDEF) > + key_cinfo->algorithm= HA_KEY_ALG_LONG_HASH; Perhaps you shouldn't set key algorithm in add_hash_field() then? or, rather replace the assignment with an assert? > + > + if (key_cinfo->algorithm != HA_KEY_ALG_HASH && > + key_cinfo->algorithm != HA_KEY_ALG_LONG_HASH) > + { > + my_error(ER_TOO_LONG_KEY, MYF(0), max_key_length); > + DBUG_RETURN(TRUE); > + } > + } > + } > + } > + DBUG_RETURN(FALSE); > +} > + > + > /* > Preparation for table creation Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 2

MDEV-29744 fix required when merging to 11.4
by Kristian Nielsen 09 Jan '25

09 Jan '25

Hi Sanja, others who might need to merge 10.6->10.11->11.4: The patch for MDEV-29744 in 10.6 needs a fix when merged to 11.4, otherwise test binlog.max_binlog_total_size (at least) will fail. I have attached the required fix. I could not push this fix to 11.4 myself, as branch protection is broken and rejected my push on a false positive. - Kristian.

1 0

Re: 87188cd3309: MDEV-29744: Fix incorrect locking order of LOCK_log/LOCK_commit_ordered and LOCK_global_system_variables
by Sergei Golubchik 08 Jan '25

08 Jan '25

Hi, Kristian, Mostly ok, but I'd prefer not to access max_binlog_size without any protection, so suggest to do something like ulong saved= max_binlog_size; mysql_mutex_unlock(&LOCK_global_system_variables); mysql_bin_log.set_max_size(saved); mysql_mutex_lock(&LOCK_global_system_variables); On Jan 08, Kristian Nielsen wrote: > revision-id: 87188cd3309 (mariadb-10.6.20-47-g87188cd3309) > parent(s): 7372ecc3965 > author: Kristian Nielsen > committer: Kristian Nielsen > timestamp: 2024-12-05 09:51:17 +0100 > message: > > MDEV-29744: Fix incorrect locking order of LOCK_log/LOCK_commit_ordered and LOCK_global_system_variables > > --- a/sql/sys_vars.cc > +++ b/sql/sys_vars.cc > @@ -1660,7 +1660,9 @@ Sys_max_binlog_stmt_cache_size( > > static bool fix_max_binlog_size(sys_var *self, THD *thd, enum_var_type type) > { > + mysql_mutex_unlock(&LOCK_global_system_variables); > mysql_bin_log.set_max_size(max_binlog_size); > + mysql_mutex_lock(&LOCK_global_system_variables); > return false; > } > static Sys_var_on_access_global<Sys_var_ulong, Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 1

Why is innodb-log-file-mmap is available only on amd64/arm64/ppc64el but not other Linux architectures?
by Otto Kekäläinen 08 Jan '25

08 Jan '25

Hi, The documentation at https://mariadb.com/kb/en/innodb-system-variables/#innodb_log_file_mmap says this is supposed to be enabled by default on Linux, regardless of the architecture. However, in my testing it is not available on all architectures. Can somebody help me read the code path of innodb-log-file-mmap to figure out why the visibility/availability of this option it is currently architecture-dependent? ## Context In Debian I maintain an extra test called "configuration-trace" which basically dumps the output of these four commands into a file: /usr/bin/mariadb --print-defaults /usr/bin/mariadb --verbose --help /usr/sbin/mariadbd --print-defaults /usr/sbin/mariadbd --verbose --help Then, using the static file containing the text output of these files, it compares to the output of these commands on a running system to detect if the MariaDB defaults or features randomly changed (for example due to changes in dependencies or platform). This "configuration-trace" is also a handy way to detect what changed upstream in minor MariaDB releases, which aren't supposed to have much changes. The update of the static text output files for MariaDB 11.4.4 were done in https://salsa.debian.org/mariadb-team/mariadb-server/-/commit/e2851fcfca9e8… and they show among others a new variable innodb-log-file-mmap ## Problem The issues I am running into is that this new innodb-log-file-mmap only seems to exist on amd64 and arm64? I discovered this in the Debian CI system (autpkgtests) at https://ci.debian.net/packages/m/mariadb/ which is passing for amd64/arm64/ppc64el, but failing on other platforms: https://ci.debian.net/packages/m/mariadb/unstable/armel/55869348/ https://ci.debian.net/packages/m/mariadb/unstable/armhf/55869349/ https://ci.debian.net/packages/m/mariadb/unstable/i386/55869350/ https://ci.debian.net/packages/m/mariadb/unstable/loong64/55869351/ https://ci.debian.net/packages/m/mariadb/unstable/s390x/55869354/ (The output also shows that mariadbd --verbose --help prints out wsrep-provider twice, once wsrep-provider=ON and once wsrep-provider=none, but that is a different issue.) On systems where it isn't available, shouldn't the option still show up (but as permanently disabled)? It seems odd to have system variables appear or disappear depending on the Linux architecture.

2 8

Re: b16319bdd0f: MDEV-35469 Heap tables are calling mallocs to often
by Sergei Golubchik 30 Dec '24

30 Dec '24

Hi, Monty, > commit b16319bdd0f > Author: Michael Widenius <monty(a)mariadb.org> > AuthorDate: Thu Nov 21 12:28:57 2024 +0200 > Commit: Michael Widenius <monty(a)mariadb.org> > CommitDate: Fri Dec 27 13:53:37 2024 +0200 > > diff --git a/storage/heap/hp_block.c b/storage/heap/hp_block.c > index 324efc8b4af..44bca363746 100644 > --- a/storage/heap/hp_block.c > +++ b/storage/heap/hp_block.c > @@ -75,9 +75,11 @@ int hp_get_new_block(HP_SHARE *info, HP_BLOCK *block, size_t *alloc_length) > Next time we allocate data for X rows. > When level 1 is full, we allocate data for HP_PTRS_IN_NOD at level 2 and 1 > + X rows at level 0. > */ > *alloc_length= (sizeof(HP_PTRS) * ((i == block->levels) ? i : i - 1) + > (ulonglong)block->records_in_block * block->recbuffer); > + /* Alloc in blocks of powers of 2 */ > + *alloc_length= MY_MAX(*alloc_length, block->alloc_size); What will happen to block->alloc_size - *alloc_length bytes? they'll be unused? > if (!(root=(HP_PTRS*) my_malloc(hp_key_memory_HP_PTRS, *alloc_length, > MYF(MY_WME | > (info->internal ? > diff --git a/storage/heap/hp_create.c b/storage/heap/hp_create.c > index c07a1e968c4..29812f08107 100644 > --- a/storage/heap/hp_create.c > +++ b/storage/heap/hp_create.c > @@ -15,11 +15,23 @@ > Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1335 USA */ > > #include "heapdef.h" > +#include <my_bit.h> > > static int keys_compare(void *heap_rb, const void *key1, const void *key2); > static void init_block(HP_BLOCK *block,uint reclength,ulong min_records, > ulong max_records); > > + > +/* > + In how many parts are we going to do allocations of memory and indexes > + If we assigne 1M to the heap table memory, we will allocate roughly > + (1M/16) bytes per allocaiton > +*/ > +int heap_allocation_parts= 16; static. technically, it could be `static const`, But I presume the compiler can figure out the `const` part if the variable is static. And e.g. use >> 4. Otherwise it has to assume it can be modified in some other file. > + > +/* min block allocation */ > +ulong heap_min_allocation_block= 16384; same. static. > + > /* Create a heap table */ > > int heap_create(const char *name, HP_CREATE_INFO *create_info, > @@ -170,7 +182,8 @@ int heap_create(const char *name, HP_CREATE_INFO *create_info, > share->keydef= (HP_KEYDEF*) (share + 1); > share->key_stat_version= 1; > keyseg= (HA_KEYSEG*) (share->keydef + keys); > - init_block(&share->block, visible_offset + 1, min_records, max_records); > + init_block(&share->block, hp_memory_needed_per_row(visible_offset + 1), visible_offset is already MY_MAX(reclength, sizeof (char*)). perhaps you don't need visible_offset and should use hp_memory_needed_per_row(reclength) here? > + min_records, max_records); > /* Fix keys */ > memcpy(share->keydef, keydef, (size_t) (sizeof(keydef[0]) * keys)); > for (i= 0, keyinfo= share->keydef; i < keys; i++, keyinfo++) > @@ -266,44 +279,90 @@ static int keys_compare(void *heap_rb_, const void *key1_, > heap_rb->search_flag, not_used); > } > > + > +/* > + Calculate length needed for storing one row > +*/ > + > +size_t hp_memory_needed_per_row(size_t reclength) > +{ > + /* Data needed for storing record + pointer to records */ > + reclength= MY_MAX(reclength, sizeof(char*) + 1); > + reclength= MY_ALIGN(reclength, sizeof(char*)); > + return reclength; > +} > + > +/* > + Calculate the number of rows that fits into a given memory size > +*/ > + > +ha_rows hp_rows_in_memory(size_t reclength, size_t index_size, > + size_t memory_limit) > +{ > + /* We need one extra byte for the delete flag */ > + reclength= hp_memory_needed_per_row(reclength+1); you have +1 for the delete flag in hp_memory_needed_per_row() > + if ((memory_limit - sizeof(HP_PTRS)) < (index_size + reclength)) > + return 0; /* Wrong arguments */ Why do you need this if()? If its condition is true the expression below will return 0 naturally, you don't need a separate check for that. > + return (ha_rows) ((memory_limit - sizeof(HP_PTRS)) / > + (index_size + reclength)); > +} > + > + > static void init_block(HP_BLOCK *block, uint reclength, ulong min_records, > ulong max_records) > { > diff --git a/storage/heap/hp_write.c b/storage/heap/hp_write.c > index 5469784c8c1..cb079eac757 100644 > --- a/storage/heap/hp_write.c > +++ b/storage/heap/hp_write.c > @@ -145,6 +145,8 @@ static uchar *next_free_record_pos(HP_SHARE *info) > DBUG_PRINT("exit",("Used old position: %p", pos)); > DBUG_RETURN(pos); > } > + if (!(block_pos=(info->records % info->block.records_in_block))) > + { why? > if ((info->records > info->max_records && info->max_records) || > (info->data_length + info->index_length >= info->max_table_size)) > { Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 2

Re: 448e82f9f0d: MDEV-35720 Add query_time to statistics
by Sergei Golubchik 30 Dec '24

30 Dec '24

Hi, Monty, Only a couple of comments about the test case: > commit 448e82f9f0d > Author: Michael Widenius <monty(a)mariadb.org> > Date: Fri Dec 27 19:26:55 2024 +0200 > > diff --git a/mysql-test/main/log_state.test b/mysql-test/main/log_state.test > --- a/mysql-test/main/log_state.test > +++ b/mysql-test/main/log_state.test > @@ -321,6 +321,48 @@ SET GLOBAL general_log_file = @old_general_log_file; > > --enable_ps_protocol > > +--echo # > +--echo # MDEV-35720 Add query_time to statistics > +--echo # > + > +connect (con2,localhost,root,,); > +let $s1=`show status like "query_time"`; > +select sleep(3) into @a; > +let $s2=`show status like "query_time"`; > +let $s3=`show global status like "query_time"`; > +--disable_query_log > +--eval SET @s1= SUBSTRING_INDEX('$s1', '\t', -1); > +--eval SET @s2= SUBSTRING_INDEX('$s2', '\t', -1); > +--eval SET @s3= SUBSTRING_INDEX('$s3', '\t', -1); Eh, that's not how one is supposed to do it. I mean, it works, but the easier approach is let $s1=query_get_value(show status like "query_time", Value); or purely in SQL set @s1=(select variable_value from information_schema.global_status where variable_name='query_time'); > +select @s1 >= 0.00 and @s1 <= 2.00 as "should be true"; > +--eval select @s2 >= 2.00 and @s2 < 10.00 as "should be true"; > +--eval select @s3 >= 3.00 as "should be true"; 1. you don't need eval here 2. I expect this test, like almost all other timing tests will be horribly unstable in buildbot and will be eventually disabled. No, I don't have a recipe for a stable timing test. > +--enable_query_log > + > +# select @s1,@s2,@s3; > +disconnect con2; > +connection default; > + > +delimiter |; > +create procedure p1() > +begin > +select sleep(1) into @a; > +select sleep(2) into @a; > +end| > +delimiter ;| > + > +connect (con2,localhost,root,,); > +call p1(); > +let $s1=`show status like "query_time"`; > +--disable_query_log > +--eval SET @s1= SUBSTRING_INDEX('$s1', '\t', -1); > +--eval select @s1 > 2.00 and @s1 < 10.00 as "should be true"; > +--enable_query_log > + > +# select @s1; > +disconnect con2; > +connection default; > + > # > # Cleanup > # Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 1

Re: dfc1632da73: Change default MEM_ROOT allocations to decrease calls to malloc
by Sergei Golubchik 27 Dec '24

27 Dec '24

Hi, Monty, Please, cherry-pick it into 10.11 and push as bb-10.11-merge. There will be *a lot* of conflicts in memroot code because of your "Added support to MEM_ROOT for write protected memory" change, it'd be best if you resolve your conflicts yourself. On Dec 22, Michael Widenius wrote: > revision-id: dfc1632da73 (mariadb-10.6.20-60-gdfc1632da73) > parent(s): 3297680579f > author: Michael Widenius > committer: Michael Widenius > timestamp: 2024-12-17 10:45:25 +0200 > message: > > Change default MEM_ROOT allocations to decrease calls to malloc ... > --- a/mysys/my_alloc.c > +++ b/mysys/my_alloc.c > @@ -173,7 +180,7 @@ void *alloc_root(MEM_ROOT *mem_root, size_t length) > DBUG_ENTER("alloc_root"); > - DBUG_PRINT("enter",("root: %p", mem_root)); > + DBUG_PRINT("enter",("root: %p length: %ld", mem_root, (long) length)); FYI, my_vsnprintf() supports standard "%zu" to print size_t values. Not important here, of course. > DBUG_ASSERT(alloc_root_inited(mem_root)); Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 1

Re: 2ba3bbb2463: Updated misc_session_status.test to not fail if select does not fail
by Sergei Golubchik 27 Dec '24

27 Dec '24

Hi, Monty, On Dec 22, Michael Widenius wrote: > revision-id: 2ba3bbb2463 (mariadb-10.6.20-61-g2ba3bbb2463) > parent(s): dfc1632da73 > author: Michael Widenius > committer: Michael Widenius > timestamp: 2024-12-17 10:45:25 +0200 > message: > > Updated misc_session_status.test to not fail if select does not fail > > The test with memory restrictions randomly works or fails depending on > server configurations. > As the test was there to just check if the server crashes when run with > small memory configurations, I disabled testing if the query would fail > or not. The test still has its original purpose. This is very questionable. The test would still serve its original purpose indeed if it'll hit OOM at least somewhere. But with your change it's not clear that it'll ever do. The query might as well succeed everywhere and we'll never know. Or it might start succeeding in the future. That is, technically you are right, the test still does its job (presuming you've verified that it hits OOM at least on some builders). But your change basically guarantees that it'll become meaningless in the future. Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 1

Re: 6c5c1d0bf00: MDEV-35469 Heap tables are calling mallocs to often
by Sergei Golubchik 23 Dec '24

23 Dec '24

Hi, Monty, On Dec 21, Michael Widenius wrote: > revision-id: 6c5c1d0bf00 (mariadb-10.6.20-57-g6c5c1d0bf00) > parent(s): 671f80c7388 > author: Michael Widenius > committer: Michael Widenius > timestamp: 2024-12-17 10:45:25 +0200 > message: > > MDEV-35469 Heap tables are calling mallocs to often > > Heap tables are allocated blocks to store rows according to > my_default_record_cache (mapped to the server global variable > read_buffer_size). > This causes performance issues when the record length is big > (> 1000 bytes) and the my_default_record_cache is small. > > Changed to instead split the default heap allocation to 1/16 of the > allowed space and not use my_default_record_cache anymore when creating > the heap. The allocation is also aligned to be just under a power of 2. > > For some test that I have been running, which was using record length=633, > the speed of the query doubled thanks to this change. > > Other things: > - Fixed calculation of max_records passed to hp_create() to take > into account padding between records. > > diff --git a/storage/heap/hp_create.c b/storage/heap/hp_create.c > index c07a1e968c4..fcfcf607822 100644 > --- a/storage/heap/hp_create.c > +++ b/storage/heap/hp_create.c ... > + of the block. > + */ > + records_in_block= > + (my_round_up_to_next_power((uint32) > + (records_in_block * recbuffer + > + sizeof(HP_PTRS) + MALLOC_OVERHEAD)/2) / > + recbuffer); Strictly speaking, it should be (my_round_up_to_next_power(records_in_block * recbuffer + extra) - extra ) / recbuffer And why /2 ? Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 1

Re: 07364d701b3: Added status variable "Max_memory_used" to SHOW STATUS
by Sergei Golubchik 22 Dec '24

22 Dec '24

Hi, Michael, On Dec 22, Michael Widenius wrote: > revision-id: 07364d701b3 (mariadb-10.6.20-64-g07364d701b3) > parent(s): 13fed908291 > author: Michael Widenius > committer: Michael Widenius > timestamp: 2024-12-17 10:45:26 +0200 > message: > > Added status variable "Max_memory_used" to SHOW STATUS > > This shows the maximum memory allocations used by the current connection. > The value for @@global.max_memory_used is 0 as we are not collecting this > value as it would cause a notable performance issue registering this for > all threads for every memory allocation Showing 0 is very confusing. I suggest not to print this variable in SHOW GLOBAL STATUS at all (probably possible, by returning a zero-length SHOW_ARRAY). Or show it as NULL. Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: 13fed908291: Added more information to errors reported by report_reply_packet()
by Sergei Golubchik 22 Dec '24

22 Dec '24

Hi, Monty, On Dec 22, Michael Widenius wrote: > revision-id: 13fed908291 (mariadb-10.6.20-63-g13fed908291) > parent(s): ca5ac5ac490 > author: Michael Widenius > committer: Michael Widenius > timestamp: 2024-12-17 10:45:26 +0200 > message: > > Added more information to errors reported by report_reply_packet() > > In particular ""Read semi-sync reply magic number error" now prints out > what was wrong with the packet. > > @@ -622,7 +626,8 @@ int Repl_semi_sync_master::report_reply_packet(uint32 server_id, > log_file_len = packet_len - REPLY_BINLOG_NAME_OFFSET; > if (unlikely(log_file_len >= FN_REFLEN)) > { > - sql_print_error("Read semi-sync reply binlog file length too large"); > + sql_print_error("Read semi-sync reply binlog file length too large: %llu", > + (longlong) log_file_pos); You used unsigned "%llu" but cast to a signed longlong. Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Can find sponsor: Would anyone be interested in patching mha to be MariaDB gtid compatible?
by highclass99 22 Dec '24

22 Dec '24

Hello, Would anyone be interested in patching mha to be MariaDB gtid compatible? I think I would be able to find a sponsor to pay a bit for it. reference links) https://github.com/yoshinorim/mha4mysql-manager https://github.com/yoshinorim/mha4mysql-node https://github.com/charlesdirk/MariaDB-MHA -> could not get this to work: if anyone got it to work please let me know how to get it to work. Thank you. Have a nice day.

3 3

Re: 3297680579f: Remove end newlines from CHECK TABLE output.
by Sergei Golubchik 21 Dec '24

21 Dec '24

Hi, Michael, On Dec 21, Michael Widenius wrote: > revision-id: 3297680579f (mariadb-10.6.20-59-g3297680579f) > parent(s): 4038ec2cc2c > author: Michael Widenius > committer: Michael Widenius > timestamp: 2024-12-17 10:45:25 +0200 > message: > > Remove end newlines from CHECK TABLE output. > > InnoDB has in some cases (not always) an newline at the end of error > messages. This messes up the CHECK TABLE output. > Fixed CHECK TABLE to remove the end newlines in its result set. I'd rather fix InnoDB to not do that, instead of working around InnoDB bug that garbles the CHECK TABLE output Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: e0afb5f08b5: cleanup: further replace thd->alloc() with operator new
by Sergei Golubchik 10 Dec '24

10 Dec '24

Hi, Nikita, Despite the commit has in the title, comments below apply to four commits: $ git log --oneline 7d5165894282^..0f6fd23e1604 0f6fd23e160 cleanup: add new(thd) to Sql_alloc and Item e0afb5f08b5 cleanup: further replace thd->alloc() with operator new 8daea2e14b2 cleanup: add operator new(size_t, const THD*) for single-object allocations 7d516589428 cleanup: add operator new[](size_t, const THD*) On Dec 06, Nikita Malyavin wrote: > diff --git a/sql/sql_class.h b/sql/sql_class.h > index aff02df69ec..299c8298606 100644 > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -1279,11 +1279,5 @@ class Query_arena > { return state == STMT_CONVENTIONAL_EXECUTION; } > > template <typename T=char> > - inline T* alloc(size_t size) const > - { > - return (T*)alloc_root(mem_root, sizeof(T)*size); > - } > - > - template <typename T=char> > inline T* calloc(size_t size) const Hmm, you didn't remove thd->calloc<>() I understand why, but that kind of makes the API inconsistent, perhaps it would've been cleaner to keep thd->alloc<>() and only implement new (thd) and new (arena) for Sql_alloc? May be not only for Sql_alloc. There are 1. pod types. they can be alloc'ed an calloc'ed, I think thd->alloc/thd->calloc is fine for them 2. non-pod types, can be allocated on the heap and on a mem_root. your new (thd)/delete (thd) operators are best for them. 3. non-pod types that can only be allocated on a mem_root. Sql_alloc can be used for them, if it'll disallow non-placement new. I feel like we don't need to make a distinction between 2 and 3, in that case Sql_alloc can simply go away. > { > diff --git a/plugin/feedback/feedback.cc b/plugin/feedback/feedback.cc > index bc8a98c11ae..dcc2e4417e5 100644 > --- a/plugin/feedback/feedback.cc > +++ b/plugin/feedback/feedback.cc > @@ -103,22 +103,22 @@ static COND* make_cond(THD *thd, TABLE_LIST *tables, LEX_STRING *filter) > nrc->resolve_in_table_list_only(tables); > nrc->select_lex= tables->select_lex; > > - res= new (thd) Item_cond_or(thd); > + res= new (thd) Item_cond_or(thd); > if (!res) > return OOM; > > for (; filter->str; filter++) > { > - Item_field *fld= new (thd) Item_field(thd, nrc, db, table, > + Item_field *fld= new (thd) Item_field(thd, nrc, db, table, > field); > - Item_string *pattern= new (thd) Item_string(thd, filter->str, > + Item_string *pattern= new (thd) Item_string(thd, filter->str, > (uint) filter->length, cs); you might want to go through the diff and fix the indentation > - Item_string *escape= new (thd) Item_string(thd, "\\", 1, cs); > + Item_string *escape= new (thd) Item_string(thd, "\\", 1, cs); > > if (!fld || !pattern || !escape) > return OOM; > > - Item_func_like *like= new (thd) Item_func_like(thd, fld, pattern, > + Item_func_like *like= new (thd) Item_func_like(thd, fld, pattern, > escape, 0); > > if (!like) > diff --git a/sql/item.h b/sql/item.h > index 631fd1b19f8..b0cf03c1435 100644 > --- a/sql/item.h > +++ b/sql/item.h > @@ -857,8 +857,9 @@ class Item :public Value_source, > static void *operator new(size_t size); > > public: > - static void *operator new(size_t size, MEM_ROOT *mem_root) throw () > + static void *operator new(size_t size, MEM_ROOT *mem_root) noexcept > { return alloc_root(mem_root, size); } > + static void *operator new(size_t size, const THD *thd) noexcept; > static void operator delete(void *ptr,size_t size) { TRASH_FREE(ptr, size); } > static void operator delete(void *ptr, MEM_ROOT *mem_root) {} Better inherit it from Sql_alloc > diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc > index d86fdf5c5d6..e0adc5c3bb4 100644 > --- a/sql/item_cmpfunc.cc > +++ b/sql/item_cmpfunc.cc > @@ -2007,7 +2007,7 @@ bool Item_func_interval::fix_length_and_dec(THD *thd) > > if (not_null_consts) > { > - intervals= thd->alloc<interval_range>(rows - 1); > + intervals= new(current_thd) interval_range[rows - 1]; current_thd ? > if (!intervals) > return true; > > diff --git a/sql/item_subselect.cc b/sql/item_subselect.cc > index 008384a6590..21364286260 100644 > --- a/sql/item_subselect.cc > +++ b/sql/item_subselect.cc > @@ -6086,8 +6086,8 @@ bool Ordered_key::init(int col_idx) > > // TIMOUR: check for mem allocation err, revert to scan > > - key_columns= thd->alloc<Item_field*>(1); > - compare_pred= thd->alloc<Item_func_lt*>(1); > + key_columns= new(thd) Item_field*();; > + compare_pred= new(thd) Item_func_lt*();; ;; > > key_columns[0]= new (thd) Item_field(thd, tbl->field[col_idx]); > /* Create the predicate (tmp_column[i] < outer_ref[i]). */ > diff --git a/sql/opt_subselect.h b/sql/opt_subselect.h > index 8b4dd1f0092..37902c6eebe 100644 > --- a/sql/opt_subselect.h > +++ b/sql/opt_subselect.h > @@ -357,7 +357,7 @@ uint get_number_of_tables_at_top_level(JOIN *join); > the tuple. > */ > > -class SJ_TMP_TABLE : public Sql_alloc > +class SJ_TMP_TABLE why? > { > public: > /* > diff --git a/sql/sql_alloc.h b/sql/sql_alloc.h > index f5d2d4e8b1a..e643a9f817d 100644 > --- a/sql/sql_alloc.h > +++ b/sql/sql_alloc.h > @@ -23,17 +23,21 @@ > class Sql_alloc > { > public: > - static void *operator new(size_t size) throw () > + static void *operator new(size_t size) noexcept > { > return thd_alloc(_current_thd(), size); > } can this be disallowed completely? > - static void *operator new[](size_t size) throw () > + static void *operator new[](size_t size) noexcept > { > return thd_alloc(_current_thd(), size); > } > - static void *operator new[](size_t size, MEM_ROOT *mem_root) throw () > + > + static void *operator new[](size_t size, const THD *thd) noexcept; > + static void *operator new(size_t size, const THD *thd) noexcept; > + > + static void *operator new[](size_t size, MEM_ROOT *mem_root) noexcept > { return alloc_root(mem_root, size); } > - static void *operator new(size_t size, MEM_ROOT *mem_root) throw() > + static void *operator new(size_t size, MEM_ROOT *mem_root) noexcept > { return alloc_root(mem_root, size); } > static void operator delete(void *ptr, size_t size) { TRASH_FREE(ptr, size); } > static void operator delete(void *, MEM_ROOT *){} > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index 7847389dabc..37869da63ae 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -8831,3 +8831,46 @@ LEX_CSTRING make_string(THD *thd, const char *start_ptr, > size_t length= end_ptr - start_ptr; > return {strmake_root(thd->mem_root, start_ptr, length), length}; > } > +void* operator new(size_t size, const Query_arena *thd) noexcept > +{ > + return alloc_root(thd->mem_root, size); > +} > + > +void operator delete[](void *ptr, const Query_arena *thd) noexcept > +{} > + > +void operator delete(void *ptr, const Query_arena *thd) noexcept > +{} if you put them into sql_class.h - will they be inlined? Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 3

MDEV-34705: Storing binlog in InnoDB
by Kristian Nielsen 06 Dec '24

06 Dec '24

I am in the early stages of a project to hopefully implement a new binlog format that is managed transactionally by InnoDB. This is a complex change, and I want to solicit comments early, so I encourage feedback. The TLDR; of this: - The current binlog format stores events separately from InnoDB. This requires a complex and _very_ constly 2-phase commit protocol between the binlog and InnoDB for every commit. - The binlog format is rather naive, and not well suited as a high-performance transactional log. It exposes internal implementation detail to the slaves, and its flat-file nature makes it inflexible to read (for example finding the last event sequential start from the start). Replacing it with something new has the potential to reap many benefits. - This project proposes to extend the storage engine API to allow an engine to implement binlog storage. It will be implemented in InnoDB so that binlog events are stored in InnoDB tablespaces, and the standard InnoDB transactional and recovery mechanisms are used to provide crash safety and optionally durability. - This mechanism will allow full crash-recovery and consistency without any fsync per commit (--innodb-flush-log-at-trx-commit=0), providing huge improvement in throughput for applications that do not require strict durability requirements. And it will reduce fsync requirement to one per commit for durable config, with greatly improved opportunities for group commit. Below a long and somewhat raw write-up of my current state of design. There was some interest expressed in learning more about this work, an interest that I'm very pleased with ;-). The current state of my prototype implementation is available in the branch knielsen_binlog_in_engine: https://github.com/MariaDB/server/commits/knielsen_binlog_in_engine Architectural considerations. The fundamental problem with the binlog is that it functions as a transactional log, but it is separate from the InnoDB transactional log. This is very unoptimal. To ensure consistency in case of crash, a two-phase commit protocol is needed that requires two fsync per commit, which is very costly in terms of performance. And it causes a lot of complexity in the code as well. We should make it so that there is only a single transactional log ("single source of truth") used, at least for the case where only a single storage engine is used (eg. InnoDB) for all tables. Some different options for doing this can be considered (explained in detail below): 1. Implement a new binlog format that is stored in a new InnoDB tablespace type (or more generally in any storage engine that implements the appropriate new API for binlog). 2. Keep the current binlog format, extend the InnoDB redo log format with write-ahead records for binlog writes. Integrate binlog writes with InnoDB checkpointing and buffer pool flush. 3. Store the binlog events directly inside the InnoDB redo log. Implement log archiving of the InnoDB log to preserve old binlog data as long as desired. 4. (Mentioned only for completeness): Implement a general redo log facility on the server level, and change both InnoDB and binlog to use that facility. I am currently working on option (1), inspired by talks I had with Marko Mäkelä at the MariaDB fest last Autumn. He suggested that a new type of tablespace be added to InnoDB which would hold the binlog, and explained that this would require relative small changes in the InnoDB code. I think this is the best option. The InnoDB log and tablespace implementation is very mature, it is very good to be able to re-use it for the binlog. The integration into InnoDB looks to be very clean, and all the existing infrastructure for log write, recovery, checkpointing, buffer pool can be re-used directly. All the low-level detail of how the binlog is stored on disk can be handled efficiently inside the InnoDB code. This option will require a lot of changes in the binlog and replication code, which is the main cost of choosing this option. I have a _lot_ of experience with the MariaDB replication code, and I think this is feasible. And I think there are another set of huge benefits possible from implementing a new binlog format. The existing binlog format is very naive and neither very flexible, nor well suited as a high-performance transactional log. So changing the binlog format is something that should be done eventually anyway. Option (2) would also be feasible I think, and has the advantage of needing less changes to the replication code. Monty mentioned seeing a patch that sounded something like this. The main disadvantage is that I think this will require complex interaction between low-level InnoDB checkpointing code and low-level binlog writing code, which I fear will eventually lead to complex code that will be hard to maintain and will stiffle further innovation on either side. It also doesn't address the other problems with the current binlog format. To me, letting the storage engine control how it stores data transactionally - including the binlog events - is the "right" solution, and worth persuing over a quicker hack that tries to avoid changing code. Option (3) is also interesting. The idea would be that the binlog becomes identical to the InnoDB redo log, storing the binlog data interleaved with the InnoDB log records. This is conceptually simpler than options (1) and (2), in the sense that we don't have to first write-ahead-log a record for modifying binlog data, and then later write the actual data modification to a separate tablespace/file. But the InnoDB log is cyclic, and old data is overwritten as the log LSN wraps over the end. Thus, this option (3) will require implementing log archiving in InnoDB, which I am not sure how much work will be required. Either the binlog data could be archived by copying out asynchroneously to persistent binlog files, which becomes somewhat similar to option (1) or (2) then perhaps. Or InnoDB could be changed to create a new log file at wrap-over, renaming the old one to archive it, though this will leave the binlog data somewhat "dilluted", mixed up with InnoDB internal redo records. One pragmatic reason I've chosen option (1) over this option (3) for now is that I am more comfortable with doing large changes to replication code than to InnoDB code. Also, option (3) would more drastically change the way users can work with the binlog files, as a large amount of the most recent data would be sitting inside the InnoDB redo log and not in dedicated binlog files, unlike options (1) and (2). Still, option (3) is an interesting idea and has the potential to reduce the write amplification that will occur from (1) and (2). So I'm eager to hear any suggestions around this. Option (4) I mention only for completeness. Somehow, the "clean" way to handle transactional logging would be if the server layer provides a general logging service, and all the components would then use this for logging. InnoDB and the binlog, but also all the other logs or log-like things we have, eg. Aria, DDL log, partitioning, etc. However, I don't think it makes sense to try to discard the whole InnoDB write-ahead logging code and replace it with some new implementation done on the server level. And I'm not even sure it makes sense from a theoretical point of view - somehow, the logging method used is intrinsically a property of the storage engine; it seems dubious if a general server-level log facility could be designed that would be optimal for both InnoDB and RocksDB, for example. High-level design. Introduce a new option --binlog-storage-engine=innodb. Extend the storage engine API with the option for an engine to provide a binlog implementation, supported by InnoDB initially. Initially the option cannot be changed dynamically. When --binlog-storage-engine=innodb, the binlog files will use a different format, stored as a new type of InnoDB tablespace. It should support that the old part of the binlog is the legacy format and the new is the InnoDB format, to facilitate migrations. Maybe also support going back again, though this is less critical initially. A goal is to clean up some of the unnecessary complexity of the legacy binlog format. The old format will be supported for the foreseeable future, so the new format can break backwards compatibility. For example, the name of binlog files will be fixed, binlog-<NNNNNN>.ibb, so each file can be identified solely by its number NNNNNN. The option to decide the directory in which to store the binlog files should be supported though. I think we can require the slaves to use GTID when --binlog-storage-engine=innodb is enabled on the master. This way we can avoid exposing slaves to internal implementation details of the binlog, and the slaves no longer need to update the file relay-log.info at every commit. We should also be able to get rid of the Format_description_log_event and the Rotate_log_event at the start of every binlog file, so that the slave can view the events from the master as one linear stream and not care about how the data is split into separate binlog files on the master. The binlog format will be page based. This will allow pre-allocating the binlog files and efficiently writing them page by page. And it will be possible to access the files without scanning them sequentially from the start; eg. we can find the last event by reading the last page of the file, binary-searching for the last used page if the binlog file is partially written. The GTID indexes can be stored inside the same tablespace as the binlog data (eg. at the end of the tablespace), avoiding the need for a separate index file. GTID index design is still mostly TBD, but I think it can be implemented so that indexes are updated transactionally as part of the InnoDB commit and no separate crash recovery of GTID indexes is needed. With GTID indexes being guaranteed available, we can use them to obtain the GTID state at the start of each binlog file, and avoid the need for the Gtid_list_log_event at the start of the binlog. With a page-based log with extensible format, metadata can be added to the binlog that is only used on the master without having to introduce new replication events that are not relevant to the replication on the slave. This can be used eg. to eliminate the Binlog_checkpoint_log_event, for example. Possibly the binlog checkpoint mechanism can be completely avoided for the --binlog-storage-engine=innodb case, since there is no more 2-phase commit needed in the common case of an InnoDB-only transaction. The existing binlog checksum and encryption will no longer be used, instead the standard InnoDB checksums and encryption will be reused. Implementation plan. The storage engine API must be extended to provide a facilities for writing to the binlog and for reading from the binlog. The design of the API is TBD, should be discussed to try to make it generic and suitable for other engines than InnoDB. When writing to binlog in InnoDB, the central idea is to use the same mini-transaction (mtr) that marks the transaction as committed in InnoDB. This is what makes the binlog data guaranteed consistent with the InnoDB table data without the need for two-phase commit. The initial version I think can use the existing binlog group commit framework; this will simplify the implementation. This will thus keep the LOCK_commit_ordered and the queue_for_group_commit() mechanisms. Later work can then be to see if this can be even further improved in terms of scalability. I want to implement that large event groups can be split into multiple chunks in the binlog that no longer need to be consecutive. The final chunk will be the one that contains the GTID and marks the event group binlogged; this final chunk will then refer back to the other parts. This way, a large transactions can be binlogged without stalling the writing of other (smaller) transactions in parallel, which is a bottleneck in the legacy binlog. And it avoids problems with exceeding the maximum size of an mtr. In the first version, I think the binlog reader in the dump thread will collect and concatenate the different chunks before sending them to the slave, so simplify the initial implementation. But a later change could allow the slave to receive the different chunks interleaved between different event groups. This can even eventually allow the slave to speculatively execute events even before the transaction has committed on the master, to potentially reduce slave lag to less than the time of even a single transaction; this would be a generalisation of the --binlog-alter-two-phase feature. The binlog will be stored in separate tablespace files, each of size --max-binlog-size. The binlog files will be pre-allocated by a background thread. Since event groups can spill over to the next file, each file can be a fixed size, and hopefully also the rotate events will no longer be necessary. A busy server will quickly cycle through tablespace files, and we want to avoid "using up" a new tablespace ID for each file (InnoDB tablespace IDs are limited to 2**32 and are not reused). Two system tablespace IDs will be reserved for the binlog, and new binlog files will alternate between them. This way, the currently written binlog can be active while the previous one is being flushed to disk and the remaining writes checkpointed. Once the previous log has been flushed completely, its tablespace ID can be re-used for the next, pre-allocated binlog file and be ready for becoming active. This way, the switching to the next binlog file should be able to occur seamlessly, without any stalls, as long as page flushing can keep up. The flushing of binlog pages will be prioritised, to avoid stalling binlog writes, to free up buffer pool pages that can be used more efficiently than holding binlog data, and to quickly make the binlog files readable from outside the server (eg. with mysqlbinlog). The binlog dump thread that reads from the binlog and sends the data to slaves will use a binlog reading API implemented in InnoDB. I will prefer to read directly from the binlog files, in order to reduce pressure on the InnoDB buffer pool etc. A slave that connects from an old position back in time may need to read a lot of old data from the binlogs; there is little value in loading this data into the buffer pool, evicting other more useful pages. The reader can lookup in the InnoDB buffer pool with the BUF_GET_IF_IN_POOL flag. This way, the data can be accessed from the buffer pool if it is present. If not present, we can be sure that the data will be in the data file, and can read it from the file directly. If the data is known to be already flushed to disk before the specific binlog position, then the buffer pool lookup can be skipped altogether. The mysqlbinlog program will need to be extended somehow to be able to read from an InnoDB tablespace (or in general other storage engine). I think this means mysqlbinlog needs some kind of engine plugin facility for reading binlog. Note that the -read-from-remote-server option will also be available to read data from a mysqlbinlog that doesn't the new format, or to read the very newest data before it gets written to disk. Final words. This is still an early draft of the feature, as it is being worked on and refined; things are subject to change. And this also means that things are *open* to change, according to any suggestions with a good rationale.

2 7

Review of MDEV-6017 Add support for Indexes on Expressions
by Michael Widenius 03 Dec '24

03 Dec '24

Hi! Here is the review for MDEV-6017 Add support for Indexes on Expressions commit c1e7574c799163f5d8a26ca8e0f91af6c91f7077 Instead of adding bool function virtual bool vcol_subst_analyzer(uchar **) { return false; } Why not add Item_base_t flag: IS_SIMPLE_BOOL_FUNC This takes one bit instead of increasing ALL created item base structures with 8 bytes. As we have 100+ items, adding a virtual function increases code space with 1K or more + the functions. You can also replace vcol_subst_analyzer() with a top level function: bool Item::vcol_subst_analyzer/( { return (type() == Item::FUNC_ITEM && (((Item_func*) this)->bitmap_bit() & (BITMAP_EQ | BITMAP_EQUAL | BITMAP_LT | BITMAP_GT | BITMAP_LE | BITMAP_GE | BITMAP_BETWEEN | BITMAP_IN | BITMAP_ISNULL | BITMAP_ISNOTNULL ) } For this to work, you would have to add the last two bitmaps to enum Bitmap in item_func.h @@ -3272,6 +3292,7 @@ class Item_cond :public Item_bool_func bool excl_dep_on_table(table_map tab_map) override; bool excl_dep_on_grouping_fields(st_select_lex *sel) override; + bool vcol_subst_analyzer(uchar **) override { return true; } I assume you need the above to be able to traverse AND and OR expressions for WHERE. Please add a comment that this is the intent. If you decide to use the bitmaps, add an explanation why AND and OR is in the vcol_subst_analyzer list. ---- +void collect_indexed_vcols_for_table(TABLE *tbl, List<Field> *vcol_fields) +{ Please use table instead of tbl. We use table as a varible much more than tbl: ((/my/maria-10.5/sql)) grep tbl *.* | wc 1143 5371 73297 ((/my/maria-10.5/sql)) grep table *.* | wc 40886 236754 2832202 + for (uint i=0; i < tbl->s->keys; i++) + { + // note: we could also support histograms here + if (!tbl->keys_in_use_for_query.is_set(i)) + continue; I wonder if it would be a good idea to extend bitmap to have an iterator like we have for tables where we only loop over the set bits. From sql_bitmap.h: class Table_map_iterator { ulonglong bmp; public: Table_map_iterator(ulonglong t): bmp(t){} uint next_bit() { if (!bmp) return BITMAP_END; uint bit= my_find_first_bit(bmp); bmp &= ~(1ULL << bit); return bit; } int operator++(int) { return next_bit(); } enum { BITMAP_END= 64 }; }; Would be quite trival to do also for keys. This would help tables with many keys and also would allows us to remove the 'call' to if (!tbl->keys_in_use_for_query.is_set(i)) We could also do an optimized version where we implement bitmap_get_next_set() where we do not have to reset the previous bits for each call. --------- + if (field->vcol_info) + vcol_fields->push_back(field); You have to take care of error handling here. Please change the functions collect_indexed_vcols_for_table(), collect_indexed_vcols_for_join() and other functions that calls push_back() to bool and return 1 in case of error. + item->top_level_compile(ctx->thd, + &Item::vcol_subst_analyzer, &yes, + &Item::vcol_subst_transformer, (uchar*)ctx); You also have to check the return of the above for errors. Another option is to just check thd->fatal_error() as this will be set in case of OOM. .... + { + if (auto nested_join= table->nested_join) + subst_vcols_in_join_list(ctx, &nested_join->join_list); PLEASE do not use auto. I want to know the type while reading and debugging the code! auto can also introduce bugs if a type changes. + @todo: this cannot handle VIEW columns currently. View columns are wrapped + in Item_direct_view_ref objects which do not comsider themselves equal to + Item_field objects they're wrapping. Please add this also to the commit comment. + /* + Do not do the substitution if the datatypes do not match. + Virtual column's datatype can be different from the expression, for + example: + + col3 INT AS (CONCAT(col1, col2)) + + Do not allow substitutions that change the semantics of comparison. + At the moment, we require that datatypes are the same. + This probably could be relaxed. + + For strings: we allow two cases: + - vcol_expr and vcol_field have the same collation. + - vcol_field has the same collation as the comparison collation. + + (Note: MySQL calls resolve_type() after it has done the substitution. + This can potentially update the comparator. The idea is that this + shouldn't be necessary as we do not want to change the comparator. + Changing the comparator will change the semantics of the condition, + our point is that this must not happen) + */ Move to a function comment. It is hard to read read the function code with this long comment in the middle. + if (vcol_expr->type_handler_for_comparison() != + vcol_field->type_handler_for_comparison() || + vcol_expr->maybe_null() != vcol_field->maybe_null()) + fail_cause="type mismatch"; Why cannot we handle this case if vcol_expr->maybe_null is 0 and vcol_field->maybe_null() is set? In this case we know that the vcol_field's we are going to encounter can never be null, which should satisfy our requirements. + Item_field *itf= new (thd->mem_root) Item_field(thd, vcol_field); + if (!itf) + return false; Better to return true in case of errors, like we do in most of the other code. Remove and a blank line instead + //Item *const_expr; ... remove + //const_expr= args[0]; ----- +Item* Item_func_in::vcol_subst_transformer(THD *thd, uchar *arg) +{ + Vcol_subst_context *ctx= (Vcol_subst_context*)arg; + if (!compatible_types_scalar_bisection_possible()) Add comment /* Check that all arguments inside IN() are constants */ Regards, Monty

1 0

Re: 5b6b23563bf: MDEV-34979 generate SBOM from server builds
by Sergei Golubchik 02 Dec '24

02 Dec '24

Hi, Vladislav, Thanks! See comments below On Dec 02, Vladislav Vaintroub wrote: > revision-id: 5b6b23563bf (mariadb-11.7.1-5-g5b6b23563bf) > parent(s): eff9c198e32 > author: Vladislav Vaintroub > committer: Vladislav Vaintroub > timestamp: 2024-11-25 19:04:18 +0100 > message: > > MDEV-34979 generate SBOM from server builds > > This commit adds the capability to generate a Software Bill of Materials > (SBOM) from server builds. > > It introduces a new WITH_SBOM variable, which defaults to ON for package > builds (i.e if BUILD_CONFIG is used) and to OFF otherwise. > > When enabled, the build process will produce an sbom.json document in > CycloneDX format, capturing information about various dependencies, > which is gathered from various sources. > > We use git submodule information and CMake external projects properties > to gather version information for 3rd party code, but also handle > dependencies if external code is part of our repository > (zlib, or Connect storage engine's minizip) > > The SBOM document is stored in the root build directory in sbom.json file, > but is not currently installed. > > diff --git a/CMakeLists.txt b/CMakeLists.txt > --- a/CMakeLists.txt > +++ b/CMakeLists.txt > @@ -581,6 +581,13 @@ ENDIF() > > INCLUDE(build_depends) > > +INCLUDE(CMakeDependentOption) > +CMAKE_DEPENDENT_OPTION(WITH_SBOM "Generate Software Bill of Materials (SBOM)" OFF "BUILD_CONFIG" ON) > +IF(WITH_SBOM) > + INCLUDE(generate_sbom) > + GENERATE_SBOM() > +ENDIF() This doesn't work for me, I mean, cmake -DBUILD_CONFIG=mysql_release . does not generate the file, but cmake -DBUILD_CONFIG=mysql_release . -DWITH_SBOM=ON does. May be it'd be more robust to use OPTION() and set it from cmake/build_configuration/mysql_release.cmake? Like it's done for other options. > + > INCLUDE(CPack) > > IF(WIN32 AND SIGNCODE) > diff --git a/cmake/generate_submodule_info.cmake b/cmake/generate_submodule_info.cmake > --- /dev/null > +++ b/cmake/generate_submodule_info.cmake > @@ -0,0 +1,73 @@ ... > + FOREACH(s ${out_list}) > + IF(${counter} EQUAL 0) > + IF (NOT("${s}" MATCHES "Entering '")) does it depend on the locale? > + MESSAGE(FATAL "Unexpected output ${outvar}") > + ENDIF() > + STRING(LENGTH "${s}" slen) > + MATH(EXPR substr_len "${slen} - 11") ... > diff --git a/cmake/generate_sbom.cmake b/cmake/generate_sbom.cmake > --- /dev/null > +++ b/cmake/generate_sbom.cmake > @@ -0,0 +1,240 @@ > +INCLUDE(generate_submodule_info) > +INCLUDE(ExternalProject) > + > +# Remove all matches from a cmake list > +FUNCTION (REMOVE_ALL_MATCHES list_var_name match_str) > + SET(new_list) > + FOREACH(el ${${list_var_name}}) > + IF(NOT("${el}" MATCHES "${match_str}")) > + LIST(APPEND new_list ${el}) > + ENDIF() > + ENDFOREACH() > + SET(${list_var_name} ${new_list} PARENT_SCOPE) > +ENDFUNCTION() you could've used LIST(FILTER ...) instead. (but not LIST(REMOVE_ITEM ...) which is 3.16+) > + > +# Extract user name and repository name from a github URL. > +FUNCTION (EXTRACT_REPO_NAME_AND_USER repo_url repo_name_var repo_user_var) > + IF(repo_url MATCHES "^git@") > + # normalize to https-style URLs > + STRING(REGEX REPLACE "^git@([^:]+):(.*)$" "https://\\1/\\2" repo_url "${repo_url}") I don't think we have that, is it even tested? :) note, I'm not saying it isn't needed, better to have it than not. > + ENDIF() > + # Extract the repository user > + STRING(REGEX REPLACE "https://([^/]+)/([^/]+)/.*" "\\2" repo_user "${repo_url}") > + > + STRING(REGEX REPLACE ".*/([^/]*)$" "\\1" repo_name "${repo_url}") > + STRING(REGEX REPLACE "\\.git$" "" repo_name "${repo_name}") > + > + SET(${repo_name_var} ${repo_name} PARENT_SCOPE) > + SET(${repo_user_var} ${repo_user} PARENT_SCOPE) > +ENDFUNCTION() > + > +# Add a known 3rd party dependency for SBOM generation > +# Currently used for "vendored" (part of our repository) source code we know about > +# such as zlib, as well ExternalProject_Add() projects > +MACRO(ADD_THIRD_PARTY_DEPENDENCY name url tag rev version description) > + LIST(FIND ALL_THIRD_PARTY ${name} idx) > + IF (idx GREATER -1) > + MESSAGE(FATAL_ERROR "${name} is already in ALL_THIRD_PARTY") > + ENDIF() > + SET(${name}_URL ${url}) > + SET(${name}_TAG ${tag}) > + SET(${name}_REVISION ${rev}) > + SET(${name}_DESCRIPTION "${description}") > + SET(${name}_VERSION "${version}") > + LIST(APPEND ALL_THIRD_PARTY ${name}) > +ENDMACRO() > + > +# Match third party component with supplier > +# CyclonDX documentation says it is > +# "The organization that supplied the component. > +# The supplier may often be the manufacturer, but may also be a distributor or repackager." > +# > +# Perhaps it can always be "MariaDB", but security team recommendation is different > +# more towards "author" > +FUNCTION (sbom_get_supplier repo_name repo_user varname) why a function? just curious > + IF("${repo_name_SUPPLIER}") > + SET(${varname} "${repo_name_SUPPLIER}" PARENT_SCOPE) ... > + # It is possible to provide EXTRA_SBOM_DEPENDENCIES > + # and accompanying per-dependency data, to extend generared sbom > + # document. > + # Example below injects an extra "ncurses" dependency using several > + # command line parameters for CMake. > + # -DEXTRA_SBOM_DEPENDENCIES=ncurses > + # -Dncurses_URL=https://github.com/mirror/ncurses > + # -Dncurses_TAG=v6.4 > + # -Dncurses_VERSION=6.4 > + # -Dncurses_DESCRIPTION="A fake extra dependency" > + SET(ALL_THIRD_PARTY ${ALL_SUBMODULES} ${EXTRA_SBOM_DEPENDENCIES}) okay. Though I personally wouldn't use ALL_SUBMODULES and EXTRA_SBOM_DEPENDENCIES and instead would scan GET_CMAKE_PROPERTY(VARS VARIABLES) for *_TAG or something > + > + IF(TARGET ha_connect OR TARGET connect) > + ADD_THIRD_PARTY_DEPENDENCY(minizip > + "https://github.com/zlib-ng/minizip-ng" > + "" 252588f "" > + "Vendored minizip, inside connect storage engine, storage/connect/zip.c et al") did you verify that our file matches the content of the file from that repo at that commit? also, a though - how can we prevent our minigzip.c from being modified without updaing the dependency? Suggestion, check the commit hash, like git log -n1 --format=%h storage/connect/zip.c > + ENDIF() > + > + IF(TARGET libfmt) > + ExternalProject_GET_PROPERTY(libfmt URL) > + STRING(REGEX MATCH "[0-9]+\\.[0-9]+\\.[0-9]+" libmt_TAG "${URL}") > + ADD_THIRD_PARTY_DEPENDENCY(libfmt > + "https://github.com/fmtlib/fmt" > + "${libmt_TAG}" "${libmt_TAG}" "${libmt_TAG}" > + "header only library, used in server") > + ENDIF() I wonder, wouldn't it be simpler and more robust to overwrite or wrap ExternalProject() to do the above automatically? (using the same name or a different one like MyExternalProject) ... Also, I noticed that the server entry gets "purl": "pkg:github//@5b6b23563bf" perhaps it should have a repo name here? Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: 534dda1234b: MDEV-35522: MariaDB Audit does not detect all DCLs forms when masking password
by Sergei Golubchik 02 Dec '24

02 Dec '24

Hi, Oleksandr, Ok to push. But do you no longer delete server_audit.log after the test? On Dec 02, Oleksandr Byelkin wrote: > revision-id: 534dda1234b (mariadb-10.5.27-64-g534dda1234b) > parent(s): fdb6db6b47f > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2024-11-29 11:45:01 +0100 > message: > > MDEV-35522: MariaDB Audit does not detect all DCLs forms when masking password > > 1. skip OR REPLACE > (to make it filter_query_type made recursive) > > 2. skip SET STATEMENT ... FOR before checking > statements with passwords > > diff --git a/mysql-test/suite/plugins/t/server_audit_pwd_mask.test b/mysql-test/suite/plugins/t/server_audit_pwd_mask.test > --- a/mysql-test/suite/plugins/t/server_audit_pwd_mask.test > +++ b/mysql-test/suite/plugins/t/server_audit_pwd_mask.test > --echo # cleaunup > +DROP SERVER s1; > DROP USER u1; > DROP USER u2; > set global server_audit_logging=off; > ---remove_file $SEARCH_FILE > +##--remove_file $SEARCH_FILE why? > --disable_warnings > UNINSTALL PLUGIN ed25519; > UNINSTALL PLUGIN server_audit; > --enable_warnings Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 1

Re: ff3d06f6601: MDEV-22217 Make OS character sets "utf8" and "utf-8" map to MariaDB character set "utf8mb4"
by Sergei Golubchik 16 Nov '24

16 Nov '24

Hi, Alexander, Of course, there can be no comments about the code change here. But the question is, is it something we want to do? 1. Is utf8/utf-8 locale indeed 4-byte utf8? 2. Why is hard-coding to utf8mb4 better than having it dependent on UTF8_IS_UTF8MB3 ? On Nov 16, Alexander Barkov wrote: > revision-id: ff3d06f6601 (mariadb-11.6.1-44-gff3d06f6601) > parent(s): 4b6922a315f > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2024-10-28 14:11:40 +0400 > message: > > MDEV-22217 Make OS character sets "utf8" and "utf-8" map to MariaDB character set "utf8mb4" > > diff --git a/mysql-test/main/mysql_locale_posix.test b/mysql-test/main/mysql_locale_posix.test > index c047b7b1ae9..b37d04baca2 100644 > --- a/mysql-test/main/mysql_locale_posix.test > +++ b/mysql-test/main/mysql_locale_posix.test > @@ -203,3 +203,20 @@ DROP DATABASE `Ã³`; > + > +--exec LC_ALL=en_US.utf8 $MYSQL --default-character-set=auto test -e "SELECT @@character_set_client" > +--exec LC_ALL=en_US.utf8 $MYSQL --default-character-set=auto test -e "SELECT @@character_set_connection" > +--exec LC_ALL=en_US.utf8 $MYSQL --default-character-set=auto test -e "SELECT @@character_set_results" may be do it in one SELECT? or in SHOW VARIABLES LIKE 'character_set%' ? to have it a tiny bit faster? Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

2024 Q4 release part 2 (11.6.2, 11.7,1)
by Daniel Black 12 Nov '24

12 Nov '24

All, The 11.6.2 and 11.7.1 releases are now built: * 11.6.2 d8dad8c3b54cd09fefce7bc3b9749f427eed9709 (GA) * 11.7.1 74331a48dd3dadfe5041f3440b81a7183e9fd4a8 release notes still work in progress: * https://mariadb.com/kb/en/mariadb-11-6-2-release-notes/ * 11.7.1 still being prepared but will be 11.6.2 like + the features of 11.7.0 release notes + Vector search, + bugs fixes since 11.7.0. Both are rolling releases and once GA, like 11.6.2, will be the last of the release series. containers for testing are available: * quay.io/mariadb-foundation/mariadb-devel:11.6 * quay.io/mariadb-foundation/mariadb-devel:11.7 Build packages for RPM/Deb distros repo/source files: https://ci.mariadb.org/ * 11.6.2 - bb-11.6-release-latest* * 11.7.1 - bb-11.7-release-latest* Sources: * https://ci.mariadb.org/51755/ (11.6.2) * https://ci.mariadb.org/51770/ (11.7.1) Now begins the process of loading packages to mirrors and Docker Official Images releases.

1 0

Contribute to MariaDB compatibility in open source projects?
by Robert Silén 06 Nov '24

06 Nov '24

All, We have been working on community outreach as described in an earlier blog Improving MariaDB support in open source projects <https://mariadb.org/improving-mariadb-support-in-open-source-projects/>. Many projects have made efforts to improve their MariaDB compatibility with or without our help, and also documented compatibility more visibly in READMEs and documentation. However, we at the Foundation don't have the knowhow or resources to help out everybody, so I thought I'd share a list of some issues. Would somebody like to contribute to any of them? Any comment on the issues, or code contributions to them would be much appreciated. Thank you. Do share any thoughts or actions on this. Robert Silén, community advocacy at MariaDB Foundation robert.silen(a)mariadb.org - https://github.com/SOCI/soci/issues/1150: add MariaDB to README - https://github.com/tinode/chat/pull/924: Help set up CI for MariaDB (and MySQL) - https://github.com/inspircd/inspircd-docs/issues/239: tls and srv not available in MariaDB - https://github.com/umami-software/umami/discussions/2943: BIN_TO_UUID() not available in MariaDB (MDEV-15854 <https://jira.mariadb.org/browse/MDEV-15854>) - https://github.com/diesel-rs/diesel/issues/1882: Create a distinct backend for MariaDB - https://github.com/grafana/grafana: Create a dedicated DataStore for MariaDB - https://github.com/drizzle-team/drizzle-orm/pull/1692: WIP: Add support for Maria DB (core API implementation) - https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1079753: Create mariadb specific dbconfig-mariadb - https://core.trac.wordpress.org/ticket/60037: Fix version checking in next release - https://gitlab.com/zoph-project/zoph/-/issues/267: give a thumbs up to their own addition of MariaDB to documentation (preferably recommend MariaDB)

2 1