December 2020 - developers - lists.mariadb.org

Re: [Maria-developers] 33787ea47a3: MDEV-6899: Database Dump has Syntax Error When Importing
by Sergei Golubchik 23 Dec '20

23 Dec '20

Hi, Anel! On Dec 23, Anel Husakovic wrote: > revision-id: 33787ea47a3 (mariadb-10.2.31-588-g33787ea47a3) > parent(s): ceef26cf866 > author: Anel Husakovic <anel(a)mariadb.org> > committer: Anel Husakovic <anel(a)mariadb.org> > timestamp: 2020-11-17 15:11:14 +0100 > message: > > MDEV-6899: Database Dump has Syntax Error When Importing > > --- > mysql-test/r/mysqldump.result | 32 ++++++++++++++++++++++++++++++++ > mysql-test/t/mysqldump.test | 26 ++++++++++++++++++++++++++ > sql/events.cc | 10 ++++++++-- > sql/sql_show.cc | 4 +++- > 4 files changed, 69 insertions(+), 3 deletions(-) > > diff --git a/sql/events.cc b/sql/events.cc > index abac2833833..2880113bc75 100644 > --- a/sql/events.cc > +++ b/sql/events.cc > @@ -680,6 +680,12 @@ send_show_create_event(THD *thd, Event_timed *et, Protocol *protocol) > if (et->get_create_event(thd, &show_str)) > DBUG_RETURN(TRUE); > > + LEX_STRING def_show_str= show_str.lex_string(); > + if (!strcmp(&def_show_str.str[def_show_str.length -1], ";")) > + { > + --def_show_str.length; > + def_show_str.str[def_show_str.length]='\0'; > + } No, this is clearly wrong. The bug is that semicolon even gets into the event definition, see other commands in the bug report. You should fix that, not strip it away in SHOW CREATE, which will only hide the bug. Why is the semicolon parsed as a part of the event anyway? > field_list.push_back(new (mem_root) > Item_empty_string(thd, "Event", NAME_CHAR_LEN), > mem_root); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] ab7092a415f: MDEV-24179: Assertion `m_status == DA_ERROR || m_status == DA_OK ||
by Sergei Golubchik 21 Dec '20

21 Dec '20

Hi, Rucha! On Dec 21, Rucha Deodhar wrote: > revision-id: ab7092a415f (mariadb-10.2.31-584-gab7092a415f) > parent(s): 9b30212f15e > author: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > committer: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > timestamp: 2020-12-18 10:20:20 +0530 > message: > > MDEV-24179: Assertion `m_status == DA_ERROR || m_status == DA_OK || > m_status == DA_OK_BULK' failed in Diagnostics_area::message() > > Analysis: Assertion failure happens because we reach the maximum limit to > examine rows. > Fix: Return the error state. > > diff --git a/sql/sql_show.cc b/sql/sql_show.cc > index 5544c765775..73b7a84fae1 100644 > --- a/sql/sql_show.cc > +++ b/sql/sql_show.cc > @@ -5046,6 +5046,12 @@ int get_all_tables(THD *thd, TABLE_LIST *tables, COND *cond) > continue; > } > > + if (thd->killed == ABORT_QUERY) > + { > + error= 0; > + goto err; > + } > + you already have exactly the same if() few lines below. may be it shouldn't be duplicated, but moved down to the err: label? > DEBUG_SYNC(thd, "before_open_in_get_all_tables"); > if (fill_schema_table_by_open(thd, FALSE, > table, schema_table, Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 889f90365ef: MDEV-23536 : Race condition between KILL and transaction commit
by Sergei Golubchik 20 Dec '20

20 Dec '20

Hi, Jan! On Dec 19, Jan LindstrÃ¶m wrote: > revision-id: 889f90365ef (mariadb-10.2.31-623-g889f90365ef) > parent(s): 74223c33d1a > author: Jan Lindström <jan.lindstrom(a)mariadb.com> > committer: Jan Lindström <jan.lindstrom(a)mariadb.com> > timestamp: 2020-12-18 16:10:39 +0200 > message: > > MDEV-23536 : Race condition between KILL and transaction commit ... > If you look carefully into the above, you can conclude that > thd->free_connection() can be called concurrently with > KILL/thd->awake(). Which is the bug. And it is partially fixed in > THD::~THD(), that is destructor waits for KILL completion: > > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index 7eaafbd9044..46aeb4f2bb3 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -1437,6 +1437,31 @@ void THD::cleanup(void) > void THD::free_connection() > { > DBUG_ASSERT(free_connection_done == 0); > +#ifdef WITH_WSREP > + /* > + wsrep_rgi should be protected with LOCK_thd_data mutex and > + mutexing ordering rules state that LOCK_thd_data must be taken > + before LOCK_thd_kill. > + */ > + if (wsrep_rgi && WSREP(this)) mysql_mutex_lock(&LOCK_thd_data); > +#endif > + /* > + Other threads may have a lock on LOCK_thd_kill to ensure that this > + THD is not deleted while they access it. The following mutex_lock > + ensures that no one else is using this THD and it's now safe to > + continue. > + */ > + mysql_mutex_lock(&LOCK_thd_kill); > + mysql_mutex_unlock(&LOCK_thd_kill); Note that THD destructor starts from `assert_not_linked()` - it's important, because if the THD is linked in the list of threads, then someone might start using it after you released LOCK_thd_kill. With the new logic you need to move assert_not_linked to free_connection(). otherwise ok to push > + > +#ifdef WITH_WSREP > + if (rpl_group_info *rgi= wsrep_rgi) > + { > + wsrep_rgi= 0; > + if (WSREP(this)) mysql_mutex_unlock(&LOCK_thd_data); > + delete rgi; > + } > +#endif > my_free(db); > db= NULL; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

3 4

Re: [Maria-developers] a36f27e200e: Fixes that enables my_new.cc (new wrapper using my_malloc)
by Sergei Golubchik 11 Dec '20

11 Dec '20

Hi, Michael! On Dec 11, Michael Widenius wrote: > revision-id: a36f27e200e (mariadb-10.5.2-275-ga36f27e200e) > parent(s): 9f2bbf84e8d > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2020-09-24 17:26:40 +0300 > message: > > Fixes that enables my_new.cc (new wrapper using my_malloc) > > This is not enabled by default as there are leaks in the > server that needs to be fixed first. One can compile > with -DREALLY_USE_MYSYS_NEW -DSF_REMEMBER_FRAMES=14 to find the > memory leaks from 'new' The change in my_new.cc is ok. The change in my_global.h and CMakeLists.txt is confusing and redundant. You unconditionaly define USE_MYSYS_NEW then conditionally undefine it again? Everything already worked just fine. By default cxx new is used, if one wants to use mysys version, it's done with -DHAVE_CXX_NEW=0 this works for me in 10.5 and fails to compile my_new.cc, as it should. Please, only keed your my_new.cc changes and say in the commit message "One can compile with -DHAVE_CXX_NEW=0 to find memory leaks from 'new'" As for -DSF_REMEMBER_FRAMES=14, perhaps it's better to do it automatically? Like #ifdef USE_MYSYS_NEW #define SF_REMEMBER_FRAMES 14 #else #define SF_REMEMBER_FRAMES 8 #endif or may be even just always make it 14. It should be easy to enable this feature, right? > diff --git a/CMakeLists.txt b/CMakeLists.txt > index 80fac4b2ccc..f97a6354f19 100644 > --- a/CMakeLists.txt > +++ b/CMakeLists.txt > @@ -362,10 +362,8 @@ ENDIF() > # Run platform tests > INCLUDE(configure.cmake) > > -# force -DUSE_MYSYS_NEW unless already done by HAVE_CXX_NEW > -IF(NOT HAVE_CXX_NEW) > - ADD_DEFINITIONS(-DUSE_MYSYS_NEW) > -ENDIF() > +# Always use USE_MYSYS_NEW > +ADD_DEFINITIONS(-DUSE_MYSYS_NEW) > > # Find header files from the bundled libraries > # (wolfssl, readline, pcre2, etc) > diff --git a/include/my_global.h b/include/my_global.h > index 86ef5f882f6..8862a397f73 100644 > --- a/include/my_global.h > +++ b/include/my_global.h > @@ -527,6 +527,9 @@ typedef unsigned short ushort; > duplicate declaration of __cxa_pure_virtual, solved by declaring it a > weak symbol. > */ > +#ifndef REALLY_USE_MYSYS_NEW > +#undef USE_MYSYS_NEW > +#endif > #if defined(USE_MYSYS_NEW) && ! defined(DONT_DECLARE_CXA_PURE_VIRTUAL) > C_MODE_START > int __cxa_pure_virtual () __attribute__ ((weak)); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 9f2bbf84e8d1: Fixed my_addr_resolve
by Sergei Golubchik 10 Dec '20

10 Dec '20

Hi, Michael! On Dec 03, Michael Widenius wrote: > revision-id: 9f2bbf84e8d1 (mariadb-10.5.2-274-g9f2bbf84e8d1) > parent(s): 173116c3a573 > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2020-09-24 17:26:40 +0300 > message: > > Fixed my_addr_resolve > > At least on openSUSE Leap 15.1 one should not use the dli_fbase offset > provided by dladdr() for addresses in the main program. > Without this patch the stack traces from sf_report_leaked_memory() > are not usable. Sorry, I was not able to repeat it. I built on openSUSE Leap and the stack trace was perfectly usable without your patch. I tried both `kill -ABRT` and an artificially introduced memory leak. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 53a88b2b7032: Optimize usage of c_ptr(), c_ptr_quick() and String::alloc()
by Sergei Golubchik 04 Dec '20

04 Dec '20

Hi, Michael! On Dec 03, Michael Widenius wrote: > revision-id: 53a88b2b7032 (mariadb-10.5.2-272-g53a88b2b7032) > parent(s): 0aab153e05be > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2020-09-24 17:26:40 +0300 > message: > > Optimize usage of c_ptr(), c_ptr_quick() and String::alloc() > > The proble was that hen using String::alloc() to allocate a string, "hen" ? after a minute of staring I realized that you probably meant "when". (and "problem") > the String ensures that there is space for an extra NULL byte in the > buffer and if not, reallocates the string. This is a problem with the > String::set_int() that calls alloc(21), which forces > an extra malloc/free to happen. > > - We do not anymore re-allocate String if alloc() is called with the > Allocated_length. This reduces number of malloc() allocations, > especially one big re-allocation in Protocol::send_result_Set_metadata() > for almost every query that produced a result to the connnected client. > - Avoid extra mallocs when using LONGLONG_BUFFER_SIZE > This can now be done as alloc() doesn't increase buffers if new length is > not bigger than old one. > - c_ptr() is redesigned to be safer (but a bit longer) than before. > - Remove wrong usage of c_ptr_quick() > c_ptr_quick() where used in many cases to get the pointer to the used > buffer, even when it didn't need to be \0 terminated. In this case > ptr() is a better substitute. > Another problem with c_ptr_quick() is that it didn't guarantee that > the string would be \0 terminated. > - item_val_str(), an API function not used currently by the server, > now always returns a null terminated string (before it didn't always > do that). > - Ensure that all String allocations uses STRING_PSI_MEMORY_KEY. The old > mixed usage of performance keys caused assert's when String buffers > where shrunk. > - Binary_string::shrink() is simplifed > > diff --git a/sql/item.cc b/sql/item.cc > index 4a26cb6c140c..328fe96018a5 100644 > --- a/sql/item.cc > +++ b/sql/item.cc > @@ -4574,7 +4574,7 @@ const String *Item_param::value_query_val_str(THD *thd, String *str) const > break; > } > DBUG_ASSERT(str->length() <= typelen); > - buf= str->c_ptr_quick(); > + buf= (char*) str->ptr(); > ptr= buf + str->length(); > *ptr++= '\''; > ptr+= (uint) my_TIME_to_str(&value.time, ptr, decimals); this is just wrong. One cannot simply write data at the String's end pointer. One must alloc() the space first. > diff --git a/sql/partition_info.cc b/sql/partition_info.cc > index 8ad14ca260c4..ce72a7af051d 100644 > --- a/sql/partition_info.cc > +++ b/sql/partition_info.cc > @@ -126,7 +126,7 @@ partition_info *partition_info::get_clone(THD *thd) > /** > Mark named [sub]partition to be used/locked. > > - @param part_name Partition name to match. > + @param part_name Partition name to match. Must be \0 terminated! > @param length Partition name length. > > @return Success if partition found > @@ -172,9 +172,9 @@ bool partition_info::add_named_partition(const char *part_name, size_t length) > else > bitmap_set_bit(&read_partitions, part_def->part_id); > } > - DBUG_PRINT("info", ("Found partition %u is_subpart %d for name %s", > + DBUG_PRINT("info", ("Found partition %u is_subpart %d for name %.*s", > part_def->part_id, part_def->is_subpart, > - part_name)); > + length, part_name)); why did you specify a length if the name "must be \0 terminated!" ? (and I agree that it must be) > DBUG_RETURN(false); > } > > diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc > index 633d969b3dec..2b603380aa6c 100644 > --- a/sql/sql_plugin.cc > +++ b/sql/sql_plugin.cc > @@ -404,12 +404,16 @@ static int item_value_type(struct st_mysql_value *value) > static const char *item_val_str(struct st_mysql_value *value, > char *buffer, int *length) > { > + size_t org_length= *length; > - String str(buffer, *length, system_charset_info), *res; > + String str(buffer, org_length, system_charset_info), *res; > if (!(res= ((st_item_value_holder*)value)->item->val_str(&str))) > return NULL; > *length= res->length(); > - if (res->c_ptr_quick() == buffer) > + if (res->ptr() == buffer && res->length() < org_length) > + { > + buffer[res->length()]= 0; > return buffer; > + } Okay. This looked like the *only* correct usage of c_ptr_quick(). Agree, better to remove c_ptr_quick(). > > /* > Lets be nice and create a temporary string since the > diff --git a/sql/sql_string.cc b/sql/sql_string.cc > index e2defba434dd..8ab156d0e556 100644 > --- a/sql/sql_string.cc > +++ b/sql/sql_string.cc > @@ -1251,21 +1259,16 @@ bool String::append_semi_hex(const char *s, uint len, CHARSET_INFO *cs) > // Shrink the buffer, but only if it is allocated on the heap. > void Binary_string::shrink(size_t arg_length) > { > - if (!is_alloced()) > - return; > - if (ALIGN_SIZE(arg_length + 1) < Alloced_length) > + if (is_alloced() && ALIGN_SIZE(arg_length + 1) < Alloced_length) > + { > + char *new_ptr; > + /* my_realloc() can't fail as new buffer is less than the original one */ > + if ((new_ptr= (char*) my_realloc(STRING_PSI_MEMORY_KEY, Ptr, arg_length, > + MYF(thread_specific ? > + MY_THREAD_SPECIFIC : 0)))) why do you need an if() if "my_realloc() can't fail" ? (and it really cannot, you're right) > { > - char* new_ptr; > - if (!(new_ptr = (char*)my_realloc(STRING_PSI_MEMORY_KEY, Ptr, arg_length, > - MYF(thread_specific ? MY_THREAD_SPECIFIC : 0)))) > - { > - Alloced_length = 0; > - real_alloc(arg_length); > - } > - else > - { > - Ptr = new_ptr; > - Alloced_length = (uint32)arg_length; > - } > + Ptr= new_ptr; > + Alloced_length= (uint32) arg_length; > } > + } > } > diff --git a/sql/sql_string.h b/sql/sql_string.h > index 2ef817ea0adc..01d5211fe850 100644 > --- a/sql/sql_string.h > +++ b/sql/sql_string.h > @@ -599,25 +599,34 @@ class Binary_string: public Static_binary_string > > inline char *c_ptr() > { > - DBUG_ASSERT(!alloced || !Ptr || !Alloced_length || > - (Alloced_length >= (str_length + 1))); > - > - if (!Ptr || Ptr[str_length]) // Should be safe > - (void) realloc(str_length); > + if (unlikely(!Ptr)) > + return (char*) ""; > + /* > + Here we assume that any buffer used to initalize String has > + an end \0 or have at least an accessable character at end. > + This is to handle the case of String("Hello",5) efficently. > + */ > + if (unlikely(!alloced && !Ptr[str_length])) No, this is wrong. Note the difference between String a("Hello", 5) and char hello[5]; String a(buf, 5); Your assumption should only apply to the first case, not to the second. In the first case alloced==Alloced_length==0, in the second case only alloced==0 and Alloced_length==5. So in the if() above you need to look at Alloced_length. > + return Ptr; > + if (str_length < Alloced_length) > + { > + Ptr[str_length]=0; > + return Ptr; > + } > + (void) realloc(str_length+1); /* This will add end \0 */ > return Ptr; > } > + /* Use c_ptr() instead. This will be deleted soon, kept for compatiblity */ > inline char *c_ptr_quick() > { > - if (Ptr && str_length < Alloced_length) > - Ptr[str_length]=0; > - return Ptr; > + return c_ptr_safe(); > } > inline char *c_ptr_safe() > { > if (Ptr && str_length < Alloced_length) > Ptr[str_length]=0; > else > - (void) realloc(str_length); > + (void) realloc(str_length + 1); what's the difference between c_ptr_safe and c_ptr, again? they're very similar, so they'd need a good comment explaining when to use what. > return Ptr; > } > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 994ea2af3973: Optimize size of lex structures
by Sergei Golubchik 04 Dec '20

04 Dec '20

Hi, Michael! On Dec 03, Michael Widenius wrote: > revision-id: 994ea2af3973 (mariadb-10.5.2-269-g994ea2af3973) > parent(s): 77500a1a1107 > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2020-09-17 12:24:58 +0300 > message: > > Optimize size of lex structures > > LEX, st_select_lex, st_select_unit optimized for space: > - Use bit fields for bool variables > - Ensure that all bit fields are initialized (improves > performance for init functions as all bit fields can be > initalized with one memory access) > - Move members around in above structures to remove alignment > gaps I did not get any reply to my email From: Sergei Golubchik <serg(a)mariadb.org> To: Michael Widenius <michael.widenius(a)gmail.com> Cc: maria-developers(a)lists.launchpad.net Subject: Re: c3ecf0d6243: Optimize size of lex structures Date: Wed, 9 Sep 2020 15:57:23 +0200 and I don't see any related changes in this commit. Please, do at least one of the two. Regards, Sergei

1 0

Re: [Maria-developers] 77500a1a1107: Added typedef decimal_digits_t (uint16) for number of decimals
by Sergei Golubchik 04 Dec '20

04 Dec '20

Hi, Michael! I did not get any reply to my email From: Sergei Golubchik <serg(a)mariadb.org> To: Michael Widenius <michael.widenius(a)gmail.com> Cc: maria-developers(a)lists.launchpad.net Subject: Re: c79c6f78205: Added typedef decimal_digits_t (uint16) for number of decimals Date: Wed, 9 Sep 2020 13:34:57 +0200 and I don't see any related changes in this commit. Please, do at least one of the two. Regards, Sergei On Dec 03, Michael Widenius wrote: > revision-id: 77500a1a1107 (mariadb-10.5.2-268-g77500a1a1107) > parent(s): 32104b969513 > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2020-09-17 12:24:58 +0300 > message: > > Added typedef decimal_digits_t (uint16) for number of decimals > > For fields and Item's uint8 should be good enough. After > discussions with Alexander Barkov we choose uint16 (for now) > as some format functions may accept +256 digits. > > The reason for this patch was to make the storage of decimal > digits simlar. Before this patch decimals was stored/used as > uint8, int and uint. > > Changed most decimal variables and functions to use the new > typedef. >

1 0

Re: [Maria-developers] 32104b969513: MDEV-23001 Precreate static Item_bool() to simplify code
by Sergei Golubchik 04 Dec '20

04 Dec '20

Hi, Michael! I did not get any reply to my email From: Sergei Golubchik <serg(a)mariadb.org> To: Michael Widenius <michael.widenius(a)gmail.com> Cc: maria-developers(a)lists.launchpad.net Subject: Re: bf8bd057344: MDEV-23001 Precreate static Item_bool() to simplify code Date: Tue, 8 Sep 2020 20:39:35 +0200 and I don't see any related changes in this commit. Please, do at least one of the two. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org On Dec 03, Michael Widenius wrote: > revision-id: 32104b969513 (mariadb-10.5.2-267-g32104b969513) > parent(s): 32a29afea777 > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2020-09-17 12:24:45 +0300 > message: > > MDEV-23001 Precreate static Item_bool() to simplify code > > The following changes where done: > - Create global Item: Item_false and Item_true > - Replace all creation if 'FALSE' and 'TRUE' top level items used for > WHERE/HAVING/ON clauses to use Item_false and Item_true. > > The benefit are: > - Less and faster code > - No test needed if we where able to create the new item. > - Fixed possible errors if 'new' would have failed for the Item_bool's >

1 0

[Maria-developers] MDEV-21287 dev community opinion wanted
by Pierre-Jean Clement 02 Dec '20

02 Dec '20

Hi all, I created the Jira ticket MDEV-21287 10 months ago and didn't get much feedback. I tried to take a shot at it myself, but found out that the resolution MDEV-20403 introduced this behavior by purposefully evaluating 'update functions' before the BEFORE UPDATE trigger. I couldn't really understand why this was necessary, and I'd like to get feedback from the dev community to try and understand what is really supposed to happen on update. I kinda hate getting my trigger called with a new value that's not going to be persisted anyway, what do you think about that? Regards, Pierre-Jean

2 3