November 2014 - 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

[Maria-developers] Syntax for parallel replication
by Kristian Nielsen 18 Dec '14

18 Dec '14

Hi Serg, Can you help me with suggestions/comments for the following proposal for how to do configuration for MDEV-6676, speculative parallel replication? I am not too happy about how things are in 10.0. There, there is a single option --slave-parallel-threads=N. If N>0, then parallel replication is enabled, else not. One problem is that this makes it not configurable per multi-source master connection. Another problem is that there are two possible mechanisms for parallelisation, group-commit based and domain_id based; currently, one can enable none of them or both, but not just one of them. MDEV-6676 will introduce at least one other mechanism, which seems to make it essential to make a better way to configure this. Now, ideally, there would not be any configuration at all. The server would just run things in parallel when possible and to the degree desirable. However, I think there are some reasons that we need to allow the user this configurability: - Parallel replication is still a somewhat experimental feature, so it seems too risky to enable it by default. Also, it doesn't really seem possible for the server to automatically set the best number of threads to use, with current implementation (or possibly any implementation). - When replicating with non-transactional updates, or in non-gtid mode, slave state is not crash safe. This is true in non-parallel replication also, but in parallel replication, the problem seems amplified, as there may be multiple transactions in progress at the time of a crash, complicating possible manual recovery. This also suggests that parallel replication must be configurable. - When using domain-based parallel replication, the user is responsible for ensuring that independent domains are non-conflicting and can be replicated out-of-order wrt. each other. So if replication domains are used, but this property is not guaranteed, then domain-based parallel replication need to be configurable, or parallel replication cannot be used at all. - The new speculative replication feature in MDEV-6676 is not always guaranteed to be a win - in some workloads, where there are many conflicts between successive transactions, excessive rollback could cause it to be less efficient than not using it. Again, this suggests it needs to be configurable. So given this, I came up with the following idea for syntax: CHANGE MASTER TO PARALLEL_MODE=(domain,groupcommit,transactional,waiting) Each of the four keywords in the parenthesis is optional. "domain" enables domain-based parallelisation, where each replication domain is treated independently. "groupcommit" enables the non-speculative mode, where only transactions that group-committed together on the master are applied in parallel on the slave. "transactional" enables the speculative mode, where all transactional DML is optimistically tried in parallel, and then in case of conflict a rollback and retry is done. "groupcommit" and "transactional" are mutually exclusive, at most one of them can be specified. The default would be (domain,groupcommit) to be back-wards compatible with 10.0. If slave_parallel_thread=0, then no parallel apply will happen even if PARALLEL_MODE is non-empty. If slave_parallel_thread>0 but PARALLEL_MODE is empty (PARALLEL_MODE=()), then again no parallel apply will be done. The "waiting" option is not essential to add, we could remove it, I put it in because there were already a number of options so it seemed to cause no harm. The idea is that on the master, we detect if transaction T2 had to do a row lock wait on transaction T1. If so, it seems likely that a similar conflict could occur on the slave, so we will not run T2 in parallel with T1; instead we will let T2 wait for T1 to commit before T2 is started. The "waiting" option could be enabled by a user to disable this check, enabling even more aggressive parallelisation. I am not sure if it is worth it to have this configurable though, comments welcome. I have not checked how hard it will be to implement the new syntax. We do not have any similar multi-option CHANGE MASTER elements, as far as I know, but it is similar to ENUM system variables. And we already have a IGNORE_SERVER_IDS syntax with comma-separated list within parenthesis. So hopefully not too hard to do, and somewhat consistent with existing syntax. So what do you think? Is it a reasonable syntax? Any comments, or suggestions for better way to do it? Thanks, - Kristian.

5 23

[Maria-developers] Some patches for the C client library
by Daniel Schepler 02 Dec '14

02 Dec '14

While I was creating Debian packages for the "MariaDB Client Library for C" I came across several minor issues that I corrected. I thought I'd post the patches here in case you might be interested in integrating them; I'd have no problem with contributing them under the MCA if that's necessary (i.e. if they're even big enough to require such a thing). Each one has a fairly complete description of its purpose at the start. I've also noticed that mariadb_config --libs prints out something like ... -l-lz if WITH_EXTERNAL_ZLIB=ON. However, for the Debian packages I instead patched mariadb_config to remove the indirect dependencies altogether, as we generally do, so I didn't write a fix for the -l-lz issue. Also, it seems that if I try to build with WITH_SQLITE=ON then I get a build error, so I haven't enabled that in the Debian packages. -- Daniel Schepler

2 2

[Maria-developers] crash, reporting bugs
by Roberto Spadim 02 Dec '14

02 Dec '14

hi guys, i have a consistent crash (mdev-7182) but this email is not about it... when i got the crash, i got this info: ... To report this bug, see http://kb.askmonty.org/en/reporting-bugs ... should we update this url to the current one? https://mariadb.com/kb/en/mariadb/development/debugging-mariadb/reporting-b… thanks -- Roberto Spadim SPAEmpresarial Eng. Automação e Controle

3 3

Re: [Maria-developers] [Commits] 020e569: MDEV-4412 : SLOW QUERY LOG - add affected rows (UPDATE / DELETE) in slow query log
by Sergei Golubchik 30 Nov '14

30 Nov '14

Hi, Nirbhay! On Nov 29, nirbhay(a)mariadb.com wrote: > revision-id: 020e569dc7f638b6add8a043b1c9ff308f688ffa > parent(s): 7b55b67de5fdfe91283357fe6c7ccc3f9e355925 > committer: Nirbhay Choubey > branch nick: b4412 > timestamp: 2014-11-29 19:05:32 -0500 > message: > > MDEV-4412 : SLOW QUERY LOG - add affected rows (UPDATE / DELETE) in slow query log > > Added Rows_affected to slow query log & mysql.slow_log table. > > diff --git a/scripts/mysql_system_tables_fix.sql b/scripts/mysql_system_tables_fix.sql > index b9bb59c..aeaf919 100644 > --- a/scripts/mysql_system_tables_fix.sql > +++ b/scripts/mysql_system_tables_fix.sql > @@ -249,7 +249,8 @@ SET GLOBAL general_log = @old_log_state; > SET @old_log_state = @@global.slow_query_log; > SET GLOBAL slow_query_log = 'OFF'; > ALTER TABLE slow_log > - ADD COLUMN thread_id BIGINT(21) UNSIGNED NOT NULL AFTER sql_text; > + ADD COLUMN thread_id BIGINT(21) UNSIGNED NOT NULL AFTER sql_text, > + ADD COLUMN rows_affected INTEGER NOT NULL AFTER thread_id; > ALTER TABLE slow_log > MODIFY start_time TIMESTAMP(6) NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, > MODIFY user_host MEDIUMTEXT NOT NULL, I am not sure it'll work. It would work if you need to upgrade a table that has neither thread_id nor rows_affected column to the latest version. So that ALTER would've been an acceptable solution if both thread_id and rows_affected were added in the same MariaDB version. But they were not and there are quite a few MariaDB releases where slow_log table has thread_id and doesn't have rows_affected. I suspect in that case ALTER will fail. But if you'd like you can try a small test case and see whether such an ALTER will add a second column even if the first one already exists. Better add rows_affected in a separate ALTER TABLE statement. Regards, Sergei

1 0

[Maria-developers] Patch: CREATE [[NO] FORCE] VIEW Options
by James Briggs 30 Nov '14

30 Nov '14

Hi List. Below is my patch that implements the CREATE [[NO] FORCE] VIEW options against MariaDB 10.1.0. Notes: - It's a fairly solid patch, but I think the best thing is to commit it to alpha and let it bake for a while. - it passes t/view.test: # ./mysql-test-run.pl view Logging: ./mysql-test-run.pl view vardir: /usr/local/mariadb-10.1.0/mysql-test/var MariaDB Version 10.1.0-MariaDB-debug TEST RESULT TIME (ms) or COMMENT -------------------------------------------------------------------------- main.view [ pass ] 1896 -------------------------------------------------------------------------- The servers were restarted 0 times Spent 1.896 of 7 seconds executing testcases Completed: All 1 tests were successful. - I wrote tests/view.pl which does 8,000 tests. It passes. (I will include it in a separate email.) - Please take a look at the style, and the 411 comments. Thanks, James Briggs. -- Available for MySQL/NoSQL DBA/Programming in San Jose area or remote. $ cat create_force_view.patch --- ../mariadb-10.1.0/sql/sql_view.h 2014-06-27 04:50:36.000000000 -0700 +++ sql/sql_view.h 2014-09-02 02:35:42.000000000 -0700 @@ -29,10 +29,10 @@ /* Function declarations */ bool create_view_precheck(THD *thd, TABLE_LIST *tables, TABLE_LIST *view, - enum_view_create_mode mode); + enum_view_create_mode mode, enum_view_create_force force); bool mysql_create_view(THD *thd, TABLE_LIST *view, - enum_view_create_mode mode); + enum_view_create_mode mode, enum_view_create_force force); bool mysql_make_view(THD *thd, File_parser *parser, TABLE_LIST *table, uint flags); --- ../mariadb-10.1.0/sql/sql_lex.h 2014-06-27 04:50:33.000000000 -0700 +++ sql/sql_lex.h 2014-09-02 01:21:10.000000000 -0700 @@ -170,6 +170,12 @@ VIEW_CREATE_OR_REPLACE // check only that there are not such table }; +enum enum_view_create_force +{ + VIEW_CREATE_NO_FORCE, // default - check that there are not such VIEW/table + VIEW_CREATE_FORCE, // check that there are not such VIEW/table, then ignore table object dependencies +}; + enum enum_drop_mode { DROP_DEFAULT, // mode is not specified @@ -2442,6 +2448,7 @@ }; enum enum_var_type option_type; enum enum_view_create_mode create_view_mode; + enum enum_view_create_force create_view_force; enum enum_drop_mode drop_mode; uint profile_query_id; --- ../mariadb-10.1.0/sql/sql_parse.cc 2014-06-27 04:50:34.000000000 -0700 +++ sql/sql_parse.cc 2014-09-02 02:34:31.000000000 -0700 @@ -4943,7 +4943,7 @@ Note: SQLCOM_CREATE_VIEW also handles 'ALTER VIEW' commands as specified through the thd->lex->create_view_mode flag. */ - res= mysql_create_view(thd, first_table, thd->lex->create_view_mode); + res= mysql_create_view(thd, first_table, thd->lex->create_view_mode, thd->lex->create_view_force); break; } case SQLCOM_DROP_VIEW: --- ../mariadb-10.1.0/sql/sql_yacc.yy 2014-06-27 04:50:37.000000000 -0700 +++ sql/sql_yacc.yy 2014-09-05 17:19:29.000000000 -0700 @@ -1851,7 +1851,7 @@ statement sp_suid sp_c_chistics sp_a_chistics sp_chistic sp_c_chistic xa opt_field_or_var_spec fields_or_vars opt_load_data_set_spec - view_algorithm view_or_trigger_or_sp_or_event + view_algorithm view_or_trigger_or_sp_or_event view_force_option definer_tail no_definer_tail view_suid view_tail view_list_opt view_list view_select view_check_option trigger_tail sp_tail sf_tail udf_tail event_tail @@ -2446,6 +2446,7 @@ VIEW_CREATE_OR_REPLACE); Lex->create_view_algorithm= DTYPE_ALGORITHM_UNDEFINED; Lex->create_view_suid= TRUE; + Lex->create_view_force= VIEW_CREATE_NO_FORCE; /* initialize just in case */ } view_or_trigger_or_sp_or_event { @@ -15887,6 +15888,15 @@ | event_tail ; +view_force_option: + /* empty */ /* 411 - is there a cleaner way of initializing here? */ + { Lex->create_view_force = VIEW_CREATE_NO_FORCE; } + | NO_SYM FORCE_SYM + { Lex->create_view_force = VIEW_CREATE_NO_FORCE; } + | FORCE_SYM + { Lex->create_view_force = VIEW_CREATE_FORCE; } + ; + /************************************************************************** DEFINER clause support. @@ -15944,7 +15954,7 @@ ; view_tail: - view_suid VIEW_SYM table_ident + view_suid view_force_option VIEW_SYM table_ident { LEX *lex= thd->lex; lex->sql_command= SQLCOM_CREATE_VIEW; --- ../mariadb-10.1.0/sql/sql_view.cc 2014-06-27 04:50:36.000000000 -0700 +++ sql/sql_view.cc 2014-09-05 19:33:58.000000000 -0700 @@ -248,7 +248,7 @@ */ bool create_view_precheck(THD *thd, TABLE_LIST *tables, TABLE_LIST *view, - enum_view_create_mode mode) + enum_view_create_mode mode, enum_view_create_force force) { LEX *lex= thd->lex; /* first table in list is target VIEW name => cut off it */ @@ -259,7 +259,7 @@ DBUG_ENTER("create_view_precheck"); /* - Privilege check for view creation: + Privilege check for view creation with default (NO FORCE): - user has CREATE VIEW privilege on view table - user has DROP privilege in case of ALTER VIEW or CREATE OR REPLACE VIEW @@ -272,6 +272,7 @@ checked that we have not more privileges on correspondent column of view table (i.e. user will not get some privileges by view creation) */ + if ((check_access(thd, CREATE_VIEW_ACL, view->db, &view->grant.privilege, &view->grant.m_internal, @@ -285,6 +286,11 @@ check_grant(thd, DROP_ACL, view, FALSE, 1, FALSE)))) goto err; + if (force) { + res = false; + DBUG_RETURN(res || thd->is_error()); + } + for (sl= select_lex; sl; sl= sl->next_select()) { for (tbl= sl->get_table_list(); tbl; tbl= tbl->next_local) @@ -369,7 +375,7 @@ #else bool create_view_precheck(THD *thd, TABLE_LIST *tables, TABLE_LIST *view, - enum_view_create_mode mode) + enum_view_create_mode mode, enum_view_create_force force) { return FALSE; } @@ -391,7 +397,7 @@ */ bool mysql_create_view(THD *thd, TABLE_LIST *views, - enum_view_create_mode mode) + enum_view_create_mode mode, enum_view_create_force force) { LEX *lex= thd->lex; bool link_to_local; @@ -425,14 +431,13 @@ goto err; } - if ((res= create_view_precheck(thd, tables, view, mode))) + if (res= create_view_precheck(thd, tables, view, mode, force)) goto err; lex->link_first_table_back(view, link_to_local); view->open_type= OT_BASE_ONLY; - if (open_temporary_tables(thd, lex->query_tables) || - open_and_lock_tables(thd, lex->query_tables, TRUE, 0)) + if (open_temporary_tables(thd, lex->query_tables) || (!force && open_and_lock_tables(thd, lex->query_tables, TRUE, 0))) { view= lex->unlink_first_table(&link_to_local); res= TRUE; @@ -513,6 +518,7 @@ } } +if (!force) { /* prepare select to resolve all fields */ lex->context_analysis_only|= CONTEXT_ANALYSIS_ONLY_VIEW; if (unit->prepare(thd, 0, 0)) @@ -612,6 +618,7 @@ } } #endif +} res= mysql_register_view(thd, view, mode); @@ -621,7 +628,7 @@ meta-data changes after ALTER VIEW. */ - if (!res) + // if (!res) + if (!res && !force) /* 411 - solves segfault problems with CREATE FORCE VIEW option sometimes */ tdc_remove_table(thd, TDC_RT_REMOVE_ALL, view->db, view->table_name, false); if (mysql_bin_log.is_open()) @@ -908,6 +915,8 @@ fn_format(path_buff, file.str, dir.str, "", MY_UNPACK_FILENAME); path.length= strlen(path_buff); if (ha_table_exists(thd, view->db, view->table_name, NULL)) { if (mode == VIEW_CREATE_NEW) --- ../mariadb-10.1.0/mysql-test/t/view.test 2014-06-27 04:50:30.000000000 -0700 +++ mysql-test/t/view.test 2014-09-06 00:23:32.000000000 -0700 @@ -5263,4 +5263,17 @@ --echo # ----------------------------------------------------------------- --echo # -- End of 10.0 tests. --echo # ----------------------------------------------------------------- + +create no force view v1 as select 1; +drop view if exists v1; + +create force view v1 as select 1; +drop view if exists v1; + +create force view v1 as select * from missing_base_table; +drop view if exists v1; + +--echo # ----------------------------------------------------------------- +--echo # -- End of 10.1 tests. +--echo # ----------------------------------------------------------------- SET optimizer_switch=@save_optimizer_switch;

2 2

Re: [Maria-developers] MDEV-4412 : SLOW QUERY LOG - add affected rows (UPDATE / DELETE) in slow query log
by Sergei Golubchik 30 Nov '14

30 Nov '14

Hi, Nirbhay! On Nov 28, nirbhay(a)mariadb.com wrote: > revision-id: 7a5559421f4f3dda44d04bf946e79661d09b4dc2 > parent(s): 7b55b67de5fdfe91283357fe6c7ccc3f9e355925 > committer: Nirbhay Choubey > branch nick: b4412 > timestamp: 2014-11-28 20:53:02 -0500 > message: > > MDEV-4412 : SLOW QUERY LOG - add affected rows (UPDATE / DELETE) in slow query log > > Added Rows_affected to slow query log & mysql.slow_log table. Just a couple of comments: > diff --git a/scripts/mysql_system_tables_fix.sql b/scripts/mysql_system_tables_fix.sql > index b9bb59c..4973dc9 100644 > --- a/scripts/mysql_system_tables_fix.sql > +++ b/scripts/mysql_system_tables_fix.sql > @@ -262,7 +262,8 @@ ALTER TABLE slow_log > MODIFY insert_id INTEGER NOT NULL, > MODIFY server_id INTEGER UNSIGNED NOT NULL, > MODIFY sql_text MEDIUMTEXT NOT NULL, > - MODIFY thread_id BIGINT(21) UNSIGNED NOT NULL; > + MODIFY thread_id BIGINT(21) UNSIGNED NOT NULL, > + MODIFY rows_affected INTEGER NOT NULL; Eh? That makes no sense. This file is for mysql_upgrade. Your ALTER TABLE would help if there was some MariaDB version where rows_affected column would've been nullable and you wanted to change it to NOT NULL. But there was no such MariaDB version. There was MariaDB version without rows_affected column at all. So, instead of this MODIFY rows_affected, you need to put another ALTER TABLE statement that adds rows_affected INTEGER NOT NULL column. > SET GLOBAL slow_query_log = @old_log_state; > > ALTER TABLE plugin > diff --git a/sql/log.cc b/sql/log.cc > index ff04105..24e5283 100644 > --- a/sql/log.cc > +++ b/sql/log.cc > @@ -2918,12 +2924,16 @@ bool MYSQL_QUERY_LOG::write(THD *thd, time_t current_time, > sprintf(lock_time_buff, "%.6f", ulonglong2double(lock_utime)/1000000.0); > if (my_b_printf(&log_file, > "# Thread_id: %lu Schema: %s QC_hit: %s\n" \ > - "# Query_time: %s Lock_time: %s Rows_sent: %lu Rows_examined: %lu\n", > + "# Query_time: %s Lock_time: %s Rows_sent: %lu Rows_examined: %lu\n" \ > + "# Rows_affected: %lu\n", I'd rather have Rows_affected on the same line with Rows_sent and Rows_examined > (ulong) thd->thread_id, (thd->db ? thd->db : ""), > ((thd->query_plan_flags & QPLAN_QC) ? "Yes" : "No"), > query_time_buff, lock_time_buff, > (ulong) thd->get_sent_row_count(), > - (ulong) thd->get_examined_row_count()) == (size_t) -1) > + (ulong) thd->get_examined_row_count(), > + thd->get_stmt_da()->is_ok() ? > + (ulong) thd->get_stmt_da()->affected_rows() : > + 0) == (size_t) -1) > tmp_errno= errno; > if ((thd->variables.log_slow_verbosity & LOG_SLOW_VERBOSITY_QUERY_PLAN) && > (thd->query_plan_flags & Regards, Sergei

2 1

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