September 2016 - developers - lists.mariadb.org

[Maria-developers] Windows line endings in MariaDB sources again (
by Otto Kekäläinen 24 Jul '19

24 Jul '19

Hello! While trying to release 10.0.26 for Debian and Ubuntu I noticed git chokes again on Windows line endings. As our .gitattributes states that by default git should clean up the line endings, why do we keep getting new Windows line endings in new code? mariadb-10.0$ cat .gitattributes # Set the default behavior, in case people don't have core.autocrlf set. * text=auto ... storage/connect/mysql-test/connect/std_data/*.txt -text mariadb-10.0.26$ find . | xargs file | grep CRLF ./storage/connect/mysql-test/connect/std_data/boyswin.txt: ASCII text, with CRLF line terminators ./storage/connect/mysql-test/connect/std_data/expenses.txt: ASCII text, with CRLF line terminators ./storage/connect/mysql-test/connect/std_data/emp.txt: ASCII text, with CRLF line terminators ./mysql-test/std_data/loaddata7.dat: ASCII text, with CRLF line terminators ./mysql-test/r/loadxml.result: ASCII text, with CRLF, LF line terminators ./mysql-test/r/perror-win.result: ASCII text, with CRLF, LF line terminators ./mysql-test/r/mysql_binary_mode.result: ASCII English text, with CRLF, LF line terminators ./mysql-test/r/func_regexp_pcre.result: UTF-8 Unicode C++ program text, with CRLF, CR, LF line terminators ./pcre/testdata/grepoutputN: ASCII text, with CRLF, CR, LF line terminators ./pcre/testdata/greppatN4: ASCII text, with CRLF line terminators This leads for me to: dpkg-source: info: building mariadb-10.0 using existing ./mariadb-10.0_10.0.26.orig.tar.gz dpkg-source: info: local changes detected, the modified files are: mariadb-10.0/storage/connect/mysql-test/connect/std_data/boyswin.txt mariadb-10.0/storage/connect/mysql-test/connect/std_data/emp.txt mariadb-10.0/storage/connect/mysql-test/connect/std_data/expenses.txt dpkg-source: error: aborting due to unexpected upstream changes, see /tmp/mariadb-10.0_10.0.26-1.diff.Yzwwhw Which means I have to do extra work with importing the upstream sources.. - Otto

2 4

[Maria-developers] MDEV-7660 MySQL WL#6671 "Improve scalability by not using thr_lock.c locks for InnoDB tables"
by Sergei Golubchik 29 Oct '16

29 Oct '16

Hi, Sergey, I think it's fine. I had a few questions though, see below: > commit 47aa5f9 > Author: Sergey Vojtovich <svoj(a)mariadb.org> > Date: Fri May 6 13:44:07 2016 +0400 > > MDEV-7660 - MySQL WL#6671 "Improve scalability by not using thr_lock.c locks > for InnoDB tables" > > Don't use thr_lock.c locks for InnoDB tables. > > Let HANDLER READ call external_lock() even if SE is not going to be locked by > THR_LOCK. This fixes at least main.implicit_commit failure. > > Removed tests for BUG#45143 and BUG#55930 which cover InnoDB + THR_LOCK. To > operate properly these tests require code flow to go through THR_LOCK debug > sync points, which is not the case after this patch. These tests are removed > by WL#6671 as well. An alternative is to port them to different storage engine. > > For the very same reason partition_debug_sync test was adjusted to use MyISAM. > > diff --git a/sql/sql_handler.cc b/sql/sql_handler.cc > index e8ade81..76107ae 100644 > --- a/sql/sql_handler.cc > +++ b/sql/sql_handler.cc > @@ -752,11 +752,12 @@ bool mysql_ha_read(THD *thd, TABLE_LIST *tables, > tables->table= table; // This is used by fix_fields > table->pos_in_table_list= tables; > > - if (handler->lock->lock_count > 0) > + if (handler->lock->table_count > 0) > { > int lock_error; > > - handler->lock->locks[0]->type= handler->lock->locks[0]->org_type; > + if (handler->lock->lock_count > 0) > + handler->lock->locks[0]->type= handler->lock->locks[0]->org_type; I don't understand this code in mysql_ha_read() at all :( even before your changes > /* save open_tables state */ > TABLE* backup_open_tables= thd->open_tables; > diff --git a/storage/xtradb/handler/ha_innodb.h b/storage/xtradb/handler/ha_innodb.h > index 2027a59..efb8120 100644 > --- a/storage/xtradb/handler/ha_innodb.h > +++ b/storage/xtradb/handler/ha_innodb.h > @@ -218,6 +218,7 @@ class ha_innobase: public handler > bool can_switch_engines(); > uint referenced_by_foreign_key(); > void free_foreign_key_create_info(char* str); > + uint lock_count(void) const; > THR_LOCK_DATA **store_lock(THD *thd, THR_LOCK_DATA **to, > enum thr_lock_type lock_type); > void init_table_handle_for_HANDLER(); > > commit 5645626 > Author: Sergey Vojtovich <svoj(a)mariadb.org> > Date: Tue May 24 12:25:56 2016 +0400 > > MDEV-7660 - MySQL WL#6671 "Improve scalability by not using thr_lock.c locks > for InnoDB tables" > > - InnoDB now acquires shared lock for HANDLER ... READ Why? > - LOCK TABLES now disables autocommit implicitely > - UNLOCK TABLES now re-enables autocommit implicitely if it was disabled by > LOCK TABLES > - adjusted test cases to this new behavior > > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > index 3091bd6..7d39484 100644 > --- a/sql/sql_base.cc > +++ b/sql/sql_base.cc > @@ -2824,6 +2824,8 @@ Locked_tables_list::unlock_locked_tables(THD *thd) > request for metadata locks and TABLE_LIST elements. > */ > reset(); > + if (thd->variables.option_bits & OPTION_AUTOCOMMIT) > + thd->variables.option_bits&= ~(OPTION_NOT_AUTOCOMMIT); 1. Was it possible - before your change - for OPTION_AUTOCOMMIT and OPTION_NOT_AUTOCOMMIT to be out of sync? 2. What if someone changes @@autocommit under LOCK TABLES? Do you have a test for that? 3. Do you need to set SERVER_STATUS_AUTOCOMMIT here? > } > > > diff --git a/mysql-test/t/innodb_mysql_lock.test b/mysql-test/t/innodb_mysql_lock.test > index cb57c09..85ba418 100644 > --- a/mysql-test/t/innodb_mysql_lock.test > +++ b/mysql-test/t/innodb_mysql_lock.test > @@ -150,14 +150,16 @@ let $wait_condition= > --source include/wait_condition.inc > LOCK TABLES t1 READ; > SELECT release_lock('bug42147_lock'); > +let $wait_condition= > + SELECT COUNT(*) > 0 FROM information_schema.processlist > + WHERE state = 'executing' > + AND info = 'INSERT INTO t1 SELECT get_lock(\'bug42147_lock\', 60)'; > +--source include/wait_condition.inc > +UNLOCK TABLES; I don't understand the original test case. But after your changes it actually makes sense :) > > connection default; > --reap > > -connection con2; > -UNLOCK TABLES; > - > -connection default; > disconnect con2; > DROP TABLE t1; > > diff --git a/mysql-test/r/partition_explicit_prune.result b/mysql-test/r/partition_explicit_prune.result > index 765803d..7b9c53d 100644 > --- a/mysql-test/r/partition_explicit_prune.result > +++ b/mysql-test/r/partition_explicit_prune.result > @@ -281,7 +281,7 @@ UNLOCK TABLES; > SELECT * FROM INFORMATION_SCHEMA.SESSION_STATUS > WHERE VARIABLE_NAME LIKE 'HANDLER_%' AND VARIABLE_VALUE > 0; > VARIABLE_NAME VARIABLE_VALUE > -HANDLER_COMMIT 2 > +HANDLER_COMMIT 3 why is that? > HANDLER_READ_RND_NEXT 52 > HANDLER_TMP_WRITE 72 > HANDLER_WRITE 2 > diff --git a/mysql-test/suite/handler/handler.inc b/mysql-test/suite/handler/handler.inc > index b1e881f..8cad6a5 100644 > --- a/mysql-test/suite/handler/handler.inc > +++ b/mysql-test/suite/handler/handler.inc > @@ -1091,6 +1091,12 @@ connection default; > --reap > drop table t2; > > +# This test expects "handler t1 read a next" to get blocked on table level > +# lock so that further "drop table t1" can break the lock and close handler. > +# This notification mechanism doesn't work with InnoDB since it bypasses > +# table level locks. what happens for InnoDB then? I'd expect "handler t1 read a next" to get blocked inside the InnoDB. what does then "drop table t1" do? > +if ($engine_type != 'InnoDB') > +{ > --echo # > --echo # Bug #46224 HANDLER statements within a transaction might > --echo # lead to deadlocks Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 8

[Maria-developers] Problem with parallel replication in 10.2
by Michael Widenius 17 Oct '16

17 Oct '16

Hi! I was trying to run a test that fails in the upcoming bb-10.2-jan on the normal 10.2 tree, when I noticed this strange issue: - Test fails with timeout when running with --debug - When looking at the trace file, I notice that we get a duplicate key error for the table gtid_slave_post (MyISAM table). Is this something normal ? To repeat: Store the included test in suite/rpl and run it with: mysql-test-run --debug --record rpl_skr Issue 2: bb-10.2-jan tree is a working tree for a merge of MariaDB 10.2 and MySQL 5.7 When running rpl_skr in 10.2 it takes 2 seconds When running it in the bb-10.2-jan tree it takes either a long time or we get a timeout. This is probably because of the new lock code in lock0lock.cc and lock0wait.cc which doesn't break conflicting transaction but instead waits for a timeout Would appreciate any help with this! Note that rpl_skr is a test that was originally part of rpl_parallel.test, but I have made it separate to be able to more easily test for this issue. Before testing bb-10.2-jan, please apply this patch that fixes one critical issue in this tree related to group-commit: diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc index b69468d..0e9caed 100644 --- a/storage/innobase/handler/ha_innodb.cc +++ b/storage/innobase/handler/ha_innodb.cc @@ -4976,16 +4953,15 @@ innobase_commit( thd_wakeup_subsequent_commits(thd, 0); /* Now do a write + flush of logs. */ - if (!read_only) { - trx_commit_complete_for_mysql(trx); - } + trx_commit_complete_for_mysql(trx); trx_deregister_from_2pc(trx); } else { /* We just mark the SQL statement ended and do not do a transaction commit */ + DBUG_PRINT("info", ("Just mark SQL statement")); /* If we had reserved the auto-inc lock for some table in this SQL statement we release it now */ Regards, Monty

3 9

Re: [Maria-developers] MDEV-7145
by Kristian Nielsen 17 Oct '16

17 Oct '16

Phil Sweeney <launchpad(a)sweeneymail.com> writes: > Re: https://jira.mariadb.org/browse/MDEV-7145 > Delayed replication (a feature shipped in MySQL 5.6) has been a MariaDB > feature request since 2014, and has been listed as 'major/red' priority in > Can anyone comment on whether this is still likely to make it into 10.2. I am planning to look into this. - Kristian.

3 4

Re: [Maria-developers] [Commits] 1c9da8d: MDEV-9312: storage engine not enforced during galera cluster replication
by Jan Lindström 29 Sep '16

29 Sep '16

Hi Nirbhay, This looks ok but one question (no need to change now): On Wed, Sep 28, 2016 at 7:36 PM, Nirbhay Choubey <nirbhay(a)mariadb.com> wrote: > > + Since some wsrep threads (THDs) are create before plugins are > + initialized, LOCK_plugin mutex needs to be initialized here. > + */ > Is there some fundamental reason why we can't create wsrep threads after other plugins and related global system variables (that are really needed for wsrep) are initialized ? R: Jan

2 1

Re: [Maria-developers] [Commits] a8162d4: MDEV-9312: storage engine not enforced during galera cluster replication
by Sergei Golubchik 28 Sep '16

28 Sep '16

Hi, Nirbhay! I don't understand, why do you need to create a dummy plugin here? On Sep 27, Nirbhay Choubey wrote: > revision-id: a8162d4a8737cff67889390fad0153acc175391d (mariadb-10.1.17-22-ga8162d4) > parent(s): 6a6b253a6ecbd4d3dd254044d12ec64475453275 > author: Nirbhay Choubey > committer: Nirbhay Choubey > timestamp: 2016-09-27 09:03:26 -0400 > message: > > MDEV-9312: storage engine not enforced during galera cluster replication > > Perform a post initialization of plugin-related variables > of wsrep threads after their global counterparts have been > initialized. ... > +#ifdef WITH_WSREP > + > +/* > + Placeholder for global_system_variables.table_plugin required during > + initialization of startup wsrep threads. > +*/ > +static st_plugin_int *wsrep_dummy_plugin; > + > +/* > + Initialize wsrep_dummy_plugin and assign it to > + global_system_variables.table_plugin. > +*/ > +void wsrep_plugins_pre_init() > +{ > + wsrep_dummy_plugin= > + (st_plugin_int *) my_malloc(sizeof(st_plugin_int), MYF(0)); > + wsrep_dummy_plugin->state= PLUGIN_IS_DISABLED; > + global_system_variables.table_plugin= plugin_int_to_ref(wsrep_dummy_plugin); > +} > + Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 2

Re: [Maria-developers] 12ffe70: MDEV-9416: MariaDB galera got signal 11 when altering table add unique index
by Sergei Golubchik 28 Sep '16

28 Sep '16

Hi, Nirbhay! On Sep 24, Nirbhay Choubey wrote: > revision-id: 12ffe70831c9c69c3d6ef83431443696455c332b (mariadb-10.1.17-23-g12ffe70) > parent(s): 8ffded0a78c0a4912b32acac4c3f58f04c3bcd87 > author: Nirbhay Choubey > committer: Nirbhay Choubey > timestamp: 2016-09-24 00:27:38 -0400 > message: > > MDEV-9416: MariaDB galera got signal 11 when altering table add unique index > > When a BF thread attempts to abort a victim thread's transaction, > the victim thread is not locked and thus its not safe to rely on > its data structures like htons registered for the trx. > > So, instead of getting the registered htons from victim, innodb's > hton can be looked up directly from installed_htons[] and used to > abort the transaction. (Same technique is used in older versions) > > diff --git a/sql/handler.cc b/sql/handler.cc > index 4e4c8fa..6a32fec 100644 > --- a/sql/handler.cc > +++ b/sql/handler.cc > @@ -6108,29 +6108,16 @@ int ha_abort_transaction(THD *bf_thd, THD *victim_thd, my_bool signal) > DBUG_RETURN(0); > } > > - /* Try statement transaction if standard one is not set. */ > - THD_TRANS *trans= (victim_thd->transaction.all.ha_list) ? > - &victim_thd->transaction.all : &victim_thd->transaction.stmt; > - > - Ha_trx_info *ha_info= trans->ha_list, *ha_info_next; > - > - for (; ha_info; ha_info= ha_info_next) > + handlerton *hton= installed_htons[DB_TYPE_INNODB]; > + if (hton && hton->abort_transaction) > { > - handlerton *hton= ha_info->ht(); > - if (!hton->abort_transaction) > - { > - /* Skip warning for binlog & wsrep. */ > - if (hton->db_type != DB_TYPE_BINLOG && hton != wsrep_hton) > - { > - WSREP_WARN("Cannot abort transaction."); > - } > - } > - else > - { > - hton->abort_transaction(hton, bf_thd, victim_thd, signal); > - } > - ha_info_next= ha_info->next(); > + hton->abort_transaction(hton, bf_thd, victim_thd, signal); Okay, but please add a comment that it's safe to abort from one thread (bf_thd) the transaction, running in another thread (victim_thd), because innodb's lock_sys and trx_mutex guarantee the necessary protection. And that it's not safe to access victim_thd->transaction, because it's not protected from concurrent accesses. And it's an overkill to take LOCK_plugin and iterate the whole installed_htons[] array every time. Then ok to push > } > + else > + { > + WSREP_WARN("Cannot abort InnoDB transaction"); > + } > + > DBUG_RETURN(0); > } Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] [Commits] f2aea43: MDEV-10649: Optimizer sometimes use "index" instead of "range" access for UPDATE
by Jan Lindström 28 Sep '16

28 Sep '16

On Tue, Sep 6, 2016 at 8:37 PM, Sergei Petrunia <psergey(a)askmonty.org> wrote: > revision-id: f2aea435df7e92fcf8f09f8f6c160161168c5bed > parent(s): a14f61ef749ad9f9ab2b0f5badf6754ba7443c9e > committer: Sergei Petrunia > branch nick: 10.0 > timestamp: 2016-09-06 20:37:21 +0300 > message: > > MDEV-10649: Optimizer sometimes use "index" instead of "range" access for > UPDATE > > (XtraDB variant only, for now) > > Re-opening a TABLE object (after e.g. FLUSH TABLES or open table cache > eviction) causes ha_innobase to call > dict_stats_update(DICT_STATS_FETCH_ONLY_IF_NOT_IN_MEMORY). > > Inside this call, the following is done: > dict_stats_empty_table(table); > dict_stats_copy(table, t); > > On the other hand, commands like UPDATE make this call to get the "rows in > table" statistics in table->stats.records: > > ha_innobase->info(HA_STATUS_VARIABLE|HA_STATUS_NO_LOCK) > > note the HA_STATUS_NO_LOCK parameter. It means, no locks are taken by > ::info() If the ::info() call happens between dict_stats_empty_table > and dict_stats_copy calls, the UPDATE's optimizer will get an estimate > of table->stats.records=1, which causes it to pick a full table scan, > which in turn will take a lot of row locks and cause other bad > consequences. > > --- > storage/xtradb/dict/dict0stats.cc | 29 +++++++++++++++++++---------- > 1 file changed, 19 insertions(+), 10 deletions(-) > > diff --git a/storage/xtradb/dict/dict0stats.cc b/storage/xtradb/dict/ > dict0stats.cc > index b073398..a4aa436 100644 > --- a/storage/xtradb/dict/dict0stats.cc > +++ b/storage/xtradb/dict/dict0stats.cc > @@ -673,7 +673,10 @@ Write all zeros (or 1 where it makes sense) into a > table and its indexes' > dict_stats_copy( > /*============*/ > dict_table_t* dst, /*!< in/out: destination table */ > - const dict_table_t* src) /*!< in: source table */ > + const dict_table_t* src, /*!< in: source table */ > + bool reset_ignored_indexes) /*!< in: if true, set ignored > indexes > + to have the same statistics > as if > + the table was empty */ > { > dst->stats_last_recalc = src->stats_last_recalc; > dst->stat_n_rows = src->stat_n_rows; > @@ -692,7 +695,16 @@ Write all zeros (or 1 where it makes sense) into a > table and its indexes' > && (src_idx = dict_table_get_next_index(src_idx)))) { > > if (dict_stats_should_ignore_index(dst_idx)) { > - continue; > + if (reset_ignored_indexes) { > + /* Reset index statistics for all ignored > indexes, > + unless they are FT indexes (these have no > statistics)*/ > + if (dst_idx->type & DICT_FTS) { > + continue; > + } > + dict_stats_empty_index(dst_idx); > Does this really help? Yes, we hold dict_sys mutex here so dict_stats_empty_index here is safe for all readers using same mutex. However, as you pointed up above, info() uses no locking method. Thus, we really do not know what it will return, all values before dict_stats_copy(), some of the indexes with new stats, all indexes with new stats. > > @@ -3240,13 +3252,10 @@ N*AVG(Ui). In each call it searches for the > currently fetched index into > > dict_table_stats_lock(table, RW_X_LATCH); > > - /* Initialize all stats to dummy values before > - copying because dict_stats_table_clone_create() > does > - skip corrupted indexes so our dummy object 't' may > - have less indexes than the real object 'table'. */ > - dict_stats_empty_table(table); > - > - dict_stats_copy(table, t); > + /* Pass reset_ignored_indexes=true as parameter > + to dict_stats_copy. This will cause statictics > + for corrupted indexes to be set to empty values */ > + dict_stats_copy(table, t, true); > This is better solution than the original but as noted above, is this enough ? R: Jan

2 2

[Maria-developers] Review for MDEV-6112 multiple triggers per table
by Alexander Barkov 28 Sep '16

28 Sep '16

Hello Monty. I checked your patch for MDEV-6112 multiple triggers per table. Here are some quick notes. I'll send a detailed review separately. 1. It would be nice to push refactoring changes in separate patch: a. do sql_mode_t refactoring b. "trg_idx -> Trigger *" refactoring c. opt_debugging related things 2. Why not to reuse the FOLLOWING_SYM and PRECEDING_SYM keywords instead of adding new ones FOLLOWS_SYM, PRECEDES_SYM ? (this is a non-standard syntax anyway) 3. I'd prefer PRECEDES/FOLLOWS to go before FOR EACH ROW. "FOR EACH ROW stmt" is a kind of single clause responsible for the action. It's very confusing to have PRECEDES/FOLLOWS characteristics in between "FOR EACH ROW" and stmt. 4. You have a lot of changes related to trim_whitespace() where you declare an unused "prefix_removed" variable. It would be nice to avoid declaring it. Possible ways: a. Perhaps the new function version can be defined like this: size_t trim_whitespace(CHARSET_INFO *cs, LEX_STRING *str); and make it return "prefix length removed", instead of adding a new parameter. b. Another option is just to overload it: extern void trim_whitespace(CHARSET_INFO *cs, LEX_STRING *str, uint *prefix_removed); inline void trim_whitespace(CHARSET_INFO *cs, LEX_STRING *str) { uint not_used; trim_whitespace(cs, str, &not_used); } 5. The "CREATED" field is not populated well. I'm getting strange values like 1970-04-24 09:35:00.30. Btw, his query returns a correct value: SELECT FROM_UNIXTIME(UNIX_TIMESTAMP(CREATED)*100) FROM INFORMATION_SCHEMA.triggers; You must have forgotten to multiply or delete to 100 somewhere. 6. In replication tests it's better to make sure that CREATED is safely replicated (instead of changing it to #). By the way, in other tests it's also a good idea not to use # for CREATED. (see the previous problem) You can use this: SET TIMESTAMP=UNIX_TIMESTAMP('2001-01-01 10:20:30'); ... SET TIMESTAM=DEFAULT; 7. I suggest to move this code into a method, say find_trigger(): + if (table->triggers) + { + Trigger *t; + for (t= table->triggers->get_trigger(TRG_EVENT_INSERT, + TRG_ACTION_BEFORE); + t; + t= t->next) + if (t->is_fields_updated_in_trigger(&full_part_field_set)) + DBUG_RETURN(false); + } It's repeated at least two times. 8. The following structure fields are defined two times: enum trigger_order_type ordering_clause; LEX_STRING anchor_trigger_name; The first time for st_trg_chistics. The second time for %union in sql_yacc.yy Note, there will be the third time in sql_yacc_ora.yy. Please define a structure in sql_lex.h instead: struct st_trg_execution_order_chistics { /** FOLLOWS or PRECEDES as specified in the CREATE TRIGGER statement. */ enum trigger_order_type ordering_clause; /** Trigger name referenced in the FOLLOWS/PRECEDES clause of the CREATE TRIGGER statement. */ LEX_STRING anchor_trigger_name; }; and reuse it in here: struct st_trg_chirstics: public st_trg_execution_order_chistics { ... }; and in here: %union { ... st_trg_execition_order_chistics trg_execution_order_chirstics; ... }; Please also rename trg_characteristics to trg_execution_order_chistics, because "trg_characteristics" is more suitable to something having type st_trg_chirstics rather than st_trg_execution_order_chistics. Greetings.

1 1

[Maria-developers] Possibly a problem in "Allowed to use WITH clauses before SELECT in CREATE ... SELECT and INSERT ... SELECT"
by Alexander Barkov 28 Sep '16

28 Sep '16

Hello Igor, I was rebasing bb-10.2-compatibility on top of the latest 10.2 and noticed this grammar change: > create_select_query_specification: > - SELECT_SYM create_select_part2 create_select_part3 create_select_part4 > + SELECT_SYM opt_with_clause create_select_part2 create_select_part3 > + create_select_part4 > + { > + Select->set_with_clause($2); > + } > ; I have some questions: - From my understanding, opt_with_clause should go before SELECT_SYM Why does it go after? - Why this grammar change is not covered in tests? - What is the MDEV number for this change? Thanks!

1 0