May 2015 - developers - lists.mariadb.org

Re: [Maria-developers] [Commits] 6efaf34: MDEV-7011: MAX_STATEMENT_TIME has no effect in a procedure after a previous successful statement
by Sergei Golubchik 29 May '15

29 May '15

Hi, Sanja! On May 21, sanja(a)mariadb.com wrote: > revision-id: 6efaf349adec30115bdb94b9a9927f7c5103ed99 > parent(s): 3e55ef26d49a900782d2c2bb2c03470faed6ec9d > committer: Oleksandr Byelkin > branch nick: server > timestamp: 2015-05-21 16:31:24 +0200 > message: > > MDEV-7011: MAX_STATEMENT_TIME has no effect in a procedure after a previous successful statement > > Do not reset timer inside stored procedures and functions. > > diff --git a/sql/sql_class.h b/sql/sql_class.h > index ca532ae..ebb76df 100644 > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -3966,6 +3966,8 @@ class THD :public Statement, > void reset_query_timer() > { > #ifndef EMBEDDED_LIBRARY > + if (spcont || in_sub_stmt) > + return; > if (!query_timer.expired) > thr_timer_end(&query_timer); > #endif I'd expect reset_query_timer() to use the same condition that set_query_timer() does: void set_query_timer() { if (!variables.max_statement_time || spcont || in_sub_stmt || slave_thread || query_timer.expired == 0) return; thr_timer_settime(&query_timer, variables.max_statement_time); } So, I'd think that reset_query_timer() could look like void reset_query_timer() { if (spcont || in_sub_stmt || slave_thread) return; if (!query_timer.expired) thr_timer_end(&query_timer); } Regards, Sergei

2 1

[Maria-developers] [GSoC] And it begins
by sriram patil 29 May '15

29 May '15

Hi All, Today is the official day (at least in my timezone) to start working on the GSoC'15 projects. I am totally excited! I will be working on MaxScale, MXS-2, for next 3 months. I will be posting a blog link every Monday starting from next week which will give an idea about what I did in the week previous to that and the tasks lined up for that week. I hope you all follow my blog. It will be great to get some inputs from the community as well. Also, all the best to all the other students who will be working with MariaDB this summer. Thanks, Sriram

2 2

Re: [Maria-developers] 8928497: MDEV-7067: Server outputs Galera (WSREP) information, even if Galera is disabled
by Sergei Golubchik 29 May '15

29 May '15

Hi, Nirbhay! On May 09, Nirbhay Choubey wrote: > revision-id: 8928497acbe3e17a105897f405d5e5138d57a249 > parent(s): 3674c363a7e77e534c3e0d28659dc614c53fbcbc > committer: Nirbhay Choubey > branch nick: 10.1-wsrep > timestamp: 2015-05-09 16:11:59 -0400 > message: > > MDEV-7067: Server outputs Galera (WSREP) information, even if Galera is disabled > > * mysqld_safe: Since wsrep_on variable is mandatory in 10.1, skip wsrep > position recovery if its OFF. > * mysqld: Remove "-wsrep" from server version > * mysqld: Remove wsrep patch version from @@version_comment > * mysqld: Introduce @@wsrep_patch_version Looks ok, I just didn't like the wsrep.cmake change. See below. > diff --git a/cmake/wsrep.cmake b/cmake/wsrep.cmake > index c37cf74..305ab27 100644 > --- a/cmake/wsrep.cmake > +++ b/cmake/wsrep.cmake > @@ -28,29 +28,22 @@ OPTION(WITH_WSREP "WSREP replication API (to use, e.g. Galera Replication librar > # Set the patch version > SET(WSREP_PATCH_VERSION "10") > > -# MariaDB addition: Revision number of the last revision merged from > -# codership branch visible in @@version_comment. > -# Branch : codership-mysql/5.6 > -SET(WSREP_PATCH_REVNO "4144") # Should be updated on every merge. > - > -# MariaDB addition: Revision number of the last revision merged from > -# Branch : lp:maria/maria-10.0-galera > -SET(WSREP_PATCH_REVNO2 "3919") # Should be updated on every merge. > - > -# MariaDB: Obtain patch revision number: > -# Update WSREP_PATCH_REVNO if WSREP_REV environment variable is set. > -IF (DEFINED ENV{WSREP_REV}) > - SET(WSREP_PATCH_REVNO $ENV{WSREP_REV}) > -ENDIF() > > -SET(WSREP_INTERFACE_VERSION 25) > +# Obtain wsrep API version > +EXECUTE_PROCESS( > + COMMAND sh -c "grep WSREP_INTERFACE_VERSION ${MySQL_SOURCE_DIR}/wsrep/wsrep_api.h | cut -d '\"' -f 2" > + OUTPUT_VARIABLE WSREP_API_VERSION > + RESULT_VARIABLE RESULT > +) I don't like calling grep|cut from cmake files. If you want to extract the version from wsrep_api.h, use cmake commands, e.g. FILE(STRINGS "${MySQL_SOURCE_DIR}/wsrep/wsrep_api.h" WSREP_API_H LIMIT_COUNT 1 REGEX "WSREP_INTERFACE_VERSION") STRING(REGEX MATCH .... > +STRING(REGEX REPLACE "(\r?\n)+$" "" WSREP_API_VERSION "${WSREP_API_VERSION}") > > -SET(WSREP_VERSION > - "${WSREP_INTERFACE_VERSION}.${WSREP_PATCH_VERSION}.r${WSREP_PATCH_REVNO}") > +SET(WSREP_VERSION "${WSREP_API_VERSION}.${WSREP_PATCH_VERSION}" > + CACHE STRING "WSREP version") make it INTERNAL, not STRING > SET(WSREP_PROC_INFO ${WITH_WSREP}) > > IF(WITH_WSREP) > - SET(COMPILATION_COMMENT "${COMPILATION_COMMENT}, wsrep_${WSREP_VERSION}") > + SET(WSREP_PATCH_VERSION "wsrep_${WSREP_VERSION}") > ENDIF() > > diff --git a/mysql-test/suite/sys_vars/r/sysvars_wsrep.result b/mysql-test/suite/sys_vars/r/sysvars_wsrep.result > index 9c392b1..1763078 100644 > --- a/mysql-test/suite/sys_vars/r/sysvars_wsrep.result > +++ b/mysql-test/suite/sys_vars/r/sysvars_wsrep.result > @@ -365,6 +365,20 @@ NUMERIC_BLOCK_SIZE NULL > ENUM_VALUE_LIST TOI,RSU > READ_ONLY NO > COMMAND_LINE_ARGUMENT OPTIONAL > +VARIABLE_NAME WSREP_PATCH_VERSION > +SESSION_VALUE NULL > +GLOBAL_VALUE wsrep_25.10 > +GLOBAL_VALUE_ORIGIN COMPILE-TIME > +DEFAULT_VALUE NULL > +VARIABLE_SCOPE GLOBAL > +VARIABLE_TYPE VARCHAR > +VARIABLE_COMMENT wsrep patch version > +NUMERIC_MIN_VALUE NULL > +NUMERIC_MAX_VALUE NULL > +NUMERIC_BLOCK_SIZE NULL > +ENUM_VALUE_LIST NULL > +READ_ONLY YES > +COMMAND_LINE_ARGUMENT NULL > VARIABLE_NAME WSREP_PROVIDER > SESSION_VALUE NULL > GLOBAL_VALUE none > diff --git a/mysql-test/suite/sys_vars/r/wsrep_patch_version_basic.result b/mysql-test/suite/sys_vars/r/wsrep_patch_version_basic.result > new file mode 100644 > index 0000000..80f29e6 > --- /dev/null > +++ b/mysql-test/suite/sys_vars/r/wsrep_patch_version_basic.result I've actually stopped creating sys_var tests for every new variable (even though I was the one who introduced sys_vars.all_vars test in the first place :). With the new I_S.SYSTEM_VARIABLES table we have all variables listed there anyway, with all properties. So there's no need to copy these boilerplate tests over and over. > diff --git a/mysql-test/suite/wsrep/suite.pm b/mysql-test/suite/wsrep/suite.pm > index 02dff24..33744a6 100644 > --- a/mysql-test/suite/wsrep/suite.pm > +++ b/mysql-test/suite/wsrep/suite.pm > @@ -27,6 +27,7 @@ push @::global_suppressions, > qr(WSREP: Could not open saved state file for reading: ), > qr(WSREP: option --wsrep-casual-reads is deprecated), > qr(WSREP: --wsrep-casual-reads=ON takes precedence over --wsrep-sync-wait=0), > + qr|WSREP: access file$gvwstate.dat$ failed$No such file or directory$|, why? > ); > > diff --git a/sql/mysqld.cc b/sql/mysqld.cc > index 14d904b..49a639b 100644 > --- a/sql/mysqld.cc > +++ b/sql/mysqld.cc > @@ -1902,13 +1902,15 @@ static void __cdecl kill_server(int sig_ptr) > } > #endif > > - if (WSREP_ON) > + /* Stop wsrep threads in case they are running. */ > wsrep_stop_replication(NULL); Does this print any messages? Regards, Sergei

2 1

Re: [Maria-developers] [Commits] 6f03ea7: MDEV-8202 - st_select_lex::master_unit() takes 0.17% in OLTP RO
by Sergei Golubchik 28 May '15

28 May '15

Hi, Sergey! On May 21, svoj(a)mariadb.org wrote: > revision-id: 6f03ea78aa4a70b9a9f2f096f1d7a6c282ca9575 > parent(s): 79372cf58e426eb60acb9f0abf862a1f0fe0e4e3 > committer: Sergey Vojtovich > branch nick: mariadb > timestamp: 2015-05-21 19:35:49 +0400 > message: > > MDEV-8202 - st_select_lex::master_unit() takes 0.17% in OLTP RO > > To make st_select_lex::master_unit() inlinable: > - moved it's definition to sql_lex.h > - removed base class virtual master_unit() declaration since this method is > specific to st_select_lex ok to push Regards, Sergei

1 0

[Maria-developers] MDEV-7950 Item_func::type() takes 0.26% in OLTP RO
by Alexander Barkov 28 May '15

28 May '15

Hello Sergey, Please review the next step for MDEV-7950. It does the following: - Adds specific implementations of virtual method get_mm_tree() into Item_func_like, Item_bool_rowready_func2, Item_func_spatial_rel - Gets rid of two virtual calls: select_optimize() and have_rev_func(). - Removes virtual methods Item_func::select_optimize(), Item_func::have_rev_func(), as well as enum Item_func::optimize_type, because they are not needed any more. Thanks.

2 1

Re: [Maria-developers] [Commits] 8a595a9: cleanup: LOAD DATA replication support in IO_CACHE
by Sergey Vojtovich 28 May '15

28 May '15

Hi Sergei, a few thoughts/questions inline. On Tue, May 26, 2015 at 03:37:36PM +0200, serg(a)mariadb.org wrote: > revision-id: 8a595a9825de286ad073f6f043db4c0dce88a2ea > parent(s): 3e55ef26d49a900782d2c2bb2c03470faed6ec9d > committer: Sergei Golubchik > branch nick: maria > timestamp: 2015-05-26 15:09:25 +0200 > message: > > cleanup: LOAD DATA replication support in IO_CACHE > > remove some 14-year old code that added support for > LOAD DATA replication to IO_CACHE: > * three callbacks, of which only two were actually used and that > were only needed for LOAD DATA replication but were > tested in every IO_CACHE instance > * an additional opaque void * argument in IO_CACHE, also only > used for LOAD DATA replication, but present everywhere > * the code to close IO_CACHE prematurely in LOAD DATA to have > these callbacks called in the correct order and a long > comment explaining what will happen if IO_CACHE is not > closed prematurely > * a variable to track whether IO_CACHE was closed prematurely > (to avoid double-closing it) > > this also fixes a bug with replication of LOAD DATA LOCAL > (IO_CACHE of READ_NET type was not calling these callbacks at all) I couldn't track this down. That is GET/my_b_get/pre_read was called in both cases as well as end_io_cache/pre_close. Could you elaborate? ...skip... > diff --git a/sql/sql_load.cc b/sql/sql_load.cc > index 18982c5..62b6e17 100644 > --- a/sql/sql_load.cc > +++ b/sql/sql_load.cc > @@ -1410,20 +1372,15 @@ READ_INFO::READ_INFO(File file_par, uint tot_length, CHARSET_INFO *cs, > } > else > { > - /* > - init_io_cache() will not initialize read_function member > - if the cache is READ_NET. So we work around the problem with a > - manual assignment > - */ > - need_end_io_cache = 1; > - > #ifndef EMBEDDED_LIBRARY > if (get_it_from_net) > cache.read_function = _my_b_net_read; > > if (mysql_bin_log.is_open()) > - cache.pre_read = cache.pre_close = > - (IO_CACHE_CALLBACK) log_loaded_block; > + { > + cache.real_read_function= cache.read_function; > + cache.read_function= log_loaded_block; > + } > #endif > } > } Will this compile with replication disabled? OTOH I guess it didn't compile even before: log_loaded_block() seem to be unavailable when replication is disabled. IOCACHE::read_function may change, e.g. on error in _my_b_async_read(). Shouldn't we handle this? > @@ -1432,8 +1389,7 @@ READ_INFO::READ_INFO(File file_par, uint tot_length, CHARSET_INFO *cs, > > READ_INFO::~READ_INFO() > { > - if (need_end_io_cache) > - ::end_io_cache(&cache); > + ::end_io_cache(&cache); > my_free(buffer); > List_iterator<XML_TAG> xmlit(taglist); > XML_TAG *t; Hmm... I may miss something, but shouldn't we call log_loaded_block() for last successfully read block? Thanks, Sergey

2 2

Re: [Maria-developers] [Commits] 1cd9319: myisam/aria: don't mess with IO_CACHE::file
by Sergei Golubchik 28 May '15

28 May '15

Hi, Sergey! On May 27, Sergey Vojtovich wrote: > Hi Sergei, > > I'm lost now. In Aria you did change very similar code and kept > init_io_cache(-1) while init_io_cache(info->dfile) was moved down > and replaced with init_io_cache(new_file). This sounds reasonable. > FWICS MyISAM has very same code. > > Isn't "temp" in fact info->dfile and not -1? Ok, the original problem. init_io_cache(-1) means that IO_CACHE uses a temporary file as a backing store. That is, when in-memory buffer is full, IO_CACHE creates a temporary file to keep the data. MyISAM in a couple of places was creating IO_CACHE with init_io_cache(-1) and later was replacing IO_CACHE::file. That is, IO_CACHE was created as if it was a temporary storage, but later changed to use a real file. Which is not a good idea now, as I (in a later changeset) detect temporary IO_CACHE's in init_io_cache. So, I've fixed MyISAM/Aria not to access internal IO_CACHE fields and create IO_CACHE to have the correct file descriptior from the start. Regards, Sergei

2 1

Re: [Maria-developers] [Commits] 1cd9319: myisam/aria: don't mess with IO_CACHE::file
by Sergei Golubchik 27 May '15

27 May '15

Hi, Sergey! On May 27, Sergey Vojtovich wrote: > Hi Sergei, > > looks good. Ok to push, a few minor suggestions inline. > > > diff --git a/storage/maria/ma_check.c b/storage/maria/ma_check.c > > index 8c012d2..d50528e 100644 > > --- a/storage/maria/ma_check.c > > +++ b/storage/maria/ma_check.c > > @@ -4267,20 +4267,22 @@ int maria_repair_parallel(HA_CHECK *param, register MARIA_HA *info, > > > > if (!(sort_info.key_block= > > alloc_key_blocks(param, (uint) param->sort_key_blocks, > > - share->base.max_key_block_length)) || > > - init_io_cache(&param->read_cache, info->dfile.file, > > + share->base.max_key_block_length))) > > + goto err; > > + > > + if (init_io_cache(&param->read_cache, info->dfile.file, > > (uint) param->read_buffer_length, > > - READ_CACHE, share->pack.header_length, 1, MYF(MY_WME)) || > > - (!rep_quick && > > - (init_io_cache(&info->rec_cache, info->dfile.file, > > - (uint) param->write_buffer_length, > > - WRITE_CACHE, new_header_length, 1, > > - MYF(MY_WME | MY_WAIT_IF_FULL) & param->myf_rw) || > > - init_io_cache(&new_data_cache, -1, > > - (uint) param->write_buffer_length, > > - READ_CACHE, new_header_length, 1, > > - MYF(MY_WME | MY_DONT_CHECK_FILESIZE))))) > > + READ_CACHE, share->pack.header_length, 1, MYF(MY_WME))) > > goto err; > > + > > + if (!rep_quick) > > + { > > + if (init_io_cache(&new_data_cache, -1, > > + (uint) param->write_buffer_length, > > + READ_CACHE, new_header_length, 1, > > + MYF(MY_WME | MY_DONT_CHECK_FILESIZE))) > > + goto err; > > + } > Why not to move new_data_cache initialization down along with rec_cache? I've simply kept it where it was, the goal was to avoid io_cache.file=new_file_descriptor; for example, there was info->rec_cache.file=new_file; and I moved info->rec_cache initialization down where new_file was known and could be passed as an argument to init_io_cache(). There was no such problem with new_data_cache. Regards, Sergei

2 2

[Maria-developers] step7 (cleanup) MDEV-7950 Item_func::type() takes 0.26% in OLTP RO
by Alexander Barkov 27 May '15

27 May '15

Hi Sergey, please review the next step for MDEV-7950. It breaks get_mm_parts() into a virtual method in Item, so - replaces one virtual call item->type() to another virtual call, item->get_mm_tree(). - and also removes this virtual call for functype(), which used to distinguish between COND_AND_FUNC and COND_OR_FUNC: >- if (((Item_cond*) cond)->functype() == Item_func::COND_AND_FUNC) The next patches will: 1. Split the code in Item::get_mm_tree() responsible for functions into methods in Item_func and its descendants: Item_func Item_func_like, to get rid of the select_optimize() call Item_func_between Item_func_in Item_equal 2. Remove virtual method Item_func::select_optimize(), as well as enum Item_func::optimize_type, because they won't be needed any more. Thanks.

2 3

[Maria-developers] Memory barrier problem in InnoDB/XtraDB mutex implementation causing hangs
by Kristian Nielsen 27 May '15

27 May '15

I have been debugging some test failures seen in Buildbot: https://mariadb.atlassian.net/browse/MDEV-7026 After some debugging, this is what I think is going on. I will refer to MariaDB 5.5 sources, though the problem is also in 10.0. InnoDB/XtraDB mutexes are taken in mutex_enter_func() and unlocked in mutex_exit_func(). Simplifying for clarity, here are the relevant parts of the logic: ENTER: # First spin wait, hoping to get the mutex without yielding. os_compare_and_swap_ulint(&mutex->waiters, 0, 1); if (os_atomic_test_and_set_byte(&mutex->lock_word, 1)) sync_array_wait_event() RELEASE: os_atomic_lock_release_byte(&mutex->lock_word); os_rmb; waiters = *(volatile *)&mutex->waiters; if (waiters) mutex_signal_object(mutex); The interesting point here is how to ensure that we do not lose the wakeup of a waiting thread when the holding thread releases the mutex. The idea is that the waiting thread first sets mutex->waiters=1. Then it checks if the mutex is free - if not, it goes to sleep. The thread that releases the lock first marks the mutex free. Then it checks if there are any waiters, and if so wakes them up. For this to work, we need to ensure that the setting of waiters=1 has completed before the load of lock_word can start - a full memory barrier. Likewise, we need the setting of lock_word to 0 to complete before the load of mutex->waiters - again a full memory barrier. Otherwise we can get something like this: ENTER RELEASE Start store to mutex->waiters Start store to lock_word Load mutex->lock_word Store to lock_word becomes visible Load mutex->waiters Store to waiters becomes visible Check lock word==1 Check waiters==0, skip wakeup Sleep (forever) Or this: ENTER RELEASE Start store to lock_word Start store to mutex->waiters Load mutex->waiters Store to waiters becomes visible Load mutex->lock_word Store to lock_word becomes visible Check lock word==1 Sleep (forever) Check waiters==0, skip wakeup (So both full barriers are needed, the one in ENTER to prevent the first example, the one in RELEASE to prevent the second). It looks like for x86, the full barriers were broken in MariaDB 5.5.40 and 10.0.13. os_compare_and_swap_ulint() is defined as either __sync_bool_compare_and_swap() or atomic_cas_ulong(), both of which implies a full barrier. So ENTER looks ok. os_atomic_lock_release_byte() however is now defined as __sync_lock_release(). This is _not_ a full barrier, just a release barrier. So the RELEASE case seems to be incorrect, allowing the second deadlock example to occur. Previously, RELEASE used os_atomic_test_and_set_byte(), which is __sync_lock_test_and_set(). This is defined in GCC docs as an "aquire" barrier only, which is also wrong, obviously. But it happens to generate an XCHG instruction on x86, which is a full barrier. This is probably why it works on x86 (and probably why it did not work on PowerPC). Here is a small test program, and the assembly from GCC 5.7 on amd64: void test(unsigned *p, unsigned *q) { while (__sync_lock_test_and_set(q, 1)) { } *p++; __sync_lock_release(q); } test: movl $1, %edx .L2: movl %edx, %eax xchgl (%rsi), %eax testl %eax, %eax jne .L2 movl $0, (%rsi) ret As can be seen, the __sync_lock_test_and_set() generates a full barrier, but the __sync_lock_release() generates nothing (as this is sufficient for release semantics for the rather strong memory order on x86). So the InnoDB mutexes look completely broken from MariaDB 5.5.40 and 10.0.13 :-( This is consistent with the observations in MDEV-7026. A thread is waiting for wakeup on a mutex that is already marked free. Apparently this is not a new problem. This comment in mutex_exit_func() makes me want to cry: /* A problem: we assume that mutex_reset_lock word is a memory barrier, that is when we read the waiters field next, the read must be serialized in memory after the reset. A speculative processor might perform the read first, which could leave a waiting thread hanging indefinitely. Our current solution call every second sync_arr_wake_threads_if_sema_free() to wake up possible hanging threads if they are missed in mutex_signal_object. */ So someone found the mutex implementation broken, but instead of fixing it, they implemented a horrible workaround to try and break any deadlocks that might occur. (The reason that the deadlocks are not broken in the MDEV-7026 seems to be that it occurs during startup, before starting the srv_error_monitor_thread() that does the sync_arr_wake_threads_if_sema_free()). Now, what is also interesting is that this seems to explain very nicely the many server lockups we have seen for users that upgrade to recent MariaDB versions. I was doing a very detailed investigation of one such case where the user was able to supply unusually detailed information. I found a thread that was stuck waiting on a mutex, but no other threads seemed to be holding that mutex. This is exactly what would be seen in case of a lost wakeup due to this issue. Furthermore, the sync_arr_wake_threads_if_sema_free() mechanism, as well as the normal InnoDB "long semaphore wait", was itself blocked on that same mutex wait, which explains why they were not effective for the user in that particular case. So it looks like we need to revert the incorrect patch and release new 5.5.40a and 10.0.15 releases ASAP, to solve these hangs for users. And do a proper patch for PowerPC with proper full memory barriers. (Incidentally, the os_rmb in RELEASE seems to also be part of some PowerPC patch, it also looks completely wrong). However, more importantly, this issue clearly shows some fundamental problems in our development procedures that we need to fix ASAP. Just look at it. We have users (and customers!) that suffer from strange hangs. This is their production systems that are down. Support and developers are unable to help them due to the difficulty of debugging rare race conditions in production systems. And all the time the bug was staring us in the face, easily reproducable in our own Buildbot! But we deliberately chose to ignore those failures. The only thing that matters is "when can you push", and getting more releases out. Never mind what kind of crap is then pushed out to users. I am deeply ashamed of treating our users like this. And extremely unhappy. I even wrote about this (the importance of not ignoring Buildbot failures) just a few weeks ago. This was completely ignored, I got zero replies, and the weekly calls continued with mindless juggling of Jira numbers and buggy releases. As a result, our users suffer. We need to stop this. Please? - Kristian.

4 16