- developers - lists.mariadb.org

Re: [Maria-developers] [Commits] Rev 4484: MDEV-4255 MIXED replication: Unique key updates are not marked unsafe, replication fails in lp:~maria-captains/maria/10.0
by Kristian Nielsen 29 Nov '14

29 Nov '14

Sergei Golubchik <serg(a)mariadb.org> writes: > Kristian Nielsen, could you please review this patch? I guess my main question here is, did you think about why you want to) fix this 1) in a post-GA release, 2) at all? One problem with adding new statements as unsafe for statement-based binlogging is that it introduces new warnings in existing applications. This is especially annoying on a slave with binlogging enabled, as every unsafe statement now causes a warning in the slave error log. Thus, it can happen that a security update for a user leaves them with an error log that constantly fills up on a busy slave. It is my understanding that statement-based binlogging (as opposed to mixed or row) is still quite popular, and there have been complaints about these warnings before. Another point is that this patch seems to fix only a corner case of a bigger problem? This particular patch is about non-transactional updates. But updating a unique key can break replication just as well on transactional tables: CREATE TABLE t1 (a INT PRIMARY KEY); INSERT INTO t1 VALUES (2), (3), (4); UPDATE t1 SET a=a-1; Since we do not have deferred constraints, this UPDATE will fail or succeed depending on the order in which rows are updated. So a statement that succeeds andgets binlogged on the master can break on the slave if the rows are updated in a different order. So any reason that you fix one part of UPDATE of unique key but not the other? (I suppose the same problem can also happen with foreign key constraints, though it might take some creativity to come up with an example where an UPDATE will conflict or not with foreign keys depending on row update order). The bug has fix version 5.5, your patch seems to be against 10.0, where do you intend to push it? I would probably have chosen to fix in 10.1, if at all, but I'm not necessarily against 5.5 or 10.0, I'm mostly just asking what the reason is for whichever version was chosen? > I don't particularly like that it explicitly invokes > thd->decide_logging_format() . But there's no way around it, first time > logging format is decided in lock_tables(), basically when tables are just > opened. At that point we don't know what fields will be updated yet. I am not really familiar with how binlogging is decided on the master, have not worked with that code so far. So unfortunately I am not able to say much about whether this is a problem or not. At least the call is still before the actual update of rows starts, so I suppose it should be ok... > At lp:~maria-captains/maria/10.0 > > ------------------------------------------------------------ > revno: 4484 > revision-id: sergii(a)pisem.net-20141116234900-747qdhm9ti9xh070 > parent: sergii(a)pisem.net-20141113161325-mdz5ox7e86iyazss > fixes bug: https://mariadb.atlassian.net/browse/MDEV-4255 > committer: Sergei Golubchik <sergii(a)pisem.net> > branch nick: 10.0 > timestamp: Mon 2014-11-17 00:49:00 +0100 > message: > MDEV-4255 MIXED replication: Unique key updates are not marked unsafe, replication fails I think this needs a better description, especially since this can cause new warnings in slave error logs. Here is a suggestion, but feel free to use something else if you prefer: "MDEV-4255 MIXED replication: Unique key updates are not marked unsafe, replication fails Statement-based replication of UPDATE statements that modify (parts of) unique keys is not safe. If the order of row updates differ between master and slave, the slave can end up modifying a different set of rows before a unique key violation error than what was updated on the master. If the table is non-transactional, then the statement will be partially executed on the master and on the slave, but with different result, causing replication to diverge. This patch marks as unsafe for statement-based replication any UPDATE that modifies a unique key on a non-transactinal table. In mixed-mode replication, this will cause the query to be logged in row mode. In statement-mode replication, this will produce a warning about binlogging of unsafe statement." > === modified file 'sql/share/errmsg-utf8.txt' > --- a/sql/share/errmsg-utf8.txt 2014-11-13 16:13:25 +0000 > +++ b/sql/share/errmsg-utf8.txt 2014-11-16 23:49:00 +0000 > @@ -7111,3 +7111,5 @@ ER_SLAVE_SKIP_NOT_IN_GTID > eng "When using GTID, @@sql_slave_skip_counter can not be used. Instead, setting @@gtid_slave_pos explicitly can be used to skip to after a given GTID position." > ER_TABLE_DEFINITION_TOO_BIG > eng "The definition for table %`s is too big" > +ER_BINLOG_UNSAFE_UPDATE_UNIQUE_NONTRANS > + eng "The statement is unsafe because it updates a UNIQUE key colums in a non-transactional table. This is unsafe because in the case of a conflict a non-predictable part of the table will stay updated." (Adding a new error code in a GA release requires some care, especially if fix-version:5.5 is intended, but I'm sure you're better aware of this than most). > === modified file 'sql/sql_lex.h' > --- a/sql/sql_lex.h 2014-08-07 16:06:56 +0000 > +++ b/sql/sql_lex.h 2014-11-16 23:49:00 +0000 > @@ -1319,6 +1319,12 @@ class Query_tables_list > */ > BINLOG_STMT_UNSAFE_AUTOINC_NOT_FIRST, > > + /** > + INSERT into auto-inc field which is not the first part of composed > + primary key. > + */ > + BINLOG_STMT_UPDATE_UNIQUE_NONTRANS, > + Copy-paste error. Here, you copied the comment of the earlier entry, you surely intended to re-write the comment to describe the newly added entry. Hope this helps, - Kristian.

3 7

Re: [Maria-developers] [Commits] 953dd8d: MDEV-6997 MDEV-6996 MDEV-7002 MDEV-7006 MDEV-7009 MDEV-7012 MDEV-7015 MDEV-6977
by Sergei Golubchik 29 Nov '14

29 Nov '14

Hi, sanja! On Nov 05, sanja(a)mariadb.com wrote: > revision-id: 953dd8d87fea224e26d11452eb458c79ba098f4a > parent(s): d6981391eb8f904135dfdedb5b90a7592b35e648 > committer: Oleksandr Byelkin > branch nick: work-maria-10.1-sysvar4 > timestamp: 2014-11-05 11:05:01 +0100 > message: > > MDEV-6997 MDEV-6996 MDEV-7002 MDEV-7006 MDEV-7009 MDEV-7012 MDEV-7015 MDEV-6977 I'd prefer a slightly more verbose commit comment. And don't forget - first line is the subject. For example: Use NO_SET_STMT flag for more variables Fixes MDEV-6997 MDEV-6996 MDEV-7002 MDEV-7006 MDEV-7009 MDEV-7012 MDEV-7015 MDEV-6977 I won't comment on individual variables here, as you're simply disabling them all. See my comments in Jira. Regards, Sergei

1 0

[Maria-developers] MDEV-7004 review
by Sergei Golubchik 28 Nov '14

28 Nov '14

Hi, Sergey I didn't review MDEV-7200 (InnoDB) and patches that are already in 10.1. Almost everything else was fine, see few comments below. > ------------------------------------------------------------ > revno: 4513 > committer: Sergey Vojtovich <svoj(a)mariadb.org> > branch nick: 10.0-power > timestamp: Fri 2014-11-07 22:03:18 +0400 > message: > Multi-rwlock lock for table definition cache hash. > added: > modified: > diff: > === modified file 'sql/table_cache.cc' > --- sql/table_cache.cc 2014-10-21 12:20:39 +0000 > +++ sql/table_cache.cc 2014-11-07 18:03:18 +0000 > @@ -53,6 +53,77 @@ > #include "table.h" > #include "sql_base.h" > > + > +/** > + Table definition hash lock. > + > + Lock consists of multiple rw-locks (8 by default). Lock is write locked > + when all instances are write locked. Lock is read locked when any instance > + is read locked. > + > + This class was implemented to workaround scalability bottleneck of table > + definition hash rw-lock. > + > + To be replaced with lock-free hash when we understand how to solve the > + following problems: > + - lock-free hash iterator (can be workarounded by introducing global > + all_tables list); I don't particularly like that solution. Could you try to switch to lock-free hash now? I'll try to do the iterator asap > + - lf_hash_search() may fail because of OOM. It is rather complex to handle > + it properly. > +*/ > + > +class TDC_lock > +{ > + uint m_instances; > + bool m_write_locked; > + char pad[128]; > + struct st_locks > + { > + mysql_rwlock_t lock; > + char pad[128]; In these cases I usually do: pad[128 - sizeof(mysql_rwlock_t)]; > + } *locks; > +public: > + int init(PSI_rwlock_key key, uint instances) > + { > + m_instances= instances; > + m_write_locked= false; > + locks= (st_locks *) my_malloc(sizeof(struct st_locks) * m_instances, MYF(MY_WME)); > + if (!locks) > + return 1; > + for (uint i= 0; i < m_instances; i++) > + mysql_rwlock_init(key, &locks[i].lock); > + return 0; > + } > + void destroy(void) > + { > + for (uint i= 0; i < m_instances; i++) > + mysql_rwlock_destroy(&locks[i].lock); > + my_free(locks); > + } > + void wrlock(void) > + { > + for (uint i= 0; i < m_instances; i++) > + mysql_rwlock_wrlock(&locks[i].lock); > + m_write_locked= true; > + } > + void rdlock(THD *thd= 0) > + { > + mysql_rwlock_rdlock(&locks[thd ? thd->thread_id % m_instances : 0].lock); > + } > + void unlock(THD *thd= 0) > + { > + if (m_write_locked) > + { > + m_write_locked= false; > + for (uint i= 0; i < m_instances; i++) > + mysql_rwlock_unlock(&locks[i].lock); > + } > + else > + mysql_rwlock_unlock(&locks[thd ? thd->thread_id % m_instances : 0].lock); > + } > +}; > + > + > /** Configuration. */ > ulong tdc_size; /**< Table definition cache threshold for LRU eviction. */ > ulong tc_size; /**< Table cache threshold for LRU eviction. */ > ------------------------------------------------------------ > revno: 4512 > committer: Sergey Vojtovich <svoj(a)mariadb.org> > branch nick: 10.0-power > timestamp: Fri 2014-10-31 15:44:36 +0400 > message: > Preallocate locks on THD mem_root to avoid expensive malloc. > modified: > diff: > === modified file 'sql/lock.cc' > --- sql/lock.cc 2014-09-30 17:31:14 +0000 > +++ sql/lock.cc 2014-10-31 11:44:36 +0000 > @@ -700,7 +699,7 @@ MYSQL_LOCK *get_lock_data(THD *thd, TABL > TABLE **to, **table_buf; > DBUG_ENTER("get_lock_data"); > > - DBUG_ASSERT((flags == GET_LOCK_UNLOCK) || (flags == GET_LOCK_STORE_LOCKS)); > + DBUG_ASSERT((flags & GET_LOCK_UNLOCK) || (flags & GET_LOCK_STORE_LOCKS)); Not the same, old assert also verified that these flags aren't set both. New assert doesn't. But I don't think it matters much. > DBUG_PRINT("info", ("count %d", count)); > > for (i=lock_count=table_count=0 ; i < count ; i++) > ------------------------------------------------------------ > revno: 4508 > committer: Sergey Vojtovich <svoj(a)mariadb.org> > branch nick: 10.0 > timestamp: Tue 2014-10-21 16:20:39 +0400 > message: > modified: > diff: > === modified file 'sql/handler.cc' > --- sql/handler.cc 2014-11-13 10:01:31 +0000 > +++ sql/handler.cc 2014-10-21 12:20:39 +0000 > @@ -410,13 +410,16 @@ static int hton_ext_based_table_discover > static void update_discovery_counters(handlerton *hton, int val) > { > if (hton->discover_table_existence == full_discover_for_existence) > - my_atomic_add32(&need_full_discover_for_existence, val); > + my_atomic_add32_explicit(&need_full_discover_for_existence, val, > + MY_MEMORY_ORDER_RELAXED); > > if (hton->discover_table_names) > - my_atomic_add32(&engines_with_discover_table_names, val); > + my_atomic_add32_explicit(&engines_with_discover_table_names, val, > + MY_MEMORY_ORDER_RELAXED); > > if (hton->discover_table) > - my_atomic_add32(&engines_with_discover, val); > + my_atomic_add32_explicit(&engines_with_discover, val, > + MY_MEMORY_ORDER_RELAXED); I wouldn't do that. Technically there's a dependency between these counters and loaded engines, so the order, kind of, matters. I don't think there is any practical risk of bugs here, but, on the other hand, these counters are only updated when an engine is loaded or unloaded, so there's no need to bother with the relaxed memory ordering here. > } > > int ha_finalize_handlerton(st_plugin_int *plugin) > > === modified file 'sql/mysqld.cc' > --- sql/mysqld.cc 2014-11-18 21:25:47 +0000 > +++ sql/mysqld.cc 2014-10-21 12:20:39 +0000 > @@ -3884,7 +3884,7 @@ static void my_malloc_size_cb_func(long > } > // workaround for gcc 4.2.4-1ubuntu4 -fPIE (from DEB_BUILD_HARDENING=1) > int64 volatile * volatile ptr=&global_status_var.memory_used; > - my_atomic_add64(ptr, size); > + my_atomic_add64_explicit(ptr, size, MY_MEMORY_ORDER_RELAXED); good! > } > } > > > === modified file 'sql/mysqld.h' > --- sql/mysqld.h 2014-11-13 08:56:28 +0000 > +++ sql/mysqld.h 2014-10-21 12:20:39 +0000 > @@ -629,7 +629,7 @@ inline __attribute__((warn_unused_result > { > query_id_t id; > my_atomic_rwlock_wrlock(&global_query_id_lock); > - id= my_atomic_add64(&global_query_id, 1); > + id= my_atomic_add64_explicit(&global_query_id, 1, MY_MEMORY_ORDER_RELAXED); I believe this is fine > my_atomic_rwlock_wrunlock(&global_query_id_lock); > return (id); > } > === modified file 'sql/table_cache.cc' > --- sql/table_cache.cc 2014-06-10 18:20:33 +0000 > +++ sql/table_cache.cc 2014-10-21 12:20:39 +0000 > @@ -139,7 +139,7 @@ uint tc_records(void) > { > uint count; > my_atomic_rwlock_rdlock(&LOCK_tdc_atomics); > - count= my_atomic_load32(&tc_count); > + count= my_atomic_load32_explicit(&tc_count, MY_MEMORY_ORDER_RELAXED); I'm not sure about this file. You should know better whether it's safe to use relaxed memory ordering here. > my_atomic_rwlock_rdunlock(&LOCK_tdc_atomics); > return count; > } Regards, Sergei

2 5

[Maria-developers] add a rotate event at begin of relay log make greate help
by nanyi607rao 28 Nov '14

28 Nov '14

Hi kristian, currently, a relaylog is immediately purged after it be completely replayed (relay_log_purge=1). and if IO thread crashes, we should delete all relay logs and fetch master binlog again from exec_master_log_pos/file (relay_log_purge=ON). I think that make a greap waste of disk IO especially to cloud disk. The critical problem is that there is no method to know the original binlog filename on master of a event in relaylog. Actually we can know a relaylog event's original binlog offset on master, but we can't know the original binlog filename. so why not forcely add a rotate event or any other new type event which contained the original master binlog filename at begin of a relay log. as I know two different master binlogs' events wouldn't contained in one relay log, so the original binlog filename of a relaylog's events is only one. If we want to know a relaylog event's original binlog filename/position, we can get original filename from the begin of relaylog, and get original binlog offset from event's "end log pos". oppositely, if we know a event's binlog filename/end positon on master, wo also can quickly find it in slave's relay log. That can make many benefits, firstly no need to delete all relay logs and fetch master binlogs again when IO thread crashed, because we can get exact last read_master_log_filename/position from last relay log. secondly, there is no need to use GTID in 1 vs n replication failover Scenario (Gtid must set log_slave_updates=ON in mysql 5.6, which increase disk IO load), if master crashes, other slaves can get lost events for the newest slave's relay log, as long as relay log don't be purged, then promotes the newest slave to master. Thanks. 2014-11-26 nanyi607rao

3 3

[Maria-developers] MDEV-6431 review
by Sergey Vojtovich 27 Nov '14

27 Nov '14

Hi Sergei, overall it looks good. I want to double check the clean-up stuff, since I didn't understand everything at the first glance. Some comments inline. > diff --git a/include/mysql/plugin_password_validation.h b/include/mysql/plugin_password_validation.h > new file mode 100644 > index 0000000..0bbb8f4 > --- /dev/null > +++ b/include/mysql/plugin_password_validation.h > @@ -0,0 +1,45 @@ > +/** > + Password validation plugin descriptor > +*/ > +struct st_mysql_password_validation > +{ > + int interface_version; /**< version plugin uses */ > + /** > + Function provided by the plugin which should perform password validation > + and return 0 if the password has passed the validation. > + */ > + int (*validate_password)(MYSQL_LEX_STRING *username, > + MYSQL_LEX_STRING *password); > +}; > +#endif > + This differs from MySQL API, so we can't reuse their plugins. Also they have this cool get_password_strength method, but that's something we can expose via UDF. OTOH we can compare password against username, and that's quite common password misuse. And what I always hated in this validation stuff is that you'll never come up with a password that makes it happy. Of course unless you know requirements. In our case requirements are exposed via system variables. This is more or less acceptable. But I believe it would be nice if this stuff would be able to generate strong passwords. Again, that's something that can be exposed as UDF. > diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc > index 529a795..cdf8c7b 100644 > --- a/sql/sql_plugin.cc > +++ b/sql/sql_plugin.cc > @@ -1051,7 +1055,8 @@ static bool plugin_add(MEM_ROOT *tmp_root, > continue; // invalid plugin type > > if (plugin->type == MYSQL_UDF_PLUGIN || > - (plugin->type == 8 && tmp.plugin_dl->mariaversion == 0)) > + (plugin->type == MariaDB_PASSWORD_VALIDATION_INTERFACE_VERSION && > + tmp.plugin_dl->mariaversion == 0)) > continue; // unsupported plugin type > > if (name->str && my_strnncoll(system_charset_info, You compare type against version here. Why? Regards, Sergey

2 1

Re: [Maria-developers] [Commits] Rev 4369: MDEV-7148 - Recurring: InnoDB: Failing assertion: !lock->recursive in lp:maria/5.5
by Kristian Nielsen 27 Nov '14

27 Nov '14

Sergey Vojtovich <svoj(a)mariadb.org> writes: > revno: 4369 > revision-id: svoj(a)mariadb.org-20141120094823-fozdsrm1kzv46kxi > parent: jplindst(a)mariadb.org-20141119182734-cwbaed0ka90ocj5e > committer: Sergey Vojtovich <svoj(a)mariadb.org> > branch nick: 5.5 > timestamp: Thu 2014-11-20 13:48:23 +0400 > message: > MDEV-7148 - Recurring: InnoDB: Failing assertion: !lock->recursive > > On PPC64 high-loaded server may crash due to assertion failure in InnoDB > rwlocks code. > > This happened because load order between "recursive" and "writer_thread" > wasn't properly enforced. Looks ok. I failed to understand why the patch would fix the assertion mentioned in the bug report. But the patch itself looks correct. - Kristian.

3 3

Re: [Maria-developers] [Commits] Rev 3902: MDEV-7123: MariaDB 10.0.14 Galera node shutdown with signal 11 in lp:~maria-captains/maria/maria-10.0-galera
by Kristian Nielsen 26 Nov '14

26 Nov '14

Nirbhay Choubey <nirbhay(a)mariadb.com> writes: > revno: 3902 > revision-id: nirbhay(a)mariadb.com-20141123163533-9ov1yirj26cur301 > parent: nirbhay(a)mariadb.com-20141119183337-n5spskf54beqbhif > committer: Nirbhay Choubey <nirbhay(a)mariadb.com> > branch nick: maria-10.0-galera > timestamp: Sun 2014-11-23 11:35:33 -0500 > message: > MDEV-7123: MariaDB 10.0.14 Galera node shutdown with signal 11 > > wsrep-patch uses same connection name for constructing Master_info > objects. As a result all existing wsrep Master_info objects refer > to same rpl_filter object. This could lead to race when multiple > threads try to destruct/delete Master_info object, as they would all > try to delete the same relay_log object. > Fixed by adding a mutex to protect the critical section. The patch is ok. But please consider the following points, and act on them or not as you see fit: 1. You are introducing a mutex lock/unlock in a general data structure used in a couple of places (looks like keycaches and rpl_filters?). Did you consider the performance impact of this? When will the additional locking/unlocking take place? If it only happens in less often used operations (like re-configuring replication filters or key caches?), then extra locking is not an issue. If it happens often, like once per query or something like that, it would probably be best to find a solution with less performance impact. (mutex lock and unlock are quite expensive even if not contended). 2. Consider using explicit mysql_mutex_lock() and mysql_mutex_unlock() calls rather than introducing extra lock()/unlock() methods. The extra methods are needless abtractions that make it harder to understand exactly what is happening wrt. locking. 3. Did you consider if a test case could be made for the originally reported bug? We used to have a rule that any bug fix must include a test case that reproduces the bug fixed (ie. the test case must fail before the patch). Not a hard rule (a few bugs can be extremely hard to write mtr testcases for), but a good rule nevertheless. But maybe this rule has been dropped, now that the focus is on quantity of Jira tickes closed rather than quality. - Kristian.

1 0

[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