- developers - lists.mariadb.org

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