- developers - lists.mariadb.org

[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

Re: [Maria-developers] Rotating from encry
by Jonas Oreland 22 May '15

22 May '15

8043 I tried using a search engine and googled for "jira mariadb innodb encryption jonas" :) /Jonas On Thu, May 21, 2015 at 4:36 PM, Jan Lindström <jan.lindstrom(a)mariadb.com> wrote: > Hi, > > Can you give the MDEV, I do not seem to be able to find it. > > R: Jan > > On Thu, May 21, 2015 at 2:51 PM, Jonas Oreland <jonaso(a)google.com> wrote: > >> i made a JIRA entry with latest innodb encryption... >> >> /Jonas >> >> On Thu, May 21, 2015 at 12:30 PM, Jan Lindström < >> jan.lindstrom(a)mariadb.com> wrote: >> >>> Hi, >>> >>> On Thu, May 21, 2015 at 11:33 AM, Jonas Oreland <jonaso(a)google.com> >>> wrote: >>> >>>> Hi Jan, >>>> >>>> 1) rotating encrypted => unencrypted is definitely supported, >>>> in my latest version the TODO is removed...(hope you have a recent >>>> enough version) >>>> >>> >>> Based on this, I do not have the latest version. Where, I could get >>> that, it does not seem to be on branch? >>> >>>> >>>> >>>> 2) Thanks! for testcase, it did indeed reveal a bug with the "encrypted >>>> => unencrypted => encrypted" sequence >>>> >>>> attaching fix. >>>> >>>> Let me know it fixes your version of testcase (i've used a modified >>>> version since e.g we don't have per table settings...) >>>> >>> >>> Yes, this fix is good. Thanks. >>> >>> R: Jan >>> >> >> >

1 0

[Maria-developers] How do I read information_schema.global_status not using query.
by kyung nam Kim 22 May '15

22 May '15

Hi, I hava a trouble with getting a data from informaition_schema.global_status table not using query. I wrote below code and it's possible to get data from some table in performance_schema . ** reader.cc =============================================================================================== my_thread_init(); ... MEM_ROOT mem_field; READ_RECORD read_record_info; tables.init_one_table(STRING_WITH_LEN("performance_schema"), STRING_WITH_LEN("events_statements_current"), bool result = open_and_lock_tables(ctx->thd, &tables, FALSE, MYSQL_LOCK_IGNORE_TIMEOUT); if (result) { return false; } if(table = tables.table) /* if only table exists */ { init_sql_alloc(&mem_field, 1024, 0, MYF(0)); ... while (!(error= read_record_info.read_record(&read_record_info))) { char* value = get_field(&mem_field, table->field[0]); // It's OK. .... } } ... ==================================================================================== It's possible to read data from performance_schema tables using low-level function . But I tried to read data from information_schema.global_status table as same way(just changed the name of database and table) , an error occurred in open_and_lock_tables(). Is there a way to read data from information_schema.global_status not using query? Regards, Kim kyungnam.

1 0

Re: [Maria-developers] Rotating from encry
by Jonas Oreland 21 May '15

21 May '15

i made a JIRA entry with latest innodb encryption... /Jonas On Thu, May 21, 2015 at 12:30 PM, Jan Lindström <jan.lindstrom(a)mariadb.com> wrote: > Hi, > > On Thu, May 21, 2015 at 11:33 AM, Jonas Oreland <jonaso(a)google.com> wrote: > >> Hi Jan, >> >> 1) rotating encrypted => unencrypted is definitely supported, >> in my latest version the TODO is removed...(hope you have a recent enough >> version) >> > > Based on this, I do not have the latest version. Where, I could get that, > it does not seem to be on branch? > >> >> >> 2) Thanks! for testcase, it did indeed reveal a bug with the "encrypted >> => unencrypted => encrypted" sequence >> >> attaching fix. >> >> Let me know it fixes your version of testcase (i've used a modified >> version since e.g we don't have per table settings...) >> > > Yes, this fix is good. Thanks. > > R: Jan >

1 0

Re: [Maria-developers] Rotating from encry
by Jonas Oreland 21 May '15

21 May '15

Hi Jan, 1) rotating encrypted => unencrypted is definitely supported, in my latest version the TODO is removed...(hope you have a recent enough version) 2) Thanks! for testcase, it did indeed reveal a bug with the "encrypted => unencrypted => encrypted" sequence attaching fix. Let me know it fixes your version of testcase (i've used a modified version since e.g we don't have per table settings...) /Jonas On Fri, May 15, 2015 at 1:23 PM, Jan Lindström <jan.lindstrom(a)mariadb.com> wrote: > Hi, > > At fil0crypt.cc there is > > fil_crypt_needs_rotation(uint key_version, const key_state_t *key_state) > { > // TODO(jonaso): Add support for rotating encrypted => unencrypted > > if (key_version == 0 && key_state->key_version != 0) { > /* this is rotation unencrypted => encrypted > * ignore rotate_key_age */ > return true; > } > > Thus to me it is not clear is the support for rotating encrypted => > unencrypted really missing or not and furthermore, see attached test case > for this, > > encrypted + insert + grep : ok > encrypted => unencrypted + grep: ok > unencrypted => encrypted + grep: not ok > > R: Jan >

1 0

Re: [Maria-developers] [Commits] b2db8e8: MDEV-7949: Item_field::used_tables() takes 0.29% in OLTP RO
by Sergey Vojtovich 21 May '15

21 May '15

Hi Sanja, this patch looks Ok, just one minor suggestion inline. Though effect for given workload is quite moderate: Item_field::used_tables 0.21% -> 0.20% Item_basic_constant::used_tables 0.11% -> 0.11% Item_func::used_tables 0.05% -> 0.05% Item_sum::used_tables 0.00% -> 0.00% On Wed, May 13, 2015 at 04:17:26PM +0200, sanja(a)mariadb.com wrote: > revision-id: b2db8e85422a455a10cd28ef7e44977182528f70 > parent(s): b22959903b89e798f8804ec9a815c88f75915cd9 > committer: Oleksandr Byelkin > branch nick: server > timestamp: 2015-05-13 16:17:22 +0200 > message: > > MDEV-7949: Item_field::used_tables() takes 0.29% in OLTP RO > > small sixes of used_tables() usage > ...skip... > diff --git a/sql/item_func.h b/sql/item_func.h > index 0d57c2b..c48cd84 100644 > --- a/sql/item_func.h > +++ b/sql/item_func.h > @@ -1337,7 +1337,7 @@ class Item_func_sleep :public Item_int_func > const char *func_name() const { return "sleep"; } > table_map used_tables() const > { > - return Item_int_func::used_tables() | RAND_TABLE_BIT; > + return used_tables_cache | RAND_TABLE_BIT; > } > bool is_expensive() { return 1; } > longlong val_int(); > @@ -1591,7 +1591,7 @@ class Item_func_get_lock :public Item_int_func > void fix_length_and_dec() { max_length=1; maybe_null=1;} > table_map used_tables() const > { > - return Item_int_func::used_tables() | RAND_TABLE_BIT; > + return used_tables_cache | RAND_TABLE_BIT; > } > bool const_item() const { return 0; } > bool is_expensive() { return 1; } > @@ -1611,7 +1611,7 @@ class Item_func_release_lock :public Item_int_func > void fix_length_and_dec() { max_length= 1; maybe_null= 1;} > table_map used_tables() const > { > - return Item_int_func::used_tables() | RAND_TABLE_BIT; > + return used_tables_cache | RAND_TABLE_BIT; > } > bool const_item() const { return 0; } > bool is_expensive() { return 1; } > @@ -1724,7 +1724,7 @@ class Item_func_set_user_var :public Item_func > void fix_length_and_dec(); > table_map used_tables() const > { > - return Item_func::used_tables() | RAND_TABLE_BIT; > + return used_tables_cache | RAND_TABLE_BIT; > } > bool const_item() const { return 0; } > bool is_expensive() { return 1; } Was there any reason for this change? Compiler should be clever enough to inline used_tables() of base class here. Regards, Sergey

3 2

[Maria-developers] Misnamed libmariadb-dev packages on aptitude
by pcrews 20 May '15

20 May '15

Hello, Today, I was experimenting with building mariadb images and encountered this issue: E: Unable to locate package libmariadb-dev pcrews@erlking-dev:~/git/dibtest$ aptitude search libmariadb p libmariadbd-dev - MariaDB embedded database development files p libmariadbd-dev:i386 - MariaDB embedded database development files Note how these are listed as libmariadbd vs. libmariadb. I am writing to see if this can be fixed or if this might be some change in naming (which would require some updating of other tools' references, etc). Thank you, Patrick Crews

1 0

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

19 May '15

Hi Sergey, Please review the next iteration for MDEV-7950. This one splits the function add_key_fields() into a method in Item. This change removes about 3 virtual calls item->type(), as well as some virtual calls item_func->functype(), and adds one virtual call item->add_key_fields() instead. Thanks.

2 2

Re: [Maria-developers] [Commits] 8b30c0f: MDEV-7949: Item_field::used_tables() takes 0.29% in OLTP RO
by Sergey Vojtovich 19 May '15

19 May '15

Hi Sanja, There're 3 bug fixes and 4 revisions concerning these lines: https://bugs.mysql.com/bug.php?id=16716 git: 1991a87d831bb48b31989c622e35acda11df7fef git: ca22a81b1c84ce81e1e9ea2c3ace7be1848027d8 (source for all used_tables()) https://bugs.mysql.com/bug.php?id=36488 git: 66367aeea83a0cd7f583a0194651a235b63975a9 https://bugs.mysql.com/bug.php?id=46815 git: 10406ae65871de074e807e626f9ede686e9321d4 If we don't have these bugs, why not to revert all the code introduced by these patches? Or we're not affected by the need to use used_tables() as an indication of prepared statement? If so, why? Thanks, Sergey On Sun, May 17, 2015 at 03:10:48PM +0200, sanja(a)mariadb.com wrote: > revision-id: 8b30c0f26c7b35fc73429f85aa35eb3614d60bd8 > parent(s): b2db8e85422a455a10cd28ef7e44977182528f70 > committer: Oleksandr Byelkin > branch nick: server > timestamp: 2015-05-17 15:10:45 +0200 > message: > > MDEV-7949: Item_field::used_tables() takes 0.29% in OLTP RO > > Part 2: removed hack workaround for bug we do not have. > > --- > sql/item_strfunc.cc | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/sql/item_strfunc.cc b/sql/item_strfunc.cc > index 4bf8dd5..7b9169e 100644 > --- a/sql/item_strfunc.cc > +++ b/sql/item_strfunc.cc > @@ -626,8 +626,7 @@ String *Item_func_concat::val_str(String *str) > if (!(res=args[0]->val_str(str))) > goto null; > use_as_buff= &tmp_value; > - /* Item_subselect in --ps-protocol mode will state it as a non-const */ > - is_const= args[0]->const_item() || !args[0]->used_tables(); > + is_const= args[0]->const_item(); > for (i=1 ; i < arg_count ; i++) > { > if (res->length() == 0) > @@ -639,7 +638,7 @@ String *Item_func_concat::val_str(String *str) > non-empty argument. Because of this we need is_const to be > evaluated only for it. > */ > - is_const= args[i]->const_item() || !args[i]->used_tables(); > + is_const= args[i]->const_item(); > } > else > { > @@ -975,7 +974,7 @@ String *Item_func_concat_ws::val_str(String *str) > for (i=1; i < arg_count; i++) > if ((res= args[i]->val_str(str))) > { > - is_const= args[i]->const_item() || !args[i]->used_tables(); > + is_const= args[i]->const_item(); > break; > } > > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits

1 0

[Maria-developers] MDEV-7937: Enforce SSL when --ssl client option is used
by Vicențiu Ciorbaru 18 May '15

18 May '15

Hi Sergei! I've done some work on this issue. I've read MySQL's implementation of this and have looked at our implementation. They have done a bit of refactoring, introducing an enforce_ssl flag, as well as changing the C interface a bit, to allow setting this flag programatically. I've created a patch here that changes the minimum amount possible, in order to implement what MDEV-7937 requires. That being said, I agree with (most of) MySQL's refactoring in this case. They've moved all the SSL related connection code into its own separate function, before actually calling send_client_reply_packet. I can work towards implementing things the way MySQL does, but since I saw that you've actually done most of the work in that area of the code, I figured I'd ask for your input on it. There are two more things that I'm not sure of: 1. Specifying --ssl as a command line parameter to the mysql client is not enough to enforce ssl and the client's code in this case just ignores the option. We need to provide at least one of the additional ones like --ssl-key or --ssl-ca. My patch will not cause the client to report an error in this case. Is this acceptable behaviour or not? 2. Do we want mysql's enforce_ssl feature? Regards, Vicențiu

2 1