- developers - lists.mariadb.org

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 11 Dec '20

11 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

Re: [Maria-developers] MDEV-21265: IN predicate conversion to IN subquery should be allowed for a broader set of datatype comparison
by Sergey Petrunia 26 Nov '20

26 Nov '20

Hi Varun, Please find some cosmetic input below. Ok to push after addresed. > diff --git a/mysql-test/main/subselect4.test b/mysql-test/main/subselect4.test > index 6f5eb1f2985f..e9da028618f6 100644 > --- a/mysql-test/main/subselect4.test > +++ b/mysql-test/main/subselect4.test > @@ -2238,3 +2238,38 @@ SET join_cache_level= @save_join_cache_level; > DROP TABLE t1,t2,t3,t4; > > --echo # End of 10.2 tests > + > +--echo # > +--echo # MDEV-21265: IN predicate conversion to IN subquery should be allowed for a broader set of datatype comparison > +--echo # > + > +CREATE TABLE t1(a VARCHAR(50) collate utf8_general_ci, b INT); > +INSERT INTO t1 VALUES ('abc',1), ('def', 2), ('ghi', 3), ('jkl', 4), ('mno', 5); > + > +CREATE TABLE t2(a VARCHAR(50) collate utf8mb4_general_ci, b INT); > +INSERT INTO t2 VALUES ('abc',1), ('def', 2), ('ghi', 3), ('jkl', 4), ('mno', 5); > + > +set in_predicate_conversion_threshold=2; > + ... > +EXPLAIN > +SELECT * FROM t2 WHERE (t2.a,t2.b) IN (('abx',1),('def',2), ('abc', 3)); > + > +set names default; > +DROP TABLE t1,t2; > + > +--echo # End of 10.3 tests Please restore the value of in_predicate_conversion_threshold. > diff --git a/sql/item_subselect.h b/sql/item_subselect.h > index 1cea7291c9ef..d1b2f86088d9 100644 > --- a/sql/item_subselect.h > +++ b/sql/item_subselect.h > @@ -573,6 +573,13 @@ class Item_in_subselect :public Item_exists_subselect > TRUE<=>registered in the list of semijoins in outer select > */ > bool is_registered_semijoin; > + > + Item_func_not_all *upper_item; // point on NOT/NOP before ALL/SOME subquery Is this necessary to touch the above member? I think it's better leave it as is. > + > + /* > + SET to TRUE if IN subququery is converted from an IN rredicate > + */ > + bool converted_from_in_predicate; ... > diff --git a/sql/sql_type.h b/sql/sql_type.h > index 907225b7c505..a1403165fe38 100644 > --- a/sql/sql_type.h > +++ b/sql/sql_type.h > @@ -1291,9 +1291,21 @@ class Type_handler > Item *target_expr, Item *target_value, > Item_bool_func2 *source, > Item *source_expr, Item *source_const) const= 0; > + > + /* > + @brief > + Check if an IN subquery allows materialization or not > + @param > + inner expression on the inner side of the IN subquery > + outer expression on the inner side of the IN subquery Typo. Should it should be 'outer' ? > + is_in_predicate SET to true if IN subquery was converted from an > + IN predicate or we are checking if materialization > + strategy can be used for an IN predicate > + */ > virtual bool > subquery_type_allows_materialization(const Item *inner, > - const Item *outer) const= 0; > + const Item *outer, > + bool is_in_predicate) const= 0; > /** > Make a simple constant replacement item for a constant "src", > so the new item can futher be used for comparison with "cmp", e.g.: BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0