- developers - lists.mariadb.org

Re: [Maria-developers] 980a5c31880: MDEV-14572: Assertion `! is_set()' failed in Diagnostics_area::set_eof_status upon EXPLAIN UPDATE in PS
by Sergei Golubchik 19 Jun '18

19 Jun '18

Hi, Oleksandr! On Jan 28, Oleksandr Byelkin wrote: > Hi, Sergei! > > Flag in SELECT_LEX options dropped at the end of execution (in our case > prepare). All commands where EXPLAIN is possible restore the flag in main > SELECT_LEX (in mysql_explain_union()) except UPDATE (because it do > not use mysql_explain_union() > for explain). Without the flag it first try to send OK (as part of update) > then EOF as part of EXPLAIN. Ok. 1. Why does it not use mysql_explain_union() ? 2. Why EXPLAIN flag is need to be removed and restored anyway? 3. Please append the following to the commit comment: " Without it, mysql_update() didn't know that there will be EXPLAIN result set and was sending OK at the end of the update, which conflicted with the EOF sent later by EXPLAIN. " > On Sun, Jan 28, 2018 at 2:45 PM, Sergei Golubchik <serg(a)mariadb.org> wrote: > > > Hi, Oleksandr! > > > > On Dec 22, Oleksandr Byelkin wrote: > > > revision-id: 980a5c318803d6aa7355e019869597976dcf19bf (mariadb-10.0.33-37-g980a5c31880) > > > parent(s): 042f763268c0f209e7c12e0a6a72bb5d204dfe29 > > > author: Oleksandr Byelkin > > > committer: Oleksandr Byelkin > > > timestamp: 2017-12-22 14:00:10 +0100 > > > message: > > > > > > MDEV-14572: Assertion `! is_set()' failed in Diagnostics_area::set_eof_status upon EXPLAIN UPDATE in PS > > > > > > Restore EXPAIN flag in SELECT_LEX before execution multi-update by flag in LEX > > > (the same but in other way made before INSERT/DELETE/SELECT) > > > > Please explain what was the reason for the bug, why the status was set > > twice, and how your change fixes it. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 3

Re: [Maria-developers] 398ea6a: MDEV-11917 enum/set command-line options aren't respecting max-* settings.
by Sergei Golubchik 18 Jun '18

18 Jun '18

Hi, Alexey! On Mar 13, Alexey Botchkov wrote: > revision-id: 398ea6ab012938d08126c8d0e0291000d06f6cd6 (mariadb-10.2.13-43-g398ea6a) > parent(s): 9d95b8665a3d87ab857e77e220effc64454eb881 > committer: Alexey Botchkov > timestamp: 2018-03-13 17:13:27 +0400 > message: > > MDEV-11917 enum/set command-line options aren't respecting max-* settings. > > Code added to make maximum- working for Sys_var_enum > and Sys_var_set. > > diff --git a/sql/set_var.cc b/sql/set_var.cc > index e96e636..32e52a9 100644 > --- a/sql/set_var.cc > +++ b/sql/set_var.cc > @@ -452,6 +452,22 @@ void sys_var::do_deprecated_warning(THD *thd) > > @retval true on error, false otherwise (warning or ok) > */ > + > + > +bool throw_bounds_warning(THD *thd, const char *name,const char *v) > +{ > + if (thd->variables.sql_mode & MODE_STRICT_ALL_TABLES) > + { > + my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), name, v); This is rather illogical, setting a variable doesn't touch any tables, so MODE_STRICT_ALL_TABLES should not affect it. Also, the error message could be improved. But it's how it was, and it's not subject of this bugfix, I agree. > + return true; > + } > + push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN, > + ER_TRUNCATED_WRONG_VALUE, > + ER_THD(thd, ER_TRUNCATED_WRONG_VALUE), name, v); > + return false; > +} > + > + > bool throw_bounds_warning(THD *thd, const char *name, > bool fixed, bool is_unsigned, longlong v) > { > @@ -464,14 +480,7 @@ bool throw_bounds_warning(THD *thd, const char *name, > else > llstr(v, buf); > > - if (thd->variables.sql_mode & MODE_STRICT_ALL_TABLES) > - { > - my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), name, buf); > - return true; > - } > - push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN, > - ER_TRUNCATED_WRONG_VALUE, > - ER_THD(thd, ER_TRUNCATED_WRONG_VALUE), name, buf); ah, good. only one place to change when we fix the above. thanks. > + return throw_bounds_warning(thd, name, buf); > } > return false; > } > diff --git a/sql/sys_vars.ic b/sql/sys_vars.ic > index bc913b1..adcda5d 100644 > --- a/sql/sys_vars.ic > +++ b/sql/sys_vars.ic all ok, looks good. Just one comment. Please make max_var_ptr() logic to be in one place only. If C++ cannot do it, then, in the worst case, make it a macro, like #define define_max_var_ptr(TYPE) \ TYPE *max_var_ptr() { return scope() == SESSION ? (TYPE*)(((uchar*)&max_system_variables) + offset) : 0; } after that ok to push, thanks! Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] f3dda8e: MDEV-15357 Wrong result with join_cache_level=4, BNLH join
by Sergei Golubchik 15 Jun '18

15 Jun '18

Hi, Igor! On Apr 30, Igor Babaev wrote: > revision-id: f3dda8e2002dc2ad630c07bda0d8c00be7269907 (mariadb-10.2.14-72-gf3dda8e) > parent(s): b4c5e4a717e3ce2c2d434106cc74417fe9a1d3dc > author: Igor Babaev > committer: Igor Babaev > timestamp: 2018-04-30 21:26:38 -0700 > message: > > MDEV-15357 Wrong result with join_cache_level=4, BNLH join > > This bug was introduced by the architectural changes of the patch > for MDEV-11640. The patch moved initialization of join caches after > the call of make_join_readinfo(). As any failure to initialize > a join cache caused denial of its usage the execution code for > the query had to be revised. This revision required rolling back many > actions taken by make_join_readinfo(). It was not done in the patch. > As a result if a denial of join cache happened some join conditions > could be lost. This was exactly the cause of wrong results in the > bug's reported test case. > > Thus the introduced architectural change is not valid and it would be > better to roll it back. At the same time two new methods > adjust_read_set_for_vcol_keyread() and get_covering_index_for_scan() > were added to the class JOIN_TAB to resolve the problems of MDEV-11640. > > diff --git a/sql/sql_join_cache.cc b/sql/sql_join_cache.cc > index 3612cb6..242cb91 100644 > --- a/sql/sql_join_cache.cc > +++ b/sql/sql_join_cache.cc > @@ -226,6 +226,16 @@ void JOIN_CACHE::calc_record_fields() > flag_fields+= MY_TEST(tab->table->maybe_null); > fields+= tab->used_fields; > blobs+= tab->used_blobs; > + if (tab->type == JT_ALL || tab->type == JT_HASH) > + { > + uint idx= tab->get_covering_index_for_scan(); as you moved JOIN_CACHE initialization back, I suspect that this calc_record_fields() now happens too early for get_covering_index_for_scan() to be useful. And why only JT_ALL and JT_HASH? > + if (idx != MAX_KEY) > + { > + fields-= bitmap_bits_set(tab->table->read_set); > + tab->adjust_read_set_for_vcol_keyread(idx); > + fields+= bitmap_bits_set(tab->table->read_set); > + } > + } > } > if ((with_match_flag= join_tab->use_match_flag())) > flag_fields++; > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index 30ef26f..e5b75ec 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -11404,6 +11375,61 @@ void JOIN_TAB::remove_redundant_bnl_scan_conds() > } > > > +uint JOIN_TAB::get_covering_index_for_scan() > +{ > + int idx= MAX_KEY; > + if (table->no_keyread) > + return idx; > + if (select && select->quick && > + select->quick->index != MAX_KEY && //not index_merge > + table->covering_keys.is_set(select->quick->index)) > + idx= select->quick->index; > + else if (!table->covering_keys.is_clear_all() && > + !(select && select->quick)) > + { // Only read index tree > + if (loosescan_match_tab) > + idx= loosescan_key; > + else > + { > +#ifdef BAD_OPTIMIZATION > + /* > + It has turned out that the below change, while speeding things > + up for disk-bound loads, slows them down for cases when the data > + is in disk cache (see BUG#35850): > + See bug #26447: "Using the clustered index for a table scan > + is always faster than using a secondary index". > + */ > + if (table->s->primary_key != MAX_KEY && > + table->file->primary_key_is_clustered()) > + idx= table->s->primary_key; > + else > +#endif > + idx= find_shortest_key(table, & table->covering_keys); > + } > + } > + return idx; > +} > + > + > +void JOIN_TAB::adjust_read_set_for_vcol_keyread(uint keyread_idx) > +{ > + if (!table->vcol_set || bitmap_is_clear_all(table->vcol_set)) > + return; > + > + MY_BITMAP *keyread_set= &table->cond_set; // is free for use at call normally one uses tmp_set for this, not hijacks cond_set > + bitmap_clear_all(keyread_set); > + table->mark_columns_used_by_index(keyread_idx, keyread_set); > + bitmap_intersect(keyread_set, table->read_set); > + bitmap_intersect(keyread_set, table->vcol_set); this looks wrong, you probably should do keyread_set INTERSECT (read_set UNION vcol_set) > + if (!bitmap_is_clear_all(keyread_set)) > + { > + bitmap_clear_all(keyread_set); > + table->mark_columns_used_by_index(keyread_idx, keyread_set); > + bitmap_intersect(table->read_set, keyread_set); > + } I don't understand that. 1. What if bitmap_is_clear_all(keyread_set) - what does it mean? 2. Why do you recalculate keyread_set, why not simply copy keyread_set into read_set ? > +} > + > + > /* > Plan refinement stage: do various setup things for the executor > > @@ -11526,7 +11552,10 @@ make_join_readinfo(JOIN *join, ulonglong options, uint no_jbuf_after) > tab->read_first_record= tab->type == JT_SYSTEM ? join_read_system > : join_read_const; > if (table->covering_keys.is_set(tab->ref.key) && !table->no_keyread) > + { > + tab->adjust_read_set_for_vcol_keyread(tab->ref.key); > table->file->ha_start_keyread(tab->ref.key); better combine both into inline void JOIN_TAB::start_keyread() { adjust_read_set_for_vcol_keyread(ref.key); tab->table->file->ha_start_keyread(ref.key); } or even JOIN_TAB::start_keyread_if_needed() putting the if() inside too > + } > else if ((!jcl || jcl > 4) && !tab->ref.is_access_triggered()) > push_index_cond(tab, tab->ref.key); > break; > @@ -11606,35 +11641,19 @@ make_join_readinfo(JOIN *join, ulonglong options, uint no_jbuf_after) > } > } > } > - if (!table->no_keyread) > + uint idx= tab->get_covering_index_for_scan(); > + if (idx != MAX_KEY) > { > if (tab->select && tab->select->quick && > tab->select->quick->index != MAX_KEY && //not index_merge > table->covering_keys.is_set(tab->select->quick->index)) > - table->file->ha_start_keyread(tab->select->quick->index); > + table->file->ha_start_keyread(idx); why do you need an if() here - you already checked that in get_covering_index_for_scan(). I generally don't like this code at all. In JOIN_CACHE you simply use tab->get_covering_index_for_scan(), but here you use it *plus* a bunch of if()s afterwards. It surely creates an impression that JOIN_CACHE and make_join_readinfo may have a different idea about whether keyread is used. Ideally here should be just if (idx != MAX_KEY) tab->start_keyread(idx); and even then it would be kinda questionable (because preconditions might change between two get_covering_index_for_scan() calls). > else if (!table->covering_keys.is_clear_all() && > !(tab->select && tab->select->quick)) > - { // Only read index tree > - if (tab->loosescan_match_tab) > - tab->index= tab->loosescan_key; > - else > - { > -#ifdef BAD_OPTIMIZATION > - /* > - It has turned out that the below change, while speeding things > - up for disk-bound loads, slows them down for cases when the data > - is in disk cache (see BUG#35850): > - See bug #26447: "Using the clustered index for a table scan > - is always faster than using a secondary index". > - */ > - if (table->s->primary_key != MAX_KEY && > - table->file->primary_key_is_clustered()) > - tab->index= table->s->primary_key; > - else > -#endif > - tab->index=find_shortest_key(table, & table->covering_keys); > - } > + { // Only read index tree > + tab->index= idx; > tab->read_first_record= join_read_first; > + table->file->ha_start_keyread(tab->index); > /* Read with index_first / index_next */ > tab->type= tab->type == JT_ALL ? JT_NEXT : JT_HASH_NEXT; > } Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] MDEV-6111: Optimizer trace: let's get rid of "optimizer_trace_enabled"
by Sergey Petrunia 15 Jun '18

15 Jun '18

Hi Zhzhzoo, I see that the current patch uses @@optimizer_trace_enabled variable to control it. Meanwhile, in MySQL the name of the variable is "optimizer_trace". It is a flag set: mysql> select @@optimizer_trace; +-------------------------+ | @@optimizer_trace | +-------------------------+ | enabled=on,one_line=off | +-------------------------+ 1 row in set (0.00 sec) but one can set it to 1 or 0 to just set enabled on/off: mysql> set @@optimizer_trace=1; Query OK, 0 rows affected (0.00 sec) mysql> set @@optimizer_trace=0; Query OK, 0 rows affected (0.00 sec) This obviously carries some legacy but I think we should have it the same way, in order to be compatible with MySQL where possible. Please use the same name/values as MySQL does. BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] MDEV-6111: Optimizer trace: on the first output
by Sergey Petrunia 15 Jun '18

15 Jun '18

On Tue, Jun 12, 2018 at 07:40:08AM +0800, zhzhzoo zhang wrote: > I've done > condition processing and some range analysis. > and for the test input ( > https://dev.mysql.com/doc/internals/en/tracing-example.html) It now > outputs something like > (not very readable from the email so I've made two pastes, from the current tree and from MySQL 8): https://gist.github.com/spetrunia/cb4a1a382a9e81a60491d524dbfc08f6 https://gist.github.com/spetrunia/62347cc3bf26e003909a32b378b6684f > ```{ > "join_optimization": { > "select #": 1, > "steps": { > "inner": { > "select #": 1, Two 'select #' :1 seem to be redundant. There are also two nested "steps" elements which seem redundant as well. > "steps": [ > { > "condition_processing": { > "condition": "WHERE", > "original_condition": "`test`.`alias1`.`pk` <> 0 and > `test`.`alias2`.`pk` = `test`.`alias1`.`col_int_key`", > "steps": [ > { > "transformation": "equality_propagation", > "subselect_evaluation": [], > "resulting_condition": "`test`.`alias1`.`pk` <> 0 and > multiple equal(`test`.`alias2`.`pk`, `test`.`alias1`.`col_int_key`)???" What are the "???" at the end? Please get them removed. also, if you check EXPLAIN FORMAT=JSON output, it prints conditions without overquoting, like so: "attached_condition": "alias1.pk <> 0 and alias1.col_int_key is not null" This is done by using QT_EXPLAIN parameter: item->print(&str, QT_EXPLAIN); Let's print them this way here as well. > }, > { > "transformation": "constant_propagation", > "subselect_evaluation": [], > "resulting_condition": "`test`.`alias1`.`pk` <> 0 and > multiple equal(`test`.`alias2`.`pk`, `test`.`alias1`.`col_int_key`)???" > }, BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] 46e0c8a9219: MDEV-11071: Assertion `thd->transaction.stmt.is_empty()' failed in Locked_tables_list::unlock_locked_table
by Sergei Golubchik 14 Jun '18

14 Jun '18

Hi, Oleksandr! Looks good, but please see a few comments below. On May 08, Oleksandr Byelkin wrote: > revision-id: 46e0c8a921978e7b95adf3f17f3c85b18d9f9ef6 (mariadb-10.2.14-78-g46e0c8a9219) > parent(s): 9bcd0f5fea8ca26742b10d37b95a966c69909ff1 > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2018-05-08 15:26:26 +0200 > message: > > MDEV-11071: Assertion `thd->transaction.stmt.is_empty()' failed in Locked_tables_list::unlock_locked_table > > fix_length_and_dec now return result (error/OK) > diff --git a/sql/item.h b/sql/item.h > index 8921ee76f6a..e9713730a1c 100644 > --- a/sql/item.h > +++ b/sql/item.h > @@ -4210,7 +4210,7 @@ class Item_func_or_sum: public Item_result_field, > also to make printing of items inherited from Item_sum uniform. > */ > virtual const char *func_name() const= 0; > - virtual void fix_length_and_dec()= 0; > + virtual bool fix_length_and_dec()= 0; 1. I wonder, if you change fix_length_and_dec to void in one of the derived classes, will you get a compiler warning for that? 2. may be also __attribute__ ((warn_unused_result)) here? I don't know how it'll work on virtual methods, may be it won't. both questions are for the use case "what if we merge some fix_length_and_dec related changes from 10.1, will the compiler tell us to update them?" > bool const_item() const { return const_item_cache; } > table_map used_tables() const { return used_tables_cache; } > Item* build_clone(THD *thd, MEM_ROOT *mem_root); > diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc > index 44cc4f3cae9..46cf412e229 100644 > --- a/sql/item_cmpfunc.cc > +++ b/sql/item_cmpfunc.cc > @@ -535,8 +535,9 @@ void Item_bool_rowready_func2::fix_length_and_dec() > we have to check for out of memory conditions here > */ > if (!args[0] || !args[1]) > - return; > - setup_args_and_comparator(current_thd, &cmp); > + return FALSE; > + bool res= setup_args_and_comparator(current_thd, &cmp); > + return res; not that it matters much (no need to change), but why didn't you return setup_args_and_comparator(current_thd, &cmp); directly? > } > > > diff --git a/sql/item_cmpfunc.h b/sql/item_cmpfunc.h > index de1b27cff1a..5efb98bc81d 100644 > --- a/sql/item_cmpfunc.h > +++ b/sql/item_cmpfunc.h > @@ -911,10 +911,12 @@ class Item_func_strcmp :public Item_int_func > longlong val_int(); > uint decimal_precision() const { return 1; } > const char *func_name() const { return "strcmp"; } > - void fix_length_and_dec() > + bool fix_length_and_dec() > { > - agg_arg_charsets_for_comparison(cmp_collation, args, 2); > + if (agg_arg_charsets_for_comparison(cmp_collation, args, 2)) > + return TRUE; > fix_char_length(2); // returns "1" or "0" or "-1" this comment is kinda weird. remove? > + return FALSE; > } > Item *get_copy(THD *thd, MEM_ROOT *mem_root) > { return get_item_copy<Item_func_strcmp>(thd, mem_root, this); } > @@ -1987,10 +1997,10 @@ class Item_func_like :public Item_bool_func2 > const char *func_name() const { return "like"; } > enum precedence precedence() const { return CMP_PRECEDENCE; } > bool fix_fields(THD *thd, Item **ref); > - void fix_length_and_dec() > + bool fix_length_and_dec() > { > max_length= 1; > - agg_arg_charsets_for_comparison(cmp_collation, args, 2); should this one also be __attribute__ ((warn_unused_result)) ? > + return agg_arg_charsets_for_comparison(cmp_collation, args, 2); > } > void cleanup(); > > diff --git a/sql/item_subselect.cc b/sql/item_subselect.cc > index 4214980c36c..ca49e06a9e7 100644 > --- a/sql/item_subselect.cc > +++ b/sql/item_subselect.cc > @@ -904,9 +905,10 @@ Item::Type Item_subselect::type() const > } > > > -void Item_subselect::fix_length_and_dec() > +bool Item_subselect::fix_length_and_dec() > { > engine->fix_length_and_dec(0); what about subselect_engine::fix_length_and_dec() ? should return bool too, I guess, right? > + return FALSE; > } > > > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 9e7973b745c..f3cb85f01d3 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -4894,7 +4894,13 @@ int create_table_impl(THD *thd, > file= mysql_create_frm_image(thd, orig_db, orig_table_name, create_info, > alter_info, create_table_mode, key_info, > key_count, frm); > - if (!file) > + /* > + We have to check thd->is_error() here because it can be set by > + Item::val* for example, and before it will be cought accidentally by > + Item_func::fix_fields() of the next call. Now we removed the check > + from Item_func::fix_fields() > + */ this is a very confusing comment, don't write in comments that "it used to be this way, but now we've changed it and it is that way" and I still don't understand why do you need to check for thd->is_error() here > + if (!file || thd->is_error()) > goto err; > if (rea_create_table(thd, frm, path, db, table_name, create_info, > file, frm_only)) > @@ -7377,7 +7383,8 @@ static bool mysql_inplace_alter_table(THD *thd, > */ > if (mysql_rename_table(db_type, alter_ctx->new_db, alter_ctx->tmp_name, > alter_ctx->db, alter_ctx->alias, > - FN_FROM_IS_TMP | NO_HA_TABLE)) > + FN_FROM_IS_TMP | NO_HA_TABLE) || > + thd->is_error()) and here > { > // Since changes were done in-place, we can't revert them. > (void) quick_rm_table(thd, db_type, Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 6

Re: [Maria-developers] 0918ae42a83: MDEV-14014 Multi-Slave Replication Fail: bogus data in log event
by Sergei Golubchik 13 Jun '18

13 Jun '18

Hi, Andrei! On Jun 13, andrei.elkin(a)pp.inet.fi wrote: > revision-id: 0918ae42a8394b6faf28f67b70550894851be3bb (mariadb-10.1.33-36-g0918ae42a83) > parent(s): 26be5072429082e634b8fc102609370975443439 > author: Andrei Elkin > committer: Andrei Elkin > timestamp: 2018-06-13 16:27:00 +0300 > message: > > MDEV-14014 Multi-Slave Replication Fail: bogus data in log event > > MDEV-7257 made a dump thread to read from binlog concurrently with > writers as long as the read bytes are below a water-mark > (MYSQL_BIN_LOG::binlog_end_pos). However it appeared to be possible a > dump thread reader reach out for bytes past the water mark through a > feature of IO_CACHE that fills in the internal buffer and while doing > so it could read what the reader is not supposed to see (the bytes > above MYSQL_BIN_LOG::binlog_end_pos). > > The issue is fixed with constraining the IO_CACHE buffer fill to respect > the watermark. > An added test simulates potentially unconstrained buffer fill and an > assert guards this is not the case anymore. Looks good, thanks! Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] unbundle RocksDB
by Michal Schorm 13 Jun '18

13 Jun '18

Hello, Since we have a rocksdb package ready in Fedora, I tried to unbundle it, using the system package instead. I wasn't successful though. I haven't found any CMake argument which seemed related and from the build log it seems, the code tries to link against the static libraries in any case. Before I'll start to dig further, I'm stopping by on this list, making sure it is possible. - is it currently supported? - have I overlooked any related CMake argument? Links: MariaDB in Fedora: https://src.fedoraproject.org/rpms/mariadb/tree/master -- Michal Schorm Associate Software Engineer Core Services - Databases Team Red Hat

3 3

Re: [Maria-developers] d9913834ceb: MDEV-14014 Multi-Slave Replication Fail: bogus data in log event
by Sergei Golubchik 13 Jun '18

13 Jun '18

Hi, andrei.elkin! On May 30, andrei.elkin(a)pp.inet.fi wrote: > revision-id: d9913834cebe4ed8494b3d76762cd882788a8fd5 (mariadb-10.1.33-28-gd9913834ceb) > parent(s): c1698e8dc50f0c342c1a6886426ba1d43396cd1e > author: Andrei Elkin > committer: Andrei Elkin > timestamp: 2018-05-30 20:06:44 +0300 > message: > > MDEV-14014 Multi-Slave Replication Fail: bogus data in log event > > MDEV-7257 made a dump thread to read from binlog concurrently with > writers as long as the read bytes are below a water-mark > (MYSQL_BIN_LOG::binlog_end_pos). However it appeared to be possible a > dump thread reader reach out for bytes past the water mark through a > feature of IO_CACHE that fills in the internal buffer and while doing > so it could read what the reader is not supposed to see (the bytes > above MYSQL_BIN_LOG::binlog_end_pos). > > The issue is fixed with constraining the IO_CACHE buffer fill to respect > the watermark. > An added test simulates potentially unconstrained buffer fill and an > assert guards this is not the case anymore. What about a much simpler one-liner fix: --- a/sql/sql_repl.cc +++ b/sql/sql_repl.cc @@ -2549,6 +2549,7 @@ static int send_events(binlog_send_info *info, IO_CACHE* return 1; info->last_pos= linfo->pos; + log->end_of_file= end_pos; error= Log_event::read_log_event(log, packet, info->fdev, opt_master_verify_checksum ? info->current_checksum_alg : BINLOG_CHECKSUM_ALG_OFF); It doesn't change IO_CACHE structure and doesn't add any overhead to the IO_CACHE. Did you already test that your commit actually fixes the issue? Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 3

Re: [Maria-developers] 20f3479: MDEV-15890 Strange error message if you try to FLUSH TABLES <view> after LOCK TABLES <view>.
by Sergei Golubchik 12 Jun '18

12 Jun '18

Hi, Alexey! On Jun 11, Alexey Botchkov wrote: > revision-id: 20f347974f6a9b4e2c65a5ca79aefb8e118ba6cd (mariadb-10.1.33-33-g20f3479) > parent(s): 3627dd7f6a1cf5bd7151ff23290d64cb6dffea90 > committer: Alexey Botchkov > timestamp: 2018-06-11 14:58:46 +0400 > message: > > MDEV-15890 Strange error message if you try to FLUSH TABLES <view> after LOCK TABLES <view>. > > The reload_acl_and_cache() now looks into the VIEW definition > to check the involved tables if the locked_tables_mode. That's a pretty neat solution :) But it seems that - contrary to what I expected - that MariaDB actually distinguishes between locking a view a locking all its tables. Consider this test case: ---- create table t1 (a int); create view v1 as select * from t1; lock table v1 write; insert v1 values (1); select * from t1; unlock tables; drop view v1; drop table t1; ---- this works and inserts 1 just fine. If you change the lock table statement to lock t1, it'll with Table 'v1' was not locked with LOCK TABLES It'd be great if FLUSH TABLES will exhibit the same behavior, it needs to know if the view is locked, not only if all underlying tables are locked. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0