- developers - lists.mariadb.org

Next step on MDEV-34705, implement binlog in InnoDB
by Kristian Nielsen 05 Jan '25

05 Jan '25

Hi Marko, Time flies, somehow it's already more than a year since our first discussions on implementing the binlog in InnoDB and avoiding the extra fsync() and complexity of the two-phase commit between InnoDB and binlog. But things have progressed, and I have now reached the point where most of the basic groundwork is implemented. Event groups are binlogged to InnoDB tablespaces. Binlog dump thread can read the binlog and send to slave, and replication is working. Large event groups are split into pieces, bounding the amount of data that needs to be atomically written in mini-transactions and at commit time. There are still many details left, but mostly in the server-layer replication code which should be manageable, just will take some time to get completed. I think now is a good time for you to take a first real look at the InnoDB part of the changes, I would really value your input. The main part of the InnoDB code is in two files: 1. handler/handler0binlog.cc for the high-level part that deals mostly with the new binlog file format and interfacing to the server layer. 2. fsp/fsp0binlog.cc for the low-level part that most tightly interacts with the InnoDB mini-transactions and buffer pool. The most interesting part for you to look at is fsp/fsp0binlog.cc (~1k lines), though I'm happy to hear comments on any part of the patch, of course. The code is pushed to GitHub in the branch knielsen_binlog_in_engine: https://github.com/MariaDB/server/commits/knielsen_binlog_in_engine and I've also attached the complete patch. This is my first major patch for InnoDB, so there will undoubtedly be a number of style changes required. But the overall structure of the code should now be close to what I imagine would be the final result, with some pending ToDo steps marked in comments in the code, and detailed in the below list, some of which we discussed a bit already. I hope you will take a look at the patch and let me know of any questions or other things you need from me. Maybe we can also find a chance to discuss further if you will come to FOSDEM start of February, or I could visit sometimes in Finland. - Kristian. ---- Some known outstanding issues in the InnoDB part: - We previously discussed removing some of the page header overhead for binlog tablespaces. Currently the code just leaves alone the first FIL_PAGE_DATA bytes (38) and the last FIL_PAGE_DATA_END (8 IIRC). - We discussed previously to write the current LSN at the start of the tablespace, and use this in recovery to handle that we have only two tablespace IDs that are reused. So we need code in recovery that checks the LSN at the start of the tablespace, and skips redo records with LSN smaller than this. - We want to avoid the double-write buffer for binlog pages, at least for the first page write (most pages will only be written as full pages). You mentioned an idea to completely avoid the double-write buffer and instead do some specific code for recovery in the uncommon case where a partial binlog page is written to disk due to low commit activity. - The flushing of binlog pages to disk currently happens in a dedicated thread in the background. I'd welcome ideas on how to do this differently. It is good to flush binlog pages quickly and re-use their buffer pool entries for something better. Also writing the pages to disk quickly (not necessarily fsync()'ing) makes the data readable by mysqlbinlog. - Checksum and encryption should use the standard InnoDB mechanism. I assume checksum is already handled in the code through using the buffer pool and mini-transactions to read/write pages. Not sure about encryption. I need to implement that the code handles checksum and decryption when reading the pages manually from the file (not through buffer pool).

2 2

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

05 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 6

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

01 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

1 0

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