- developers - lists.mariadb.org

[Maria-developers] Windows: git config autocrlf
by Alexander Barkov 26 Nov '14

26 Nov '14

Hi, What would be the correct value for the autocrlf setting in git on Windows? Git suggests autocrlf=true by default at installation time. But then mysql-test do not work with this value. What is a better choice between the other two: "false" or "input"? Thanks.

1 0

[Maria-developers] doubt about read uncommited
by Roberto Spadim 25 Nov '14

25 Nov '14

hi guys, i use read uncommited sometimes with innodb, that's nice and work but now, i'm using a myisam table, and a aria table.... does read uncommited work with this kind of engine? i tested and table stay 'waiting table lock' while a looooong update occurs i don't know what's the internal diference but is possible to "easily" implement read uncommited to myisam/aria? -- Roberto Spadim SPAEmpresarial Eng. Automação e Controle

4 10

[Maria-developers] Please review: MDEV-7199 Introduce the concept of data type handlers
by Alexander Barkov 25 Nov '14

25 Nov '14

Hi Sergei, while working on addressing your review suggestions for "MDEV-4912 Add a plugin to field types (column types)", I decided to create a separate tasks for a piece of additional changes: https://mariadb.atlassian.net/browse/MDEV-7199 MDEV-7199 Introduce the concept of data type handlers Please review. Thanks.

1 0

Re: [Maria-developers] [Commits] Rev 4507: MDEV-7157 plugins.server_audit fails sporadically in buildbot. in file:///home/hf/wmar/mdev-7157/
by Kristian Nielsen 24 Nov '14

24 Nov '14

<holyfoot(a)askmonty.org> writes: > ------------------------------------------------------------ > revno: 4507 > revision-id: holyfoot(a)askmonty.org-20141123225345-p2xd7yrklzee10u1 > parent: sergii(a)pisem.net-20141121192039-d0lv6cj96kg5pw02 > committer: Alexey Botchkov <holyfoot(a)askmonty.org> > branch nick: mdev-7157 > timestamp: Mon 2014-11-24 02:53:45 +0400 > message: > MDEV-7157 plugins.server_audit fails sporadically in buildbot. > Records can get to the different place in the log when multiple thread > are logged. So the delay added to let the record be saved on the same > place. > === modified file 'mysql-test/suite/plugins/t/server_audit.test' > --- a/mysql-test/suite/plugins/t/server_audit.test 2014-03-26 19:58:27 +0000 > +++ b/mysql-test/suite/plugins/t/server_audit.test 2014-11-23 22:53:45 +0000 > @@ -43,6 +43,7 @@ show variables like 'server_audit%'; > set global server_audit_mode=1; > set global server_audit_events=''; > create database sa_db; > +--sleep 2 The correct way to do this is to use DEBUG_SYNC... - Kristian.

1 0

Re: [Maria-developers] [Commits] Rev 3879: MDEV-6593 : domain_id based replication filters in lp:~maria-captains/maria/maria-10.0-galera
by Kristian Nielsen 24 Nov '14

24 Nov '14

Nirbhay Choubey <nirbhay(a)skysql.com> writes: > ------------------------------------------------------------ > revno: 3879 > revision-id: nirbhay(a)skysql.com-20140912181622-s33e7r0vtgpju95q > parent: nirbhay(a)skysql.com-20140814224304-rtkeal7p9lk06li1 > committer: Nirbhay Choubey <nirbhay(a)skysql.com> > branch nick: maria-10.0-galera_6593 > timestamp: Fri 2014-09-12 14:16:22 -0400 > message: > MDEV-6593 : domain_id based replication filters > > Added support for IGNORE_DOMAIN_IDS and DO_DOMAIN_IDS. Here is my review. Some general comments, followed by some detailed comments in-line in the patch: 1. Did you prepare any documentation? You could either add it to the knowledgebase directly, or write up a text file and ask Daniel to add it in relevant places. 2. Did you run the full test suite? I would have expected at least some tests in the multi_source suite to need modification. The chance touches enough things that you should test it in a feature tree in Buildbot to ensure that it is green before pushing it to a main tree. 3. I think some extra test cases are needed: - You should test (and document) what happens when the same domain_id is in both the DO and the IGNORE list. Is it actually ever sensible to have both DO_DOMAIN_IDS and IGNORE_DOMAIN_IDS ? If not, I think it should be an error to try. - You should also add a test using non-GTID mode. - You should also add a test where parallel replication is used (--slave-parallel-threads > 0). - Add a test for the following scenario when using GTID mode: Some domain id is ignored, some events are received from the master and ignored. Then the slave is stopped, and the ignore rule is removed. What happens when the slave is restarted and reconnects? I think the previously ignored domain id events will now be re-fetched and replicated. I guess that's ok, but it should be documented. Also test what happens when binlog files on the master have been rotated and purged, so that the events are no longer available on the master (an error should occur). - Add a similar test for when not using GTID mode, the last event replicated by the slave is ignored, the slave is stopped, the ignore rule removed and the slave restarted; will the last event now be re-fetched or not (I think not). - Add a test that checks that seconds_behind_master is properly updated to zero even when the last event is ignored due to DO_DOMAIN_IDS/IGNORE_DOMAIN_IDS. It can perhaps be a bit tricky to test, but I think you can use this existing test as a basis: mysql-test/suite/rpl/t/rpl_parallel2.test - It would also be good to check that the code works when used on an old master (which does not send GTID events). You can do this by creating a binlog file with a 5.5 server and storing it in mysql-test/std_data/, and then installing it in a master; there should be several other test cases that already do this. 4. Also see detailed comments for some possible problems with the implementation. The most serious is probably to ensure that events are not skipped after the end of the group, we need a couple of tests for this, see below. Also, there is a small memory leak, see below. > +DO_DOMAIN_IDS (before) : 1 > +IGNORE_DOMAIN_IDS (before) : 2 > +CHANGE MASTER TO DO_DOMAIN_IDS=(4,4,5,1,7,7,7,1,1,2,6,8,1,4,5,5,9,3), MASTER_USE_GTID=current_pos; So if a domain_id is specified multiple times, the redundant ones will be ignored. It would seem preferable to give an error... but as far as I can see the behaviour is the same for IGNORE_SERVER_IDS, so better to be consistent as you did in the code. Just be sure to add this to the documentation. > === added file 'mysql-test/suite/rpl/t/rpl_domain_id_filter.test' > --- a/mysql-test/suite/rpl/t/rpl_domain_id_filter.test 1970-01-01 00:00:00 +0000 > +++ b/mysql-test/suite/rpl/t/rpl_domain_id_filter.test 2014-09-12 18:16:22 +0000 > +SET @gtid_domain_id_saved = @@session.gtid_domain_id; This is not needed, nor do you use it anywhere, you can remove it. > +##### Case 3: When both DO_DOMAIN_IDS and IGNORE_DOMAIN_IDS are non-empty > + > +source include/stop_slave.inc; > +let $do_domain_ids_before= query_get_value(SHOW SLAVE STATUS, Replicate_Do_Domain_Ids, 1); > +let $ignore_domain_ids_before= query_get_value(SHOW SLAVE STATUS, Replicate_Ignore_Domain_Ids, 1); > +--echo DO_DOMAIN_IDS (before) : $do_domain_ids_before > +--echo IGNORE_DOMAIN_IDS (before) : $ignore_domain_ids_before > + > +# Replicate events belonging to "domain_id 1". (DO_DOMAIN_IDS should win!) > +CHANGE MASTER TO DO_DOMAIN_IDS=(1), IGNORE_DOMAIN_IDS=(2), MASTER_USE_GTID=current_pos; Did you intend here to test overlapping ids in DO_DOMAIN_IDS and IGNORE_DOMAIN_IDS (due to the comment "DO_DOMAIN_IDS should win") ? But if I understand correctly, IGNORE_DOMAIN_IDS will be ignored if DO_DOMAIN_IDS is non-empty. So I think it should be an error to try to set both - do you agree? > +--source include/rpl_end.inc Please add additional testcases, as suggested at the start of review. > === modified file 'sql/rpl_mi.cc' > --- a/sql/rpl_mi.cc 2014-02-19 10:05:15 +0000 > +++ b/sql/rpl_mi.cc 2014-09-12 18:16:22 +0000 > @@ -513,6 +513,25 @@ > else > mi->using_gtid= Master_info::USE_GTID_NO; > } > + > + // DO_DOMAIN_IDS > + if (0 == strncmp(buf, STRING_WITH_LEN("do_domain_ids=")) && > + mi->domain_id_filter.init_ids(buf + sizeof("do_domain_ids"), > + Domain_id_filter::DO_DOMAIN_IDS)) > + { > + sql_print_error("Failed to initialize master info do_domain_ids"); > + goto errwithmsg; > + } > + > + // IGNORE_DOMAIN_IDS > + if (0 == strncmp(buf, STRING_WITH_LEN("ignore_domain_ids=")) && > + mi->domain_id_filter.init_ids(buf + sizeof("ignore_domain_ids"), > + Domain_id_filter::IGNORE_DOMAIN_IDS)) > + { > + sql_print_error("Failed to initialize master info " > + "ignore_domain_ids"); > + goto errwithmsg; > + } So here you use the same style as adjacent code (that I wrote). However, I have since learned that the style (0 == strncmp(...)) is not used in the server. So please change this, as well as my existing code, to use either strncmp(...) == 0 or !strncmp(), as you prefer. Another thing from my old code is the sizeof("do_domain_ids"), which highly confused me for a bit before I realised that while the '=' is not included in the sizeof(), the terminating '\0' _is_ included. So since even I myself is confused by this code, better clarify it ;-) Perhaps it would be clearer if changed to (sizeof("do_domain_ids=")-1) (both in my old code and in your new code). (Normally it's good to follow the style of existing code as you did, I just noticed this in the review and realised that my own "existing code" would benefit from these small improvements). > @@ -634,6 +653,16 @@ > } > } > > + char* do_domain_ids_buf= > + mi->domain_id_filter.as_string(Domain_id_filter::DO_DOMAIN_IDS); > + if (do_domain_ids_buf == NULL) > + DBUG_RETURN(1); > + > + char* ignore_domain_ids_buf= > + mi->domain_id_filter.as_string(Domain_id_filter::IGNORE_DOMAIN_IDS); > + if (ignore_domain_ids_buf == NULL) > + DBUG_RETURN(1); You have a memory leak here, do_domain_ids_buf will not be freed if ignore_domain_ids_buf fails due to out-of-memory. > @@ -1348,4 +1382,218 @@ > DBUG_RETURN(result); > } > > +/* ctor */ > +Domain_id_filter::Domain_id_filter() : m_filter(false) > +{ > + for (int i= DO_DOMAIN_IDS; i <= IGNORE_DOMAIN_IDS; i ++) > + { > + my_init_dynamic_array(&m_domain_ids[i], sizeof(uint32), 16, 16, MYF(0)); > + } > +} > + > +/* dtor */ > +Domain_id_filter::~Domain_id_filter() I don't think comments like /* ctor */ and /* dtor */ are useful, better just omit them. > +/** > + Update the domain id list of the given type with elements from the new list. > + > + @param new_ids [IN] new list of ids > + @param type [IN] domain id list type to update > + @param changed [IN] has the domain id list changed in the > + last CHANGE MASTER command? > + > + @retval void > +*/ > +void Domain_id_filter::update_ids(DYNAMIC_ARRAY *new_ids, enum_list_type type, > + bool changed) > +{ > + DYNAMIC_ARRAY *ids= &m_domain_ids[type]; > + > + if (changed == true) > + reset_dynamic(ids); > + > + /* bsearch requires an ordered list. */ > + sort_dynamic(new_ids, change_master_domain_id_cmp); > + > + for (uint i= 0; i < new_ids->elements; i++) > + { > + uint32 domain_id; > + get_dynamic(new_ids, (void *) &domain_id, i); > + > + if (bsearch((const uint32 *) &domain_id, ids->buffer, ids->elements, > + sizeof(uint32), change_master_domain_id_cmp) == NULL) > + { > + insert_dynamic(ids, (uint32*) &domain_id); > + } > + } > + return; > +} Ok, so I understand that this code is modelled after the existing code for IGNORED_SERVER_IDS. But I found it rather confusing, because it looks like there is a bug: if changed==false, then the code does not ensure that the resulting array will still be sorted. I guess the bug is not possible to trigger, because the list of stuff to add will only be non-empty if changed==true? But that is rather confusing. Please simplify the logic; maybe you only need to call this function when changed==true, so the parameter can be omitted and the array always reset? Also, consider if you can rewrite the existing code for IGNORE_SERVER_IDS so that the same common code can be used as for IGNORE_DOMAIN_IDS and DO_DOMAIN_IDS, that would simplify the code and reduce risk of bugs or inconsistencies. > +/** > + Initialize the given domain id list with the numbers from the specified > + buffer. > + > + @param buf [IN] buffer > + @param type [IN] domain id list type > + > + @retval false Success > + true Error > +*/ > +bool Domain_id_filter::init_ids(char *buf, enum_list_type type) > +{ > + char *token, *last; > + uint num_items; > + DYNAMIC_ARRAY *ids= &m_domain_ids[type]; > + > + token= strtok_r(buf, " ", &last); > + if (token == NULL) > + { > + return true; /* error */ > + } > + > + num_items= atoi(token); > + for (uint i= 0; i < num_items; i++) > + { > + token= strtok_r(NULL, " ", &last); > + if (token == NULL) > + { > + return true; /* error */ > + } > + else > + { > + uint32 val= atoi(token); > + insert_dynamic(ids, (uchar *) &val); > + } > + } > + return false; > +} Again, did you consider using the same code for this and for IGNORE_SERVER_IDS? (I think you did, because you added a comment to init_dynarray_intvar_from_file ;-). Maybe consider it again; it is fine to rewrite/remove the existing code for IGNORE_SERVER_IDS and replace it with your new code. Again, it would reduce risk for bugs or inconsistencies. > + /* getter */ > + bool get_filter() { return m_filter; } > + > + /* setter */ > + void set_filter(uint32 domain_id); These are not really getter and setters, I think, at least I find those names confusing. Maybe something like do_filter(uint32 domain_id) and is_group_filtered() ? And expand the comments, so that they explain that these functions respectively check a GTID for needing to be filtered, and return whether the current group is being filtered. Also, personally I dislike setter/getter, and would prefer just to access the m_filter as a public member. But I suppose that's style and up to you. > + /* > + Initialize the given domain id list with the numbers from the specified > + buffer. > + > + @param buf [IN] buffer > + @param type [IN] domain id list type > + > + @retval false Success > + true Error > + */ > + bool init_ids(char *buf, enum_list_type type); Clarify in the comment that the list is initialized from a string of space-separated numbers, the first of which is the number of following entries. > @@ -182,6 +279,7 @@ > bool flush_relay_log_cache, > bool need_lock_relay_log); > int change_master_server_id_cmp(ulong *id1, ulong *id2); > +int change_master_domain_id_cmp(uint32 *id1, uint32 *id2); I think change_master_domain_id_cmp() is only used in rpl_mi.cc, so should be static there, and not refered here in rpl_mi.h > === modified file 'sql/slave.cc' > --- a/sql/slave.cc 2014-08-12 03:55:41 +0000 > +++ b/sql/slave.cc 2014-09-12 18:16:22 +0000 > @@ -1259,6 +1259,7 @@ > going to ignore (to not log them in the relay log). > Items being read are supposed to be decimal output of values of a > type shorter or equal of @c long and separated by the single space. > + It also used to restore DO_DOMAIN_IDS & IGNORE_DOMAIN_IDS lists. This comment suggests you wanted to share code, then later changed your mind. If you end up keeping the code separate, you still need to delete this comment. (And there is a typo: "It also" -> "It is also", but if you remove it anyway...). > @@ -5601,6 +5610,10 @@ > mi->last_queued_gtid= event_gtid; > mi->last_queued_gtid_standalone= > (gtid_flag & Gtid_log_event::FL_STANDALONE) != 0; > + > + /* Should filter all the subsequent events in the current GTID group? */ > + mi->domain_id_filter.set_filter(event_gtid.domain_id); So, here we will set m_filter to true, if we want to skip the following group. But I don't see any code that will set it to false at the end of the group? That seems a problem, as we could then end up wrongly skipping all kinds of other events, such as Gtid_list, Incident, ... Also note that it is possible for a GTID group to not be terminated by COMMIT/XID. This can happen at least if the master crashes in the middle of writing a group; the slave will receive only the first part of it, followed by a master restart Format_description event. That case needs to be handled, and you should add a test case for it. Also it is possible for eg. a master downgrade to introduce following event groups that do not start with GTID - seems these would also be wrongly skipped? So we also need a test case for this... Hope this helps, - Kristian.

2 8

[Maria-developers] The variable "table_definition_cache" is autosized by default on MySQL 5.6.8+ but not on MariaDB 10
by Jean Weisbuch 20 Nov '14

20 Nov '14

The value of the variable "table_definition_cache" is by default autosized (based on the table_open_cache value) on MySQL >=5.6.8 while it is still static (400) on the latest MariaDB 10.0.14 and 10.1.?, is that by choice or this feature hasn't been backported and should be? From the MySQL doc http://dev.mysql.com/doc/refman/5.6/en/server-system-variables.html#sysvar_… : *Permitted Values (>= 5.6.8)* *Type* |numeric| *Default* |-1 (autosized)| *Min Value* |400| *Max Value* |524288| The default value is based on the following formula, capped to a limit of 2000: 400 + (table_open_cache / 2) Before MySQL 5.6.8, the default is 400.

1 0

Re: [Maria-developers] [Commits] Rev 4367: MDEV-7026: Race in InnoDB/XtraDB mutex implementation can stall or hang the server. in http://bazaar.launchpad.net/~maria-captains/maria/5.5
by Sergey Vojtovich 20 Nov '14

20 Nov '14

Hi Kristian, thanks for taking over this bug! I believe this is the patch that was pushed into 5.5. Unfortunately I think there is still one potential for a hang left (details below). :( Since InnoDB and XtraDB changes are identical I'll comment only XtraDB changes. So you removed os_isync and implemented os_mb instead. That's good! You also removed a bunch of os_wmb/os_rmb calls. That's good too. And you kept RELEASE barrier for mutex_exit() on PPC64. Very good! I will also comment things that I don't really like, but they're just for your information (no action required). On Wed, Nov 19, 2014 at 01:56:46PM +0100, knielsen(a)knielsen-hq.org wrote: > At http://bazaar.launchpad.net/~maria-captains/maria/5.5 > > ------------------------------------------------------------ > revno: 4367 > revision-id: knielsen(a)knielsen-hq.org-20141119125646-njq02h7irmu9s71b > parent: sergii(a)pisem.net-20141118231952-fk3o69075dn3ravv > committer: Kristian Nielsen <knielsen(a)knielsen-hq.org> > branch nick: mariadb-5.5 > timestamp: Wed 2014-11-19 13:56:46 +0100 > message: > MDEV-7026: Race in InnoDB/XtraDB mutex implementation can stall or hang the server. > > The bug was that full memory barrier was missing in the code that ensures that > a waiter on an InnoDB mutex will not go to sleep unless it is guaranteed to be > woken up again by another thread currently holding the mutex. This made > possible a race where a thread could get stuck waiting for a mutex that is in > fact no longer locked. If that thread was also holding other critical locks, > this could stall the entire server. There is an error monitor thread than can > break the stall, it runs about once per second. But if the error monitor > thread itself got stuck or was not running, then the entire server could hang > infinitely. > > This was introduced on i386/amd64 platforms in 5.5.40 and 10.0.13 by an > incorrect patch that tried to fix the similar problem for PowerPC. JFYI: Strictly speaking that "incorrect patch" attempted to fix different problem (read carefully): it added RELEASE barrier between "stores inside mutex" and "lock_word= 0" while this patch adds full barrier between "lock_word= 0" and "if (waiters)". > > This commit reverts the incorrect PowerPC patch, and instead implements a fix > for PowerPC that does not change i386/amd64 behaviour, making PowerPC work > similarly to i386/amd64. ...skip... > === modified file 'storage/xtradb/include/os0sync.h' > --- a/storage/xtradb/include/os0sync.h 2014-09-08 15:10:48 +0000 > +++ b/storage/xtradb/include/os0sync.h 2014-11-19 12:56:46 +0000 > @@ -314,11 +331,28 @@ amount of increment. */ > /**********************************************************//** > Returns the old value of *ptr, atomically sets *ptr to new_val */ > > -# define os_atomic_test_and_set_byte(ptr, new_val) \ > +#ifdef __powerpc__ > +/* > + os_atomic_test_and_set_byte_release() should imply a release barrier before > + setting, and a full barrier after. But __sync_lock_test_and_set() is only > + documented as an aquire barrier. So on PowerPC we need to add the full > + barrier explicitly. */ > +# define os_atomic_test_and_set_byte_release(ptr, new_val) \ > + do { __sync_lock_release(ptr); \ > + __sync_synchronize(); } while (0) > +#else > +/* > + On x86, __sync_lock_test_and_set() happens to be full barrier, due to > + LOCK prefix. > +*/ > +# define os_atomic_test_and_set_byte_release(ptr, new_val) \ > + __sync_lock_test_and_set(ptr, (byte) new_val) > +#endif JFYI: "os_atomic_test_and_set_byte_release" sounds odd to me. Look at gcc manual: https://gcc.gnu.org/onlinedocs/gcc-4.1.2/gcc/Atomic-Builtins.html ... This builtin is not a full barrier, but rather an acquire barrier. This means that references after the builtin cannot move to (or be speculated to) before the builtin, but previous memory stores may not be globally visible yet, and previous memory loads may not yet be satisfied. ... You name RELEASE something that is acqually ACQUIRE. So why didn't you like "os_atomic_lock_release_byte" which is RELASE btw: ... This builtin is not a full barrier, but rather a release barrier. This means that all previous memory stores are globally visible, and all previous memory loads have been satisfied, but following memory reads are not prevented from being speculated to before the barrier. ... > +/* > + os_atomic_test_and_set_byte_acquire() is a full memory barrier on x86. But > + in general, just an aquire barrier should be sufficient. */ > +# define os_atomic_test_and_set_byte_acquire(ptr, new_val) \ > __sync_lock_test_and_set(ptr, (byte) new_val) !!! The problem is here!!! Strictly speaking not here, but in mutex_spin_wait(): ... mutex_set_waiters(mutex, 1); /* Try to reserve still a few times */ for (i = 0; i < 4; i++) { if (mutex_test_and_set(mutex) == 0) { ... We'll get stuck if "waiters= 1" will become visible after "if (mutex_test_and_set(mutex))". I don't see anything preventing this reorder. ...skip... > === modified file 'storage/xtradb/include/sync0sync.ic' > --- a/storage/xtradb/include/sync0sync.ic 2014-08-29 12:02:46 +0000 > +++ b/storage/xtradb/include/sync0sync.ic 2014-11-19 12:56:46 +0000 > @@ -80,11 +80,11 @@ mutex_test_and_set( > mutex_t* mutex) /*!< in: mutex */ > { > #if defined(HAVE_ATOMIC_BUILTINS) > - return(os_atomic_test_and_set_byte(&mutex->lock_word, 1)); > + return(os_atomic_test_and_set_byte_acquire(&mutex->lock_word, 1)); > #else > ibool ret; > > - ret = os_fast_mutex_trylock(&(mutex->os_fast_mutex)); > + ret = os_fast_mutex_trylock_full_barrier(&(mutex->os_fast_mutex)); > > if (ret == 0) { > /* We check that os_fast_mutex_trylock does not leak JFYI: This is probably the most funny thing. Here we're in the "#else" branch of "#if defined(HAVE_ATOMIC_BUILTINS)". E.g. if we compile with gcc and enter this branch it means: sync builtins are not avialble (gcc atomic builtins are very likely not available too). Now you say "os_fast_mutex_trylock_full_barrier" which does os_mb. But since neither sync bultins nor atomic builtins are available, os_mb is basically no-op. This may be different on other platforms, but to my knowledge memory barriers and atomic operations normally come together. Another thing: did you try compiling with this branch enabled? IIRC it didn't compile even berfore we started fixing it. ...skip... Regards, Sergey

2 2

[Maria-developers] constant propagation & LIKE
by Alexander Barkov 20 Nov '14

20 Nov '14

Hello Sergei, The attached patch fixes MDEV-7149 Constant propagation erroneously applied for LIKE All tests pass. But I'm not 100% sure about the patch. Is eq_cmp_result() proper place to fix this? If it's the right place, I can test the collation properties and return: - COND_TRUE for simple collations with one-character-to-one-weight rules, - COND_OK for the complex collations with tricky character to weight mapping. Thanks.

1 0

Re: [Maria-developers] [Commits] Rev 4358: MDEV-7113 difference between check_vcol_func_processor and check_partition_func_processor in lp:~maria-captains/maria/5.5
by Sergey Petrunia 20 Nov '14

20 Nov '14

Hi Sergei, * what about persistent virtual columns? Do we still support environment-dependent virtual functions for those? * the behavior of current solution is not deterministic: thread1> select @@time_zone; +-------------+ | @@time_zone | +-------------+ | SYSTEM | +-------------+ 1 row in set (0.00 sec) thread1> create table t2 ( a varchar(32), b int, v varchar(100) as (from_unixtime(b))); Query OK, 0 rows affected (0.28 sec) thread1> insert into t2 (a,b) values ('local-tz-23-11', unix_timestamp()); Query OK, 1 row affected (0.04 sec) thread1> select * from t2; +----------------+------------+---------------------+ | a | b | v | +----------------+------------+---------------------+ | local-tz-23-11 | 1416427891 | 2014-11-19 23:11:31 | +----------------+------------+---------------------+ 1 row in set (0.00 sec) thread2> set time_zone='Europe/Helsinki'; Query OK, 0 rows affected (0.00 sec) thread2> select * from t2; +----------------+------------+---------------------+ | a | b | v | +----------------+------------+---------------------+ | local-tz-23-11 | 1416427891 | 2014-11-19 23:11:31 | +----------------+------------+---------------------+ 1 row in set (0.00 sec) thread2> flush tables; Query OK, 0 rows affected (0.00 sec) thread2> select * from t2; +----------------+------------+---------------------+ | a | b | v | +----------------+------------+---------------------+ | local-tz-23-11 | 1416427891 | 2014-11-19 22:11:31 | +----------------+------------+---------------------+ 1 row in set (0.00 sec) Is this really ok? Maybe, it makes sense to use some some "default" (e.g. global variables) environment on the non-deterministic virtual column functions? Or, make these functions always fetch the environment from the current thread? On Mon, Nov 17, 2014 at 09:43:46AM +0100, Sergei Golubchik wrote: > At lp:~maria-captains/maria/5.5 > > ------------------------------------------------------------ > revno: 4358 > revision-id: sergii(a)pisem.net-20141115155753-rx5twf6utu1isczh > parent: sergii(a)pisem.net-20141113173729-hirl67og6at3xbni > fixes bugs: https://mariadb.atlassian.net/browse/MDEV-6789 https://mariadb.atlassian.net/browse/MDEV-7113 > committer: Sergei Golubchik <sergii(a)pisem.net> > branch nick: 5.5 > timestamp: Sat 2014-11-15 16:57:53 +0100 > message: > MDEV-7113 difference between check_vcol_func_processor and check_partition_func_processor > MDEV-6789 segfault in Item_func_from_unixtime::get_date on updating table with virtual columns > > * prohibit VALUES in partitioning expression > * prohibit user and system variables in virtual column expressions > * fix Item_func_date_format to cache locale (for %M/%W to return the same as MONTHNAME/DAYNAME) > * fix Item_func_from_unixtime to cache time_zone directly, not THD (and not to crash) > * added tests for other incorrectly allowed (in vcols) functions to see that they don't crash ... BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

2 2

Re: [Maria-developers] [Commits] Rev 4346: MDEV-7026 - Occasional hang during startup on Power8 in lp:maria/5.5
by Kristian Nielsen 19 Nov '14

19 Nov '14

Sergey Vojtovich <svoj(a)mariadb.org> writes: > revno: 4346 > revision-id: svoj(a)mariadb.org-20141118080742-sdf93kgsgxulenvb > parent: knielsen(a)knielsen-hq.org-20141112101013-t63ayylsk08ncgs3 > committer: Sergey Vojtovich <svoj(a)mariadb.org> > branch nick: 5.5 > timestamp: Tue 2014-11-18 12:07:42 +0400 > message: > MDEV-7026 - Occasional hang during startup on Power8 Right, so you requested second review ASAP, I'm trying to understand what's going on here. If my understanding is correct, this patch goes on top of current 5.5.40, not on top of the patch I reviewed previously. So there are two cases, depending on whether INNODB_RW_LOCKS_USE_ATOMICS is set or not. It is my understanding that this is "usually" set, eg. on our common x86 and powerpc linux binaries. The critical thing for the patch at hand is the memory barrier between mutex_set_waiters() and mutex_test_and_set() in the waiter, and mutex_reset_lock_word() and mutex_get_waiters() in the thread releasing the lock. A full memory barrier is needed in the waiter, to be sure that mutex_set_waiters() becomes visible to the releasing thread before mutex_test_and_set() runs. And a full barrier is needed in the releasing thread, to be sure that mutex_reset_lock_word() becomes visible before mutex_get_waiters() runs. So let's see how this is ensured. First the case of x86 with INNODB_RW_LOCKS_USE_ATOMICS set, HAVE_ATOMIC_BUILTINS, pre-5.5.40: - mutex_set_waiters() uses os_compare_and_swap_ulint() which is __sync_bool_compare_and_swap() which is full barrier. - mutex_reset_lock_word() uses os_atomic_test_and_set_byte() which is __sync_lock_test_and_set(). This is documented only as a release barrier, but is in fact a full barrier on x86 with current GCC. So in this case full barrier is there as needed. If HAVE_ATOMIC_BUILTINS is not set, then mutex_reset_lock_word() uses os_fast_mutex_unlock() which is pthread_mutex_unlock(), which is also full barrier on x86. If INNODB_RW_LOCKS_USE_ATOMICS is not set, then mutex_set_waiters() has no barriers. But then mutex_test_and_set() instead has a full barrier on x86. Either in os_atomic_test_and_set_byte() which is __sync_lock_test_and_set(), if HAVE_ATOMIC_BUILTINS is set, or in os_fast_mutex_trylock() otherwise. Both of these are full barriers on x86. So this is why things work ok on x86, apparently. Very bad code that seriously needs cleanup and proper documentation of memory barrier semantics. But it is what we have to work with in 5.5. So to make things work on PowerPC without risk of breaking x86, we could do this: - Make sure os_atomic_test_and_set_byte() is full barrier on PowerPC: #ifdef __powerpc__ /* os_atomic_test_and_set_byte() should imply a full barrier. But __sync_lock_test_and_set() is only documented as an aquire barrier. So on PowerPC we need to add the full barrier explicitly. */ # define os_atomic_test_and_set_byte(ptr, new_val) \ do { __sync_lock_test_and_set(ptr, (byte) new_val); \ __atomic_thread_fence(__ATOMIC_SEQ_CST); } while(0) #else /* On x86, __sync_lock_test_and_set() happens to be full barrier, due to LOCK prefix. */ # define os_atomic_test_and_set_byte(ptr, new_val) \ __sync_lock_test_and_set(ptr, (byte) new_val) #endif - Make sure that mutex_reset_lock_word() is a full barrier when HAVE_ATOMIC_BUILTINS is not set, by adding a memory barrier: os_fast_mutex_unlock(&(mutex->os_fast_mutex)); #ifdef __powerpc__ /* On PowerPC, pthread_mutex does not imply full barrier, so we need to add it explicitly. */ __atomic_thread_fence(__ATOMIC_SEQ_CST); #endif - For the case of neither HAVE_ATOMIC_BUILTINS nor INNODB_RW_LOCKS_USE_ATOMICS set, similarly add a full barrier in mutex_test_and_set() for PowerPC: #ifdef __powerpc__ /* On PowerPC, pthread_mutex does not imply full barrier, so we need to add it explicitly. */ __atomic_thread_fence(__ATOMIC_SEQ_CST); #endif ret = os_fast_mutex_trylock(&(mutex->os_fast_mutex)); Then in 10.1. if someone is brave enough, we can clean this up properly. Clearly define what the memory barrier semantics is, and implement it using the correct primitives that work as needed on all platforms. So now that I understand (hopefully...) how the code used to work, and how it can be fixed for PowerPC without changing existing code, let me look at the new patch ... > === modified file 'storage/innobase/include/os0sync.h' > --- a/storage/innobase/include/os0sync.h 2014-09-08 15:10:48 +0000 > +++ b/storage/innobase/include/os0sync.h 2014-11-18 08:07:42 +0000 > @@ -310,8 +310,16 @@ Returns the old value of *ptr, atomicall > # define os_atomic_test_and_set_byte(ptr, new_val) \ > __sync_lock_test_and_set(ptr, (byte) new_val) > > +#ifdef __powerpc__ > # define os_atomic_lock_release_byte(ptr) \ > __sync_lock_release(ptr) > +#else > +/* In theory __sync_lock_release should be used to release the lock. > +Unfortunately, it does not work properly alone. The workaround is > +that more conservative __sync_lock_test_and_set is used instead. */ > +# define os_atomic_lock_release_byte(ptr) \ > + __sync_lock_test_and_set(ptr, 0) > +#endif This is very confusing. You define a method with full barrier semantics, but call it something suggesting release semantics. Why not revert the original change so that mutex_reset_lock_word() uses os_atomic_test_and_set_byte()? And then fix the implementation of os_atomic_test_and_set_byte() to be full barrier on PowerPC, as I suggested above? The term "atomic" generally implies full barrier in the MySQL source code... > #elif defined(HAVE_IB_SOLARIS_ATOMICS) > > @@ -428,7 +436,7 @@ clobbered */ > # define os_rmb __atomic_thread_fence(__ATOMIC_ACQUIRE) > # define os_wmb __atomic_thread_fence(__ATOMIC_RELEASE) > #ifdef __powerpc__ > -# define os_isync __asm __volatile ("isync":::"memory") > +# define os_isync __atomic_thread_fence(__ATOMIC_SEQ_CST) > #else > #define os_isync do { } while(0) > #endif As Jonas suggested, don't call a full memory barrier "isync". And do not introduce something that is a barrier on one platform but a no-operation on another platform. But ... > === modified file 'storage/innobase/sync/sync0sync.c' > --- a/storage/innobase/sync/sync0sync.c 2014-11-03 13:43:44 +0000 > +++ b/storage/innobase/sync/sync0sync.c 2014-11-18 08:07:42 +0000 > @@ -474,10 +474,9 @@ mutex_set_waiters( > > ptr = &(mutex->waiters); > > - os_wmb; > - > *ptr = n; /* Here we assume that the write of a single > word in memory is atomic */ > + os_isync; > } With this, the semantics of how things work becomes even more complicated. Now on PowerPC the barrier is in one function, but on x86 the barrier is in another function. Why not instead do as I suggested above? Put in full barriers for PowerPC in the same places that they are in x86? ---- Maybe I should try to say if I think the patch fixes the actual bug, in case you don't want to go with my suggestions and instead keep your approach? Since it goes back to using __sync_lock_test_and_set() on x86, and since it has a full barrier in mutex_exit_func() on PowerPC, I think perhaps it does. But it is really hard to understand exactly what is going on, as you can see above I had to do a lot of reasoning to understand even why the old code worked... My advice is still to revert the original incorrect PowerPC patch and do something that is at least consistent with the existing code... (And then feel free to do a proper cleanup in 10.1). Hope this helps, - Kristian.

2 2