- developers - lists.mariadb.org

[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

[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