- developers - lists.mariadb.org

Re: [Maria-developers] a4be420c920: MDEV-19235 MariaDB Server compiled for 128 Indexes crashes at startup
by Sergei Golubchik 09 May '19

09 May '19

Hi, Vladislav! Thanks. That's pretty much the solution I had in mind too :) A couple of comments on details below. I've only commented on sql_bitmap.h, everything else is fine. On May 08, Vladislav Vaintroub wrote: > revision-id: a4be420c920 (mariadb-10.4.4-25-ga4be420c920) > parent(s): ee4a2fef181 > author: Vladislav Vaintroub <wlad(a)mariadb.com> > committer: Vladislav Vaintroub <wlad(a)mariadb.com> > timestamp: 2019-04-17 17:17:06 +0100 > message: > > MDEV-19235 MariaDB Server compiled for 128 Indexes crashes at startup > > With MAX_INDEXIES=64(default), key_map=Bitmap<64> is just a wrapper around > ulonglong and thus "trivial" (can be bzero-ed, or memcpy-ed, and stays > valid still) > > With MAX_INDEXES=128, key_map = Bitmap<128> is not a "trivial" type > anymore. The implementation uses MY_BITMAP, and MY_BITMAP contains pointers > which make Bitmap invalid, when it is memcpy-ed/bzero-ed. > > The problem in 10.4 is that there are many new key_map members, inside TABLE > or KEY, and those are often memcopied and bzeroed > > The fix makes Bitmap "trivial", by inlining most of MY_BITMAP functionality. > pointers/heap allocations are not used anymore. > diff --git a/sql/sql_bitmap.h b/sql/sql_bitmap.h > index 705a8d169e1..74866336a1a 100644 > --- a/sql/sql_bitmap.h > +++ b/sql/sql_bitmap.h > @@ -25,73 +25,184 @@ > > #include <my_sys.h> > #include <my_bitmap.h> > +#include <my_bit.h> > > template <uint width> class Bitmap > { > - MY_BITMAP map; > +public: > uint32 buffer[(width+31)/32]; Why is it public? > public: > - Bitmap() { init(); } > + Bitmap():buffer() What does that do? > + { > + } > - Bitmap(const Bitmap& from) { *this=from; } > - explicit Bitmap(uint prefix_to_set) { init(prefix_to_set); } > + explicit Bitmap(uint prefix_to_set):buffer() > + { > + set_prefix(prefix_to_set); > + } > - void init() { my_bitmap_init(&map, buffer, width, 0); } > - void init(uint prefix_to_set) { init(); set_prefix(prefix_to_set); } > uint length() const { return width; } > - Bitmap& operator=(const Bitmap& map2) > - { > + void init() > + { > + clear_all(); > + } > + void init(uint prefix_to_set) > + { > init(); > - memcpy(buffer, map2.buffer, sizeof(buffer)); > - return *this; > + set_prefix(prefix_to_set); I wonder whether init() method is still needed. You've removed it from the constructor. And if there's any other code that uses it, I'd rather it'd be using clear_all(). > } > - void set_bit(uint n) { bitmap_set_bit(&map, n); } > + void set_bit(uint n) > + { > + DBUG_ASSERT(n < width); > + ((uchar*)buffer)[n / 8] |= (1 << (n & 7)); > + } > - void clear_bit(uint n) { bitmap_clear_bit(&map, n); } > + void clear_bit(uint n) > + { > + DBUG_ASSERT(n < width); > + ((uchar*)buffer)[n / 8] &= ~(1 << (n & 7)); > + } > - void set_prefix(uint n) { bitmap_set_prefix(&map, n); } > + void set_prefix(uint prefix_size) > + { > + set_if_smaller(prefix_size, width); > + uint prefix_bytes, prefix_bits, d; > + uchar* m = (uchar*)buffer; > + > + if ((prefix_bytes = prefix_size / 8)) > + memset(m, 0xff, prefix_bytes); > + m += prefix_bytes; > + if ((prefix_bits = prefix_size & 7)) > + { > + *(m++) = (1 << prefix_bits) - 1; > + // As the prefix bits are set, lets count this byte too as a prefix byte. > + prefix_bytes++; > + } > + if ((d = (width + 7) / 8 - prefix_bytes)) > + memset(m, 0, d); > + } > - void set_all() { bitmap_set_all(&map); } > + void set_all() > + { > + set_prefix(width); may be better to optimize it? memset(buffer, 0xff, sizeof(buffer) - 1); ((uchar*)buffer)[sizeof(buffer)-1] = last_byte_mask(width); > + } > - void clear_all() { bitmap_clear_all(&map); } > + void clear_all() > + { > + memset(buffer, 0x00, sizeof(buffer)); > + } > - void intersect(Bitmap& map2) { bitmap_intersect(&map, &map2.map); } > + void intersect(Bitmap & map2) > + { > + for (uint i = 0; i < array_elements(buffer); i++) > + buffer[i] &= map2.buffer[i]; > + } > - void intersect(ulonglong map2buff) > - { > - // Use a spearate temporary buffer, as bitmap_init() clears all the bits. > - ulonglong buf2; > - MY_BITMAP map2; > - > - my_bitmap_init(&map2, (uint32 *) &buf2, sizeof(ulonglong) * 8, 0); > - > - // Store the original bits. > - if (sizeof(ulonglong) >= 8) > - { > - int8store(const_cast<uchar *>(static_cast<uchar *> > - (static_cast<void *>(&buf2))), > - map2buff); > - } > - else > - { > - DBUG_ASSERT(sizeof(buffer) >= 4); > - int4store(const_cast<uchar *>(static_cast<uchar *> > - (static_cast<void *>(&buf2))), > - static_cast<uint32>(map2buff)); > - } > - bitmap_intersect(&map, &map2); > - } > + void intersect_and_pad(ulonglong map2buff, bool pad_with_ones) make it private? > + { > + compile_time_assert(sizeof(ulonglong) == 8); > + uint32 tmp[2]; > + int8store(tmp, map2buff); > + if (array_elements(buffer) > 2 && pad_with_ones) > + set_all(); really? I'd expect that if pad_with_ones==true, you simply don't need to do anything to the rest of the buffer. > + > + buffer[0] &= tmp[0]; > + if (array_elements(buffer) > 1) > + buffer[1] &= tmp[1]; > + > + if (array_elements(buffer) > 2 && !pad_with_ones) > + memset((char*)buffer + 8, 0 , sizeof(buffer) - 8); > + } > + void intersect(ulonglong map2buff) > + { > + intersect_and_pad(map2buff, 0); > + } > /* Use highest bit for all bits above sizeof(ulonglong)*8. */ > void intersect_extended(ulonglong map2buff) > { > - intersect(map2buff); > - if (map.n_bits > sizeof(ulonglong) * 8) > - bitmap_set_above(&map, sizeof(ulonglong), > - MY_TEST(map2buff & (1LL << (sizeof(ulonglong) * 8 - 1)))); > + intersect_and_pad(map2buff, (map2buff & (1ULL << 63))); > } > - void subtract(Bitmap& map2) { bitmap_subtract(&map, &map2.map); } > + void subtract(Bitmap & map2) > + { > + for (size_t i = 0; i < array_elements(buffer); i++) > + buffer[i] &= ~(map2.buffer[i]); > + } > - void merge(Bitmap& map2) { bitmap_union(&map, &map2.map); } > + void merge(Bitmap & map2) > + { > + for (size_t i = 0; i < array_elements(buffer); i++) > + buffer[i] |= map2.buffer[i]; > + } > - bool is_set(uint n) const { return bitmap_is_set(&map, n); } > + bool is_set(uint n) const > + { > + DBUG_ASSERT(n < width); > + return ((uchar*)buffer)[n / 8] & (1 << (n & 7)); > + } > - bool is_prefix(uint n) const { return bitmap_is_prefix(&map, n); } > + bool is_prefix(uint prefix_size) const > + { > + uint prefix_mask = last_byte_mask(prefix_size); > + uchar* m = (uchar*)buffer; > + uchar* end_prefix = m + (prefix_size - 1) / 8; > + uchar* end; > + DBUG_ASSERT(prefix_size <= width); > + > + /* Empty prefix is always true */ > + if (!prefix_size) > + return true; > + > + while (m < end_prefix) > + if (*m++ != 0xff) > + return false; > + > + end = ((uchar*)buffer) + (width + 7) / 8 - 1; > + if (m == end) > + return ((*m & last_byte_mask(width)) == prefix_mask); > + > + if (*m != prefix_mask) > + return false; > + > + while (++m < end) > + if (*m != 0) > + return false; > + return ((*m & last_byte_mask(width)) == 0); > + } > - bool is_clear_all() const { return bitmap_is_clear_all(&map); } > + bool is_clear_all() const > + { > + for (size_t i= 0; i < array_elements(buffer); i++) > + if (buffer[i]) > + return false; > + return true; > + } > - bool is_set_all() const { return bitmap_is_set_all(&map); } > + bool is_set_all() const > + { > + if (width == sizeof(buffer) * 8) > + { > + for (size_t i = 0; i < array_elements(buffer); i++) > + if (buffer[i] != 0xFFFFFFFFU) > + return false; > + return true; > + } > + else > + return is_prefix(width); this can also be optimized to not use complex is_prefix(), but I don't think is_set_all() is used as often as set_all(), so it shouldn't matter as much. > + } > - bool is_subset(const Bitmap& map2) const { return bitmap_is_subset(&map, &map2.map); } > + bool is_subset(const Bitmap & map2) const > + { > + for (size_t i= 0; i < array_elements(buffer); i++) > + if (buffer[i] & ~(map2.buffer[i])) > + return false; > + return true; > + } > - bool is_overlapping(const Bitmap& map2) const { return bitmap_is_overlapping(&map, &map2.map); } > + bool is_overlapping(const Bitmap & map2) const > + { > + for (size_t i = 0; i < array_elements(buffer); i++) > + if (buffer[i] & map2.buffer[i]) > + return true; > + return false; > + } > - bool operator==(const Bitmap& map2) const { return bitmap_cmp(&map, &map2.map); } > + bool operator==(const Bitmap & map2) const > + { > + return memcmp(buffer, map2.buffer, sizeof(buffer)) == 0; > + } > - bool operator!=(const Bitmap& map2) const { return !(*this == map2); } > + bool operator!=(const Bitmap & map2) const > + { > + return !(*this == map2); > + } > char *print(char *buf) const > { > char *s=buf; Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 2

Re: [Maria-developers] [Commits] 0ff8b73b60f: MDEV-15777:Support Early NULLs filtering-like restrictions in the range optimizer
by Sergey Petrunia 06 May '19

06 May '19

Hi Varun, Please finally find the review below. The patch is almost there I think, but there are some minor adjustments that still need to be made. > revision-id: 0ff8b73b60fae40556f91d3959f20bf3cf182b28 (mariadb-10.3.0-947-g0ff8b73b60f) > parent(s): 15419a558370aeed9521b498c34d50f20a8d47a5 > author: Varun Gupta > committer: Varun Gupta > timestamp: 2018-05-15 13:53:11 +0530 > message: > > MDEV-15777:Support Early NULLs filtering-like restrictions in the range optimizer > > --- > mysql-test/main/mdev15777.result | 455 +++++++++++++++++++++++++++++++++++++++ > mysql-test/main/mdev15777.test | 138 ++++++++++++ > sql/opt_range.cc | 125 ++++++++++- > sql/opt_range.h | 2 + > sql/sql_select.cc | 22 +- > sql/table.cc | 1 + > sql/table.h | 6 + > 7 files changed, 745 insertions(+), 4 deletions(-) > > diff --git a/mysql-test/main/mdev15777.result b/mysql-test/main/mdev15777.result > new file mode 100644 > index 00000000000..549ac08b91f > --- /dev/null > +++ b/mysql-test/main/mdev15777.result > @@ -0,0 +1,455 @@ > +# Test added to check that null filtering is used by range optimizer > +create table ten(a int); > +insert into ten values (0),(1),(2),(3),(4),(5),(6),(7),(8),(9); > +create table one_k(a int); > +insert into one_k select A.a + B.a* 10 + C.a * 100 from ten A, ten B, ten C; > +create table one_m(a int); > +insert into one_m select A.a + B.a* 1000 from one_k A, one_k B; The testcases are extremely confusing. * Do you really need a table with one millon rows to demonstrate the issue? * Why use examples with subqueries (which are converted into join anyway)? - if we want to verify that this optimization works together with subquery-to-semi-join conversion, let's start with a join example, and then construct a subquery which is converted to the query. * The testcases produce a lot of EXPLAIN outputs, and there is no explanation provided what one should expect. Please create a testcase: - of reasonable size (I think tables of 10-20K rows should suffice) - start from a join query - Like in other testcases, tables should be named, t1,t2, ... - I assume the query plan should be: range access on the first table (constructed from a NOT NULL predicate), ref access for the second one. - The testcase should have '--echo #' comments explaining what is relevant in the query plan > +delete from one_m where a=0 limit 1; > +create table t1 ( > +id int(10) unsigned NOT NULL AUTO_INCREMENT, > +filler varchar(100), > +subset_id int(11) DEFAULT NULL, > +PRIMARY KEY (id), > +KEY t1_subset_id (subset_id) > +); > +create table t1_subsets ( ... > diff --git a/sql/opt_range.cc b/sql/opt_range.cc > index 8422917065f..ae06b899ac9 100644 > --- a/sql/opt_range.cc > +++ b/sql/opt_range.cc > @@ -155,6 +155,7 @@ class SEL_IMERGE; > #define CLONE_KEY1_MAYBE 1 > #define CLONE_KEY2_MAYBE 2 > #define swap_clone_flag(A) ((A & 1) << 1) | ((A & 2) >> 1) > +#define FT_KEYPART (MAX_FIELDS+10) > > > /* > @@ -2400,6 +2401,7 @@ int SQL_SELECT::test_quick_select(THD *thd, key_map keys_to_use, > { > uint idx; > double scan_time; > + Item *null_rejecting_conds= NULL; > DBUG_ENTER("SQL_SELECT::test_quick_select"); > DBUG_PRINT("enter",("keys_to_use: %lu prev_tables: %lu const_tables: %lu", > (ulong) keys_to_use.to_ulonglong(), (ulong) prev_tables, > @@ -2423,6 +2425,7 @@ int SQL_SELECT::test_quick_select(THD *thd, key_map keys_to_use, > read_time= (double) records + scan_time + 1; // Force to use index > > possible_keys.clear_all(); > + null_rejecting_conds= head->null_rejecting_conds; > > DBUG_PRINT("info",("Time to scan table: %g", read_time)); > > @@ -2431,7 +2434,7 @@ int SQL_SELECT::test_quick_select(THD *thd, key_map keys_to_use, > { > uchar buff[STACK_BUFF_ALLOC]; > MEM_ROOT alloc; > - SEL_TREE *tree= NULL; > + SEL_TREE *tree= NULL, *not_null_cond_tree= NULL; > KEY_PART *key_parts; > KEY *key_info; > PARAM param; > @@ -2540,6 +2543,12 @@ int SQL_SELECT::test_quick_select(THD *thd, key_map keys_to_use, > TRP_GROUP_MIN_MAX *group_trp; > double best_read_time= read_time; > Please add a comment saying that as far as range optimizer is concerned, null_rejecting_conds is just an extra condition on this table. (That is, we don't care if it has NOT NULL conditions or something else). > + if (null_rejecting_conds) > + { > + not_null_cond_tree= null_rejecting_conds->get_mm_tree(&param, > + &null_rejecting_conds); > + } > + > if (cond) > { > if ((tree= cond->get_mm_tree(&param, &cond))) > @@ -2558,6 +2567,13 @@ int SQL_SELECT::test_quick_select(THD *thd, key_map keys_to_use, > tree= NULL; > } > } > + if (not_null_cond_tree) > + { > + if (!tree) > + tree= not_null_cond_tree; > + else > + tree= tree_and(&param, tree, not_null_cond_tree); This if-else logic is redundant, as it's already present in tree_and(). Can just have this: tree= tree_and(tree, not_null_cond_tree); > + } > > /* > Try to construct a QUICK_GROUP_MIN_MAX_SELECT. > @@ -14643,6 +14659,113 @@ void QUICK_GROUP_MIN_MAX_SELECT::add_keys_and_lengths(String *key_names, > add_key_and_length(key_names, used_lengths, &first); > } > > +inline void add_cond(THD *thd, Item **e1, Item *e2) > +{ > + if (*e1) > + { > + if (!e2) > + return; > + Item *res; > + if ((res= new (thd->mem_root) Item_cond_and(thd, *e1, e2))) > + { > + res->fix_fields(thd, 0); > + res->update_used_tables(); > + *e1= res; > + } > + } > + else > + *e1= e2; > +} > + > +/* > + Create null rejecting conditions for a table, for all the equalites > + present in the WHERE clause of a query. > + > + SYNOPSIS > + make_null_rejecting_conds() > + @param TABLE - Keys of this table will participate in null > + rejecting conditions > + @param keyuse_array - array that has all the equalites of the > + WHERE clasuse > + > + DESCRIPTION > + This function creates null rejecting conditions for a table. These > + conditions are created to do range analysis on them , the conditions > + are of the form tbl.key.keypart IS NOT NULL. > + > + IMPLEMENTATION > + Lookup in the keyuse array to check if it has equalites that belong > + to the given table. If yes then find out if the conditions are null > + rejecting and accordingly create all the condition for the keys of a > + given table and AND them. > + > + > + RETURN > + NOT NULL - Found null rejecting conditions for the given table > + NULL - No null rejecting conditions for the given table > +*/ > + > +void make_null_rejecting_conds(THD *thd, TABLE *table, > + DYNAMIC_ARRAY *keyuse_array, key_map *const_keys) ^ Please fix identation. > +{ > + KEY *keyinfo; > + Item *cond= NULL; > + KEYUSE* keyuse; > + > + /* > + The null rejecting conds added will be on the keypart of a key, so for > + that we need the table to atleast have a key. > + */ please fix typos/wording in the above comment. > + if (!table->s->keys) > + return ; > + if (table->null_rejecting_conds) > + return; > + > + for(uint i=0; i < keyuse_array->elements; i++) > + { This loop does excessive work. Check out how best_access_path() examines the KEYUSE array. Key points - the array is sorted by table, index_nr, key_part_nr. - JOIN_TAB::keyuse points to the first element that refers to the table described by the JOIN_TAB - The array may have "duplicates", especially when multiple equalities are present: KEYUSE(t.keyXpartY = other_table.colX) KEYUSE(t.keyXpartY = other_table.colY) KEYUSE(t.keyXpartY = some_expr) ... Since the elements are sorted by keyXpartY, it is trival to avoid generating multiple "t.keyXpartY IS NOT NULL" for consecutive KEYUSEs. I would go even further: the same "t.keyXpartY" can be part of multiple indexes (and so its KEYUSE objects will not be adjacent). Can we use a table's column bitmap to avoid generating duplicate IS NOT NULL objects? > + keyuse= (KEYUSE*)dynamic_array_ptr(keyuse_array, i); > + if (keyuse->table == table) > + { > + /* > + No null rejecting conds for a hash key orr full-text keys > + */ please fix typos/wording in the above comment. > + if (keyuse->key == MAX_KEY || keyuse->keypart == FT_KEYPART) > + continue; > + keyinfo= keyuse->table->key_info+keyuse->key; > + Field *field= keyinfo->key_part[keyuse->keypart].field; > + > + /* > + No need to add null-rejecting condition if we have a > + keyuse element as > + - table.key.keypart= const > + - (table.key.keypart= tbl.otherfield or table.key.keypart IS NULL) > + - table.key.keypart IS NOT NULLABLE > + */ > + > + if (keyuse->val->const_item() > + || !(keyuse->null_rejecting && field->maybe_null()) Please remove the brackets: !(A && B) -> !A || !B > + || keyuse->optimize & KEY_OPTIMIZE_REF_OR_NULL) Please also put the '||' at the end of the lines, like it is done in the rest of the code. > + continue; > + > + Item_field *field_item= new (thd->mem_root)Item_field(thd, field); > + Item* not_null_item= new (thd->mem_root)Item_func_isnotnull(thd, > + field_item); Please add error handling (just return from the function if we failed to allocate any of the above) > + > + /* > + adding the key to const keys as we have the condition > + as key.keypart IS NOT NULL > + */ > + > + const_keys->set_bit(keyuse->key); > + not_null_item->fix_fields(thd, 0); > + not_null_item->update_used_tables(); > + add_cond(thd, &cond, not_null_item); > + } > + } > + table->null_rejecting_conds= cond; > + return; Please remove the above 'return;' as it is redundant and makes one suspect a merge error > +} > + > > #ifndef DBUG_OFF > > diff --git a/sql/opt_range.h b/sql/opt_range.h > index bd85a12d4a1..894d46a892c 100644 > --- a/sql/opt_range.h > +++ b/sql/opt_range.h > @@ -1728,6 +1728,8 @@ bool calculate_cond_selectivity_for_table(THD *thd, TABLE *table, Item **cond); > bool prune_partitions(THD *thd, TABLE *table, Item *pprune_cond); > #endif > void store_key_image_to_rec(Field *field, uchar *ptr, uint len); > +void make_null_rejecting_conds(THD *thd, TABLE *table, > + DYNAMIC_ARRAY *keyuse_array, key_map *const_keys); Please fix identation. > > extern String null_string; > > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index f658b78c33c..4aceb333340 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -4805,6 +4805,9 @@ make_join_statistics(JOIN *join, List<TABLE_LIST> &tables_list, > add_group_and_distinct_keys(join, s); > > s->table->cond_selectivity= 1.0; > + > + make_null_rejecting_conds(join->thd, s->table, > + keyuse_array, &s->const_keys); > > /* > Perform range analysis if there are keys it could use (1). > @@ -5352,15 +5356,24 @@ add_key_field(JOIN *join, > If the condition has form "tbl.keypart = othertbl.field" and > othertbl.field can be NULL, there will be no matches if othertbl.field > has NULL value. > + > + The field KEY_FIELD::null_rejecting is set to TRUE if we have both > + the left and right hand side of the equality are NULLABLE > + This is NOT what is happening. null_rejecting is set to true, if *either* left or right-hand side of the equality is nullable. Please fix the comment. Please also fix the comment in KEY_FIELD::null_rejecting to reflect that. > We use null_rejecting in add_not_null_conds() to add > 'othertbl.field IS NOT NULL' to tab->select_cond. > + > + We use null_rejecting in make_null_rejecting_conds() to add > + tbl.keypart IS NOT NULL so we can do range analysis on this condition > + Please join the above two sentences together: null_rejecting is used - by add_not_null_conds(), to add "othertbl.field IS NOT NULL" to othertbl's tab->select_cond. (This is called "Early NULLs filtering") - by make_null_rejecting_conds(), to provide range optimizer with additional "tbl.keypart IS NOT NULL" condition. > */ > { > Item *real= (*value)->real_item(); > if (((cond->functype() == Item_func::EQ_FUNC) || > (cond->functype() == Item_func::MULT_EQUAL_FUNC)) && > - (real->type() == Item::FIELD_ITEM) && > + (((real->type() == Item::FIELD_ITEM) && > ((Item_field*)real)->field->maybe_null()) > + ||(field->maybe_null()))) ^Please fix the identation. > (*key_fields)->null_rejecting= true; > else > (*key_fields)->null_rejecting= false; > @@ -9813,7 +9826,10 @@ static bool create_ref_for_key(JOIN *join, JOIN_TAB *j, > uint maybe_null= MY_TEST(keyinfo->key_part[i].null_bit); > j->ref.items[i]=keyuse->val; // Save for cond removal > j->ref.cond_guards[i]= keyuse->cond_guard; > - if (keyuse->null_rejecting) > + Item *real= (keyuse->val)->real_item(); > + if (keyuse->null_rejecting && > + (real->type() == Item::FIELD_ITEM) && > + ((Item_field*)real)->field->maybe_null()) ^ Please fix identation > j->ref.null_rejecting|= (key_part_map)1 << i; > keyuse_uses_no_tables= keyuse_uses_no_tables && !keyuse->used_tables; > /* > @@ -18538,7 +18554,7 @@ free_tmp_table(THD *thd, TABLE *entry) > DBUG_ASSERT(entry->pos_in_table_list->table == entry); > entry->pos_in_table_list->table= NULL; > } > - > + entry->null_rejecting_conds= NULL; What is this for? As far as I understand we are freeing the TABLE object > free_root(&own_root, MYF(0)); /* the table is allocated in its own root */ > thd_proc_info(thd, save_proc_info); > > diff --git a/sql/table.cc b/sql/table.cc > index a6e445d0d2e..097fee465de 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -4597,6 +4597,7 @@ void TABLE::init(THD *thd, TABLE_LIST *tl) > created= TRUE; > cond_selectivity= 1.0; > cond_selectivity_sampling_explain= NULL; > + null_rejecting_conds= NULL; > #ifdef HAVE_REPLICATION > /* used in RBR Triggers */ > master_had_triggers= 0; > diff --git a/sql/table.h b/sql/table.h > index 6fd3f219914..50d47899b49 100644 > --- a/sql/table.h > +++ b/sql/table.h > @@ -1356,6 +1356,12 @@ struct TABLE > SplM_opt_info *spl_opt_info; > key_map keys_usable_for_splitting; > > + /* > + Null rejecting conds added for all tables so we can do range analysis > + on these conditions > + */ "for all tables"? Please change the comment to something like: "NULL-rejecting conditions for this table. They are created from eq_ref access, and used by the range optimizer". > + Item* null_rejecting_conds; > + > void init(THD *thd, TABLE_LIST *tl); > bool fill_item_list(List<Item> *item_list) const; > void reset_item_list(List<Item> *item_list, uint skip) const; BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

[Maria-developers] [MDEV-18684] [5.5] memory leak
by Michal Schorm 04 May '19

04 May '19

Hello, can somebody please take a brief look to atleast evaulate, how hard it would be to backoport the patch form the latest library versions? https://jira.mariadb.org/browse/MDEV-18684 Thank you -- Michal Schorm Software Engineer Core Services - Databases Team Red Hat --

1 0

Re: [Maria-developers] [Commits] 5e23f561984: MDEV-18741: Optimizer trace: multi-part key ranges are printed incorrectly
by Sergey Petrunia 03 May '19

03 May '19

Hi Varun, Please find my review comments below. On Fri, Apr 05, 2019 at 02:59:17PM +0530, Varun wrote: > revision-id: 5e23f56198493ed19ac270759963f615324b2c49 (mariadb-10.4.3-162-g5e23f561984) > parent(s): 02d9b048a2ab549a3227a81e15ff2f8c45562a65 > author: Varun Gupta > committer: Varun Gupta > timestamp: 2019-04-05 14:58:39 +0530 > message: > > MDEV-18741: Optimizer trace: multi-part key ranges are printed incorrectly > > Changed the function append_range_all_keyparts to use sel_arg_range_seq_init / sel_arg_range_seq_next to produce ranges. > Also adjusted to print format for the ranges, now the ranges are printed as: > (keypart1_min, keypart2_min,..) OP (keypart1_name,keypart2_name, ..) OP (keypart1_max,keypart2_max, ..) > > Also added more tests for range and index merge access for optimizer trace > > --- > mysql-test/main/opt_trace.result | 276 ++++++++++- > mysql-test/main/opt_trace.test | 57 +++ > mysql-test/main/opt_trace_index_merge.result | 509 ++++++++++++++++++++- > mysql-test/main/opt_trace_index_merge.test | 112 +++++ > .../main/opt_trace_index_merge_innodb.result | 6 +- > sql/opt_range.cc | 281 ++++++------ > sql/opt_range.h | 7 +- > 7 files changed, 1079 insertions(+), 169 deletions(-) ... > +explain select * from t1 force index(a_b_c) where a between 1 and 4 and b < 50; > +id select_type table type possible_keys key key_len ref rows Extra > +1 SIMPLE t1 range a_b_c a_b_c 8 NULL 4 Using index condition > +select JSON_DETAILED(JSON_EXTRACT(trace, '$**.analyzing_range_alternatives')) from INFORMATION_SCHEMA.OPTIMIZER_TRACE; > +JSON_DETAILED(JSON_EXTRACT(trace, '$**.analyzing_range_alternatives')) > +[ > + > + { > + "range_scan_alternatives": > + [ > + > + { > + "index": "a_b_c", > + "ranges": > + [ > + "(1) <= (a) < (4,50)" In the example above, only the name of the first key part is printed, even if the second endpoint uses two. As far as I understand the code, just using range->start_key.keypart_map | range->end_key.keypart_map in print_keyparts_name will be sufficient to fix this. > + ], > + "rowid_ordered": false, > + "using_mrr": false, > + "index_only": false, > + "rows": 4, > + "cost": 6.2648, > + "chosen": true > + } > + ], > + "analyzing_roworder_intersect": > + { > + "cause": "too few roworder scans" > + }, > + "analyzing_index_merge_union": > + [ > + ] > + } > +] > diff --git a/sql/opt_range.cc b/sql/opt_range.cc > index 5ab3d70214d..1827e2fcaf2 100644 > --- a/sql/opt_range.cc > +++ b/sql/opt_range.cc > @@ -431,16 +431,18 @@ static int and_range_trees(RANGE_OPT_PARAM *param, > static bool remove_nonrange_trees(RANGE_OPT_PARAM *param, SEL_TREE *tree); > > static void print_key_value(String *out, const KEY_PART_INFO *key_part, > - const uchar *key); > + const uchar* key, uint length); > +static void print_keyparts_name(String *out, const KEY_PART_INFO *key_part, > + uint n_keypart, key_part_map keypart_map); > > static void append_range_all_keyparts(Json_writer_array *range_trace, > - String *range_string, > - String *range_so_far, const SEL_ARG *keypart, > + PARAM param, uint idx, > + SEL_ARG *keypart, > const KEY_PART_INFO *key_parts); > > static > -void append_range(String *out, const KEY_PART_INFO *key_parts, > - const uchar *min_key, const uchar *max_key, const uint flag); > +void append_range(String *out, const KEY_PART_INFO *key_part, > + KEY_MULTI_RANGE *range, uint n_key_parts); Why is this function called append_() while other functions that print stuff into strings are called print_key_value, print_keyparts_name ? The name "append_range_all_keyparts" is also confusing. The function prints a sequence of ranges. One can say it "appends" them to the trace, but that verb is not used to refer to this action anywhere else. Functions that print something into trace are called trace_XXX(), e.g. TRP_RANGE::trace_basic_info(), trace_plan_prefix(), etc. I think trace_ranges() would be a better name here. What do you think? > > > /* > @@ -2273,10 +2275,7 @@ void TRP_RANGE::trace_basic_info(const PARAM *param, > // TRP_RANGE should not be created if there are no range intervals > DBUG_ASSERT(key); > > - String range_info; > - range_info.length(0); > - range_info.set_charset(system_charset_info); > - append_range_all_keyparts(&trace_range, NULL, &range_info, key, key_part); > + append_range_all_keyparts(&trace_range, *param, key_idx, key, key_part); > } > > > @@ -2489,10 +2488,8 @@ void TRP_GROUP_MIN_MAX::trace_basic_info(const PARAM *param, > // can have group quick without ranges > if (index_tree) > { > - String range_info; > - range_info.set_charset(system_charset_info); > - append_range_all_keyparts(&trace_range, NULL, &range_info, index_tree, > - key_part); > + append_range_all_keyparts(&trace_range, *param, param_idx, > + index_tree, key_part); > } > } > > @@ -6398,20 +6395,9 @@ void TRP_ROR_INTERSECT::trace_basic_info(const PARAM *param, > trace_isect_idx.add("rows", (*cur_scan)->records); > > Json_writer_array trace_range(thd, "ranges"); > - for (const SEL_ARG *current= (*cur_scan)->sel_arg->first(); current; > - current= current->next) > - { > - String range_info; > - range_info.set_charset(system_charset_info); > - for (const SEL_ARG *part= current; part; > - part= part->next_key_part ? part->next_key_part : nullptr) > - { > - const KEY_PART_INFO *cur_key_part= key_part + part->part; > - append_range(&range_info, cur_key_part, part->min_value, > - part->max_value, part->min_flag | part->max_flag); > - } > - trace_range.add(range_info.ptr(), range_info.length()); > - } > + > + append_range_all_keyparts(&trace_range, *param, (*cur_scan)->idx, > + (*cur_scan)->sel_arg->first(), key_part); What is this for? I think this is not needed (at best). Range enumeration should start from the tree root, not from the leftmost element. > } > } > ... > @@ -15775,60 +15763,39 @@ void append_range(String *out, const KEY_PART_INFO *key_part, > > Add ranges to the trace > For ex: > - query: select * from t1 where a=2 ; > - and we have an index on a , so we create a range > - 2 <= a <= 2 > + lets say we have an index a_b(a,b) > + query: select * from t1 where a=2 and b=4 ; > + so we create a range: > + (2,4) <= (a,b) <= (2,4) > this is added to the trace > */ > > static void append_range_all_keyparts(Json_writer_array *range_trace, > - String *range_string, > - String *range_so_far, const SEL_ARG *keypart, > + PARAM param, uint idx, Is it intentional that PARAM is passed by value? What for? > + SEL_ARG *keypart, > const KEY_PART_INFO *key_parts) > { > - > - DBUG_ASSERT(keypart); > - DBUG_ASSERT(keypart && keypart != &null_element); > - > - // Navigate to first interval in red-black tree > + SEL_ARG_RANGE_SEQ seq; > + KEY_MULTI_RANGE range; > + range_seq_t seq_it; > + uint flags= 0; > + RANGE_SEQ_IF seq_if = {NULL, sel_arg_range_seq_init, > + sel_arg_range_seq_next, 0, 0}; > + KEY *keyinfo= param.table->key_info + param.real_keynr[idx]; > + uint n_key_parts= param.table->actual_n_key_parts(keyinfo); > + seq.keyno= idx; > + seq.real_keyno= param.real_keynr[idx]; > + seq.param= &param; > + seq.start= keypart; > const KEY_PART_INFO *cur_key_part= key_parts + keypart->part; > - const SEL_ARG *keypart_range= keypart->first(); > - const size_t save_range_so_far_length= range_so_far->length(); > - > + seq_it= seq_if.init((void *) &seq, 0, flags); > > - while (keypart_range) > + while (!seq_if.next(seq_it, &range)) > { > - // Append the current range predicate to the range String > - switch (keypart->type) > - { > - case SEL_ARG::Type::KEY_RANGE: > - append_range(range_so_far, cur_key_part, keypart_range->min_value, > - keypart_range->max_value, > - keypart_range->min_flag | keypart_range->max_flag); > - break; > - case SEL_ARG::Type::MAYBE_KEY: > - range_so_far->append("MAYBE_KEY"); > - break; > - case SEL_ARG::Type::IMPOSSIBLE: > - range_so_far->append("IMPOSSIBLE"); > - break; > - default: > - DBUG_ASSERT(false); > - break; > - } > - > - if (keypart_range->next_key_part && > - keypart_range->next_key_part->part == > - keypart_range->part + 1 && > - keypart_range->is_singlepoint()) > - { > - append_range_all_keyparts(range_trace, range_string, range_so_far, > - keypart_range->next_key_part, key_parts); > - } > - else > - range_trace->add(range_so_far->c_ptr_safe(), range_so_far->length()); > - keypart_range= keypart_range->next; > - range_so_far->length(save_range_so_far_length); > + String range_info; > + range_info.set_charset(system_charset_info); Can you use "StringBuffer<128> tmp(system_charset_info);" like you're already doing in other places of this patch? > + append_range(&range_info, cur_key_part, &range, n_key_parts); > + range_trace->add(range_info.c_ptr_safe(), range_info.length()); > } > } > BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] [Commits] bf951046e8d: MDEV-15296: wrong result with window function inside a subquery
by Oleksandr Byelkin 30 Apr '19

30 Apr '19

Hi, Varun! This bug shows us whole class of the same problem in the code which slip of our scope during window function development. so please check and fix them (one or separate change-set as you wish). See comment below. Am 29.04.19 um 16:40 schrieb Varun: > revision-id: bf951046e8df13702ee56e4e269ce5076e15407f (mariadb-10.2.23-99-gbf951046e8d) > parent(s): 092602ac9b650f921ec5380866d17d740f0eedb4 > author: Varun Gupta > committer: Varun Gupta > timestamp: 2019-04-29 20:07:47 +0530 > message: > > MDEV-15296: wrong result with window function inside a subquery > > Window Functions were treated as a constant in a dependent tables less subquery. > Made sure that the behaviour of window functions is same as the aggregate function > for dependent tables less subquery. > > --- > mysql-test/r/win.result | 17 +++++++++++++++++ > mysql-test/t/win.test | 10 ++++++++++ > sql/item_subselect.cc | 1 + > 3 files changed, 28 insertions(+) > > diff --git a/mysql-test/r/win.result b/mysql-test/r/win.result > index 0ddffc551dc..b86f5b1fc16 100644 > --- a/mysql-test/r/win.result > +++ b/mysql-test/r/win.result > @@ -3518,5 +3518,22 @@ rank() OVER (ORDER BY 1) ROW_NUMBER() OVER (ORDER BY (EXPORT_SET(5,'Y','N',',',4 > 1 3 > drop table t1; > # > +# MDEV-15296: wrong result with window function inside a subquery > +# > +CREATE TABLE t1(i INT); > +INSERT INTO t1 VALUES (1), (2); > +EXPLAIN EXTENDED SELECT (SELECT SUM(i) OVER (partition BY i)) FROM t1; > +id select_type table type possible_keys key key_len ref rows filtered Extra > +1 PRIMARY t1 ALL NULL NULL NULL NULL 2 100.00 > +2 DEPENDENT SUBQUERY NULL NULL NULL NULL NULL NULL NULL NULL No tables used > +Warnings: > +Note 1276 Field or reference 'test.t1.i' of SELECT #2 was resolved in SELECT #1 > +Note 1003 select <expr_cache><`test`.`t1`.`i`>((select sum(`test`.`t1`.`i`) over ( partition by `test`.`t1`.`i`))) AS `(SELECT SUM(i) OVER (partition BY i))` from `test`.`t1` > +SELECT (SELECT SUM(i) OVER (partition BY i)) FROM t1; > +(SELECT SUM(i) OVER (partition BY i)) > +1 > +2 > +drop table t1; > +# > # End of 10.2 tests > # > diff --git a/mysql-test/t/win.test b/mysql-test/t/win.test > index fd31e9d4bd9..6cc681cace4 100644 > --- a/mysql-test/t/win.test > +++ b/mysql-test/t/win.test > @@ -2265,6 +2265,16 @@ insert into t1 values (1),(2),(3); > SELECT rank() OVER (ORDER BY 1), ROW_NUMBER() OVER (ORDER BY (EXPORT_SET(5,'Y','N',',',4))) FROM t1; > drop table t1; > > +--echo # > +--echo # MDEV-15296: wrong result with window function inside a subquery > +--echo # > + > +CREATE TABLE t1(i INT); > +INSERT INTO t1 VALUES (1), (2); > +EXPLAIN EXTENDED SELECT (SELECT SUM(i) OVER (partition BY i)) FROM t1; > +SELECT (SELECT SUM(i) OVER (partition BY i)) FROM t1; > +drop table t1; > + > --echo # > --echo # End of 10.2 tests > --echo # > diff --git a/sql/item_subselect.cc b/sql/item_subselect.cc > index 8cff8f3a5c4..95482364225 100644 > --- a/sql/item_subselect.cc > +++ b/sql/item_subselect.cc > @@ -1138,6 +1138,7 @@ Item_singlerow_subselect::select_transformer(JOIN *join) > !select_lex->table_list.elements && > select_lex->item_list.elements == 1 && > !select_lex->item_list.head()->with_sum_func && > + !select_lex->item_list.head()->with_window_func && This case is definitely OK, but I also asked you to check other cases with with_sum_func, I do not see traces of it here so I did and it is my findings: Item_cache_wrapper::Item_cache_wrapper do not transfer with_window_func, is it impossible to have it here? (then better to put ASSERT) (same Item_cache_wrapper::get_tmp_table_item) Item_in_optimizer::fix_left, Item_in_optimizer::fix_fields, Item_func_interval::fix_length_and_dec do not transfer the flag also. ASSERT in Item_equal::fix_fields Item_func::get_tmp_table_item, Item_subselect::get_tmp_table_item impossible? than better put an ASSERT. Item_func constructors do not transfer the flag. Item_allany_subselect::cleanup reset. Item_singlerow_subselect::select_transformer, Item_in_subselect::single_value_transformer, Item_allany_subselect::transform_into_max_min, Item_exists_subselect::exists2in_processor, check_and_do_in_subquery_rewrites I doubts that window function allows the transformation is aggregate prohibit it. Item_in_subselect::create_single_in_to_exists_cond probably the same as aggregate. Item_in_subselect::create_row_in_to_exists_cond not sure if it is possible to have window functions hare but should be checked. st_select_lex::check_unrestricted_recursive, pushdown_cond_for_derived It looks like here also should be checked. > /* > We cant change name of Item_field or Item_ref, because it will > prevent it's correct resolving, but we should save name of > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits

1 0

Re: [Maria-developers] [Commits] 88a80e92dc4: MDEV-9959: A serious MariaDB server performance bug
by Sergey Petrunia 27 Apr '19

27 Apr '19

Hi Varun, On Wed, Apr 24, 2019 at 01:31:38PM +0530, Varun wrote: > revision-id: 88a80e92dc444ce30718f3e08d3ab66fb02bcea4 (mariadb-10.3.10-284-g88a80e92dc4) > parent(s): 3032cd8e91f1e1ead8b6f941e75cd29e473e7eaa > author: Varun Gupta > committer: Varun Gupta > timestamp: 2019-04-24 13:31:24 +0530 > message: > > MDEV-9959: A serious MariaDB server performance bug > > Step #2: If any field in the select list of the derived tables is present in the group by list also , then we are again guaranteed > that ref access to the derived table would always produce one row per key. ... > diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc > index c52005e7683..aa645ac00ca 100644 > --- a/sql/sql_lex.cc > +++ b/sql/sql_lex.cc > @@ -7617,6 +7617,68 @@ Item *st_select_lex::build_cond_for_grouping_fields(THD *thd, Item *cond, > } > > > +/** > + Check if any any item in the group by list is also present in the select_list > + @retval true: All elements common between select and group by list > +*/ > + > +void st_select_lex::is_group_by_prefix(KEY *keyinfo) I think the name of the function is poor - "is_something()" hints at hints at boolean return value, and I would say it doesn't imply any side effects. Something like 'mark_derived_index_stats()' would be better, I think. > +{ > + uint key_parts= keyinfo->usable_key_parts; > + KEY_PART_INFO *key_part_info= keyinfo->key_part; > + bool found= FALSE; > + > + if (key_parts < group_list.elements) > + return; > + > + uint matched_fields=0, i, j; > + Item *item; > + > + for (i= 0; i < key_parts; key_part_info++, i++) > + { > + uint fld_idx= key_part_info->fieldnr - 1; > + item= join->fields_list.elem(fld_idx); > + for (ORDER *order= group_list.first; order; order= order->next) > + { > + Item *ord_item= order->item[0]->real_item(); > + Item_equal *item_equal= ord_item->get_item_equal(); > + > + if (item_equal) > + { > + Item_equal_fields_iterator it(*item_equal); > + Item *equal_item; > + while ((equal_item= it++)) > + { > + if (equal_item->eq(item, 0)) > + { > + matched_fields++; > + found= TRUE; > + break; > + } > + } > + } > + else > + { > + if (item->eq(ord_item, 0)) > + { > + matched_fields++; > + found= TRUE; > + } > + } > + if (found) > + break; > + } > + > + if (matched_fields == group_list.elements) > + { > + for (j=matched_fields - 1; j < key_parts; j++) > + keyinfo->rec_per_key[j]= 1; This doesn't always work. An example: create table t1 (a int, b int, c int, d int); insert into t1 select a,a,a,a from one_k; create table t2 as select * from t1; # Note the "a, a AS b" in the select list. This is intentional. explain select * from t2, (select a, a as b from t1 group by a,b) TBL where t2.a=TBL.a and t2.b=TBL.b; Here, the select output is not guaranteed to be unique (as it contains only one of two GROUP BY columns). But I see the execution to reac the above line, assigning keyinfo->rec_per_key[j]= 1. > + return; > + } > + found= FALSE; > + } > +} > + > int set_statement_var_if_exists(THD *thd, const char *var_name, > size_t var_name_length, ulonglong value) > { A general comment: do you think we should expose this in optimizer trace? At the moment, Derived-with-keys optimization doesn't print anything there (neither in MariaDB nor in MySQL). One can see the generated KEYUSEs when they are printed. But perhaps we could add a node that would print that a derived table was added, with this cardinality, and index with these keypart cardinalities. This would make debugging easier. Any thoughts? BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] [Commits] 3032cd8e91f: step #1: if a derived table has SELECT DISTINCT, provide index statistics for it so that the join optimizer in the
by Sergey Petrunia 26 Apr '19

26 Apr '19

Hi Varun, Please mention the MDEV# in the commit comment. Otherwise, the patch is ok to push. On Mon, Apr 22, 2019 at 07:09:45PM +0530, Varun wrote: > revision-id: 3032cd8e91f1e1ead8b6f941e75cd29e473e7eaa (mariadb-10.3.10-283-g3032cd8e91f) > parent(s): f4019f5b3544a18f3ddf32df2c5214c3f8dabdce > author: Varun Gupta > committer: Varun Gupta > timestamp: 2019-04-22 18:19:25 +0530 > message: > > step #1: if a derived table has SELECT DISTINCT, provide index statistics for it so that the join optimizer in the > upper select knows that ref access to the table will produce one row. > > --- > mysql-test/main/cte_nonrecursive.result | 8 ++-- > mysql-test/main/derived.result | 54 ++++++++++++++++++++++ > mysql-test/main/derived.test | 30 ++++++++++++ > mysql-test/main/derived_view.result | 2 +- > mysql-test/main/subselect_extra.result | 2 +- > mysql-test/main/subselect_extra_no_semijoin.result | 2 +- > sql/sql_lex.h | 1 + > sql/sql_union.cc | 40 ++++++++++++++++ > sql/table.cc | 20 ++++++++ > 9 files changed, 152 insertions(+), 7 deletions(-) > > diff --git a/mysql-test/main/cte_nonrecursive.result b/mysql-test/main/cte_nonrecursive.result > index b846ec2d8ac..d80d34ecc7f 100644 > --- a/mysql-test/main/cte_nonrecursive.result > +++ b/mysql-test/main/cte_nonrecursive.result > @@ -244,7 +244,7 @@ with t as (select distinct a from t1 where b >= 'c') > select * from t as r1, t as r2 where r1.a=r2.a; > id select_type table type possible_keys key key_len ref rows Extra > 1 PRIMARY <derived2> ALL NULL NULL NULL NULL 8 Using where > -1 PRIMARY <derived3> ref key0 key0 5 r1.a 2 > +1 PRIMARY <derived3> ref key0 key0 5 r1.a 1 > 3 DERIVED t1 ALL NULL NULL NULL NULL 8 Using where; Using temporary > 2 DERIVED t1 ALL NULL NULL NULL NULL 8 Using where; Using temporary > explain > @@ -253,7 +253,7 @@ select * from (select distinct a from t1 where b >= 'c') as r1, > where r1.a=r2.a; > id select_type table type possible_keys key key_len ref rows Extra > 1 PRIMARY <derived2> ALL NULL NULL NULL NULL 8 Using where > -1 PRIMARY <derived3> ref key0 key0 5 r1.a 2 > +1 PRIMARY <derived3> ref key0 key0 5 r1.a 1 > 3 DERIVED t1 ALL NULL NULL NULL NULL 8 Using where; Using temporary > 2 DERIVED t1 ALL NULL NULL NULL NULL 8 Using where; Using temporary > # two references to t specified by a query > @@ -369,7 +369,7 @@ select c as a from t2 where c < 4) > select * from t2,t where t2.c=t.a; > id select_type table type possible_keys key key_len ref rows Extra > 1 PRIMARY t2 ALL NULL NULL NULL NULL 4 Using where > -1 PRIMARY <derived2> ref key0 key0 5 test.t2.c 2 > +1 PRIMARY <derived2> ref key0 key0 5 test.t2.c 1 > 2 DERIVED t1 ALL NULL NULL NULL NULL 8 Using where > 3 UNION t2 ALL NULL NULL NULL NULL 4 Using where > NULL UNION RESULT <union2,3> ALL NULL NULL NULL NULL NULL > @@ -381,7 +381,7 @@ select c as a from t2 where c < 4) as t > where t2.c=t.a; > id select_type table type possible_keys key key_len ref rows Extra > 1 PRIMARY t2 ALL NULL NULL NULL NULL 4 Using where > -1 PRIMARY <derived2> ref key0 key0 5 test.t2.c 2 > +1 PRIMARY <derived2> ref key0 key0 5 test.t2.c 1 > 2 DERIVED t1 ALL NULL NULL NULL NULL 8 Using where > 3 UNION t2 ALL NULL NULL NULL NULL 4 Using where > NULL UNION RESULT <union2,3> ALL NULL NULL NULL NULL NULL > diff --git a/mysql-test/main/derived.result b/mysql-test/main/derived.result > index f0d0289c1ce..857246d68b4 100644 > --- a/mysql-test/main/derived.result > +++ b/mysql-test/main/derived.result > @@ -1195,3 +1195,57 @@ drop table t1,t2,t3; > # > # End of 10.2 tests > # > +# > +# MDEV-9959: A serious MariaDB server performance bug > +# > +create table t1(a int); > +insert into t1 values (1),(2),(3),(4),(5),(6); > +create table t2(a int, b int,c int); > +insert into t2(a,b,c) values (1,1,2),(2,2,3),(3,1,4),(4,2,2),(5,1,1),(6,2,5); > +create table t3(a int, b int); > +insert into t3(a,b) values (1,1),(2,2),(2,1),(1,2),(5,1),(9,2); > +table "<derived2>" should have type=ref and rows=1 > +one select in derived table > +with distinct > +analyze select * from t1 , ((select distinct t2.a from t2 order by c))q where t1.a=q.a; > +id select_type table type possible_keys key key_len ref rows r_rows filtered r_filtered Extra > +1 PRIMARY t1 ALL NULL NULL NULL NULL 6 6.00 100.00 100.00 Using where > +1 PRIMARY <derived2> ref key0 key0 5 test.t1.a 1 1.00 100.00 100.00 > +2 DERIVED t2 ALL NULL NULL NULL NULL 6 6.00 100.00 100.00 Using temporary; Using filesort > +analyze select * from t1 , ((select distinct t2.a, t2.b from t2 order by c))q where t1.a=q.a; > +id select_type table type possible_keys key key_len ref rows r_rows filtered r_filtered Extra > +1 PRIMARY t1 ALL NULL NULL NULL NULL 6 6.00 100.00 100.00 Using where > +1 PRIMARY <derived2> ref key0 key0 5 test.t1.a 2 1.00 100.00 100.00 > +2 DERIVED t2 ALL NULL NULL NULL NULL 6 6.00 100.00 100.00 Using temporary; Using filesort > +# multiple selects in derived table > +# NO UNION ALL > +analyze select * from t1 , ( (select t2.a from t2 order by c) union (select t2.a from t2 order by c))q where t1.a=q.a; > +id select_type table type possible_keys key key_len ref rows r_rows filtered r_filtered Extra > +1 PRIMARY t1 ALL NULL NULL NULL NULL 6 6.00 100.00 100.00 Using where > +1 PRIMARY <derived2> ref key0 key0 5 test.t1.a 1 1.00 100.00 100.00 > +2 DERIVED t2 ALL NULL NULL NULL NULL 6 6.00 100.00 100.00 > +3 UNION t2 ALL NULL NULL NULL NULL 6 6.00 100.00 100.00 > +NULL UNION RESULT <union2,3> ALL NULL NULL NULL NULL NULL 6.00 NULL NULL > +select * from t1 , ( (select t2.a from t2 order by c) union (select t2.a from t2 order by c))q where t1.a=q.a; > +a a > +1 1 > +2 2 > +3 3 > +4 4 > +5 5 > +6 6 > +# UNION ALL and EXCEPT > +analyze select * from t1 , ( (select t2.a from t2 order by c) union all (select t2.a from t2 order by c) except(select t3.a from t3 order by b))q where t1.a=q.a; > +id select_type table type possible_keys key key_len ref rows r_rows filtered r_filtered Extra > +1 PRIMARY t1 ALL NULL NULL NULL NULL 6 6.00 100.00 100.00 Using where > +1 PRIMARY <derived2> ref key0 key0 5 test.t1.a 1 0.50 100.00 100.00 > +2 DERIVED t2 ALL NULL NULL NULL NULL 6 6.00 100.00 100.00 > +3 UNION t2 ALL NULL NULL NULL NULL 6 6.00 100.00 100.00 > +4 EXCEPT t3 ALL NULL NULL NULL NULL 6 6.00 100.00 100.00 > +NULL UNIT RESULT <unit2,3,4> ALL NULL NULL NULL NULL NULL 3.00 NULL NULL > +select * from t1 , ( (select t2.a from t2 order by c) union all (select t2.a from t2 order by c) except(select t3.a from t3 order by b))q where t1.a=q.a; > +a a > +3 3 > +4 4 > +6 6 > +drop table t1,t2,t3; > diff --git a/mysql-test/main/derived.test b/mysql-test/main/derived.test > index 6c51f23c51e..990f955450a 100644 > --- a/mysql-test/main/derived.test > +++ b/mysql-test/main/derived.test > @@ -1032,3 +1032,33 @@ drop table t1,t2,t3; > --echo # > --echo # End of 10.2 tests > --echo # > + > +--echo # > +--echo # MDEV-9959: A serious MariaDB server performance bug > +--echo # > + > +create table t1(a int); > +insert into t1 values (1),(2),(3),(4),(5),(6); > +create table t2(a int, b int,c int); > +insert into t2(a,b,c) values (1,1,2),(2,2,3),(3,1,4),(4,2,2),(5,1,1),(6,2,5); > +create table t3(a int, b int); > +insert into t3(a,b) values (1,1),(2,2),(2,1),(1,2),(5,1),(9,2); > + > +--echo table "<derived2>" should have type=ref and rows=1 > +--echo one select in derived table > + > +--echo with distinct > +analyze select * from t1 , ((select distinct t2.a from t2 order by c))q where t1.a=q.a; > +analyze select * from t1 , ((select distinct t2.a, t2.b from t2 order by c))q where t1.a=q.a; > + > +--echo # multiple selects in derived table > +--echo # NO UNION ALL > +analyze select * from t1 , ( (select t2.a from t2 order by c) union (select t2.a from t2 order by c))q where t1.a=q.a; > +select * from t1 , ( (select t2.a from t2 order by c) union (select t2.a from t2 order by c))q where t1.a=q.a; > + > +--echo # UNION ALL and EXCEPT > +analyze select * from t1 , ( (select t2.a from t2 order by c) union all (select t2.a from t2 order by c) except(select t3.a from t3 order by b))q where t1.a=q.a; > + > +select * from t1 , ( (select t2.a from t2 order by c) union all (select t2.a from t2 order by c) except(select t3.a from t3 order by b))q where t1.a=q.a; > + > +drop table t1,t2,t3; > diff --git a/mysql-test/main/derived_view.result b/mysql-test/main/derived_view.result > index 86dd73f5733..30831e75341 100644 > --- a/mysql-test/main/derived_view.result > +++ b/mysql-test/main/derived_view.result > @@ -1525,7 +1525,7 @@ EXPLAIN > SELECT a FROM t1 WHERE (a,b) IN (SELECT * FROM v2); > id select_type table type possible_keys key key_len ref rows Extra > 1 PRIMARY t1 ALL NULL NULL NULL NULL 2 Using where > -1 PRIMARY <derived3> ref key0 key0 10 test.t1.a,test.t1.b 2 FirstMatch(t1) > +1 PRIMARY <derived3> ref key0 key0 10 test.t1.a,test.t1.b 1 FirstMatch(t1) > 3 DERIVED t2 ALL NULL NULL NULL NULL 6 > 4 UNION t3 ALL NULL NULL NULL NULL 4 > NULL UNION RESULT <union3,4> ALL NULL NULL NULL NULL NULL > diff --git a/mysql-test/main/subselect_extra.result b/mysql-test/main/subselect_extra.result > index a3a0f1f9a15..dbcf00268c2 100644 > --- a/mysql-test/main/subselect_extra.result > +++ b/mysql-test/main/subselect_extra.result > @@ -409,7 +409,7 @@ EXPLAIN > SELECT a FROM t1 WHERE (a,b) IN (SELECT * FROM v2); > id select_type table type possible_keys key key_len ref rows Extra > 1 PRIMARY t1 ALL NULL NULL NULL NULL 2 Using where > -1 PRIMARY <derived3> ref key0 key0 10 test.t1.a,test.t1.b 2 FirstMatch(t1) > +1 PRIMARY <derived3> ref key0 key0 10 test.t1.a,test.t1.b 1 FirstMatch(t1) > 3 DERIVED t2 ALL NULL NULL NULL NULL 6 > 4 UNION t3 ALL NULL NULL NULL NULL 4 > NULL UNION RESULT <union3,4> ALL NULL NULL NULL NULL NULL > diff --git a/mysql-test/main/subselect_extra_no_semijoin.result b/mysql-test/main/subselect_extra_no_semijoin.result > index ec9ddb0452e..49a1431eb9b 100644 > --- a/mysql-test/main/subselect_extra_no_semijoin.result > +++ b/mysql-test/main/subselect_extra_no_semijoin.result > @@ -411,7 +411,7 @@ EXPLAIN > SELECT a FROM t1 WHERE (a,b) IN (SELECT * FROM v2); > id select_type table type possible_keys key key_len ref rows Extra > 1 PRIMARY t1 ALL NULL NULL NULL NULL 2 Using where > -2 DEPENDENT SUBQUERY <derived3> index_subquery key0 key0 10 func,func 2 Using where > +2 DEPENDENT SUBQUERY <derived3> index_subquery key0 key0 10 func,func 1 Using where > 3 DERIVED t2 ALL NULL NULL NULL NULL 6 > 4 UNION t3 ALL NULL NULL NULL NULL 4 > NULL UNION RESULT <union3,4> ALL NULL NULL NULL NULL NULL > diff --git a/sql/sql_lex.h b/sql/sql_lex.h > index 72ca4ac0b43..4eaec7d062b 100644 > --- a/sql/sql_lex.h > +++ b/sql/sql_lex.h > @@ -896,6 +896,7 @@ class st_select_lex_unit: public st_select_lex_node { > bool union_needs_tmp_table(); > > void set_unique_exclude(); > + bool check_distinct_in_union(); > > friend struct LEX; > friend int subselect_union_engine::exec(); > diff --git a/sql/sql_union.cc b/sql/sql_union.cc > index 87fbbebe4ba..3fb5552c77a 100644 > --- a/sql/sql_union.cc > +++ b/sql/sql_union.cc > @@ -2049,3 +2049,43 @@ void st_select_lex_unit::set_unique_exclude() > } > } > } > + > +/** > + @brief > + Check if the derived table is guaranteed to have distinct rows because of > + UNION operations used to populate it. > + > + @detail > + UNION operation removes duplicate rows from its output. That is, a query like > + > + select * from t1 UNION select * from t2 > + > + will not produce duplicate rows in its output, even if table t1 (and/or t2) > + contain duplicate rows. EXCEPT and INTERSECT operations also have this > + property. > + > + On the other hand, UNION ALL operation doesn't remove duplicates. (The SQL > + standard also defines EXCEPT ALL and INTERSECT ALL, but we don't support > + them). > + > + st_select_lex_unit computes its value left to right. That is, if there is > + a st_select_lex_unit object describing > + > + (select #1) OP1 (select #2) OP2 (select #3) > + > + then ((select #1) OP1 (select #2)) is computed first, and OP2 is computed > + second. > + > + How can one tell if st_select_lex_unit is guaranteed to have distinct > + output rows? This depends on whether the last operation was duplicate- > + removing or not: > + - UNION ALL is not duplicate-removing > + - all other operations are duplicate-removing > +*/ > + > +bool st_select_lex_unit::check_distinct_in_union() > +{ > + if (union_distinct && !union_distinct->next_select()) > + return true; > + return false; > +} > diff --git a/sql/table.cc b/sql/table.cc > index 80995abc1f9..c4494c9ae4b 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -7269,6 +7269,26 @@ bool TABLE::add_tmp_key(uint key, uint key_parts, > key_part_info++; > } > > + /* > + For the case when there is a derived table that would give distinct rows, > + the index statistics are passed to the join optimizer to tell that a ref > + access to all the fields of the derived table will produce only one row. > + */ > + > + st_select_lex_unit* derived= pos_in_table_list ? > + pos_in_table_list->derived: NULL; > + if (derived) > + { > + st_select_lex* first= derived->first_select(); > + uint select_list_items= first->get_item_list()->elements; > + if (key_parts == select_list_items) > + { > + if ((!first->is_part_of_union() && (first->options & SELECT_DISTINCT)) || > + derived->check_distinct_in_union()) > + keyinfo->rec_per_key[key_parts - 1]= 1; > + } > + } > + > set_if_bigger(s->max_key_length, keyinfo->key_length); > s->keys++; > return FALSE; > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits -- BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

[Maria-developers] Big endian issue in libmariadb: test: main.mysql_client_test fails
by REIX, Tony 26 Apr '19

26 Apr '19

Hi, We are building MariaDB on AIX. The test: main.mysql_client_test fails (after time-out). This is due to little endian code being used though HAVE_BIGENDIAN is correctly set. This is due to: libmariadb/libmariadb/ma_dtoa.c : #if defined(WORDS_BIGENDIAN) || (defined(__FLOAT_WORD_ORDER) && \ (__FLOAT_WORD_ORDER == __BIG_ENDIAN)) #define word0(x) (x)->L[0] #define word1(x) (x)->L[1] #else #define word0(x) (x)->L[1] #define word1(x) (x)->L[0] #endif which results in the #else part (Little Endian) since none of the tested macros are defined. File libmariadb/libmariadb/ma_dtoa.c includes ma_global.h which has: #define HAVE_BIGENDIAN 1 but nothing defined about WORDS_BIGENDIAN . However, probably that some changes were made in the past in ma_global.h, since I see: #ifdef HAVE_BIGENDIAN #endif /* WORDS_BIGENDIAN */ 2 times in this file. I do not master MariaDB, so I do not have a clear idea about how to fix this. I see 3 solutions: a) libmariadb/CMakeLists.txt : Add: SET(HAVE_WORDS_BIGENDIAN TRUE ...) after : TEST_BIG_ENDIAN(HAVE_BIGENDIAN) if it is BigEndian. b) libmariadb/include/ma_global.h : Add: #ifdef HAVE_BIGENDIAN #define WORDS_BIGENDIAN 1 #endif c) libmariadb/libmariadb/ma_dtoa.c : Change: #if defined(WORDS_BIGENDIAN) || (defined(__FLOAT_WORD_ORDER) && \ (__FLOAT_WORD_ORDER == __BIG_ENDIAN)) by: #if defined(HAVE_BIGENDIAN) || defined(WORDS_BIGENDIAN) || (defined(__FLOAT_WORD_ORDER) && \ (__FLOAT_WORD_ORDER == __BIG_ENDIAN)) Please let me know which seems appropriate. Regards, Tony ATOS / Bull SAS ATOS Expert IBM Coop Architect & Technical Leader Office : +33 (0) 4 76 29 72 67 1 rue de Provence - 38432 Échirolles - France www.atos.net<https://mail.ad.bull.net/owa/redir.aspx?C=PvphmPvCZkGrAgHVnWGsdMcDKgzl_dEIs…>

1 0

Re: [Maria-developers] [Commits] 647c22692ad: MDEV-17588 replicate-do filters cause errors when creating filtered-out tables on master with syntax unsupported on slave
by andrei.elkin＠pp.inet.fi 25 Apr '19

25 Apr '19

Sachin, howdy. > revision-id: 647c22692ad1fe466eb14b749520cc520807d4fa (mariadb-10.1.38-114-g647c22692ad) > parent(s): 6c5e4c9bc0d9ac30f7ec7ee334630bacb58687ba > author: Sachin > committer: Sachin > timestamp: 2019-04-23 17:58:03 +0530 > message: > > MDEV-17588 replicate-do filters cause errors when creating filtered-out tables on master with syntax unsupported on slave > > Prototype Commit. In next commit could you please brief about the problem and attempted solution. > > Delay throwing of error in parsing stage untill we know the result from rpl > filter. I think you were on the right track implementing this deferred error/warning issuing. Thanks for attempting it! There's a new THD::"flag" for that purpose. I suggest we place it inside struct LEX_MASTER_INFO to set pretty similarly to how the patch does. Secondly, let blocks in sql_yacc.yy that call error/warning functions that gets deferred (because of the slave thread) be invoked (if they will be) in mysql_execute_command() in verbatim. That means to turn the blocks into functions and call them at once or later. Your patch presents two such blocks, I point to them below. For each block I suggest we introduce a flag. The block flag is set when the block gets visited (but not executed 'cos of slave). The "flag" therefore is going to be an array (or enum?). > > --- > mysql-test/suite/rpl/r/rpl_mdev_17588.result | 26 ++++++++++++++++++++++ > mysql-test/suite/rpl/t/rpl_mdev_17588-slave.opt | 1 + > mysql-test/suite/rpl/t/rpl_mdev_17588.test | 29 +++++++++++++++++++++++++ > sql/sql_class.h | 1 + > sql/sql_parse.cc | 7 ++++++ > sql/sql_yacc.yy | 12 +++++++--- > 6 files changed, 73 insertions(+), 3 deletions(-) > > diff --git a/mysql-test/suite/rpl/r/rpl_mdev_17588.result b/mysql-test/suite/rpl/r/rpl_mdev_17588.result > new file mode 100644 > index 00000000000..976efde2bdd > --- /dev/null > +++ b/mysql-test/suite/rpl/r/rpl_mdev_17588.result > @@ -0,0 +1,26 @@ > +include/master-slave.inc > +[connection master] > +set sql_log_bin= 0; > +install soname 'ha_tokudb'; > +set sql_log_bin= 1; > +create table t1 (a int) engine=TokuDB; > +create table t2 (a int); > +create table t3 (a int) engine=TokuDB; > +include/wait_for_slave_sql_error.inc [errno=1286] > +show create table t1; > +ERROR 42S02: Table 'test.t1' doesn't exist > +show create table t2; > +Table Create Table > +t2 CREATE TABLE `t2` ( > + `a` int(11) DEFAULT NULL > +) ENGINE=MyISAM DEFAULT CHARSET=latin1 > +show create table t3; > +ERROR 42S02: Table 'test.t3' doesn't exist > +SET GLOBAL SQL_SLAVE_SKIP_COUNTER=1; > +START SLAVE; > +drop table t1, t2, t3; > +set sql_log_bin= 0; > +uninstall soname 'ha_tokudb'; > +set sql_log_bin= 1; > +CALL mtr.add_suppression('Slave: Unknown storage engine .* Error_code: 1286'); > +include/rpl_end.inc > diff --git a/mysql-test/suite/rpl/t/rpl_mdev_17588-slave.opt b/mysql-test/suite/rpl/t/rpl_mdev_17588-slave.opt > new file mode 100644 > index 00000000000..19497afd22a > --- /dev/null > +++ b/mysql-test/suite/rpl/t/rpl_mdev_17588-slave.opt > @@ -0,0 +1 @@ > +--replicate-do-table=test.t2 --replicate-do-table=test.t3 --sql-mode='NO_ENGINE_SUBSTITUTION' > diff --git a/mysql-test/suite/rpl/t/rpl_mdev_17588.test b/mysql-test/suite/rpl/t/rpl_mdev_17588.test > new file mode 100644 > index 00000000000..c636077e4d1 > --- /dev/null > +++ b/mysql-test/suite/rpl/t/rpl_mdev_17588.test > @@ -0,0 +1,29 @@ > +--source include/master-slave.inc > + > +--connection master > +set sql_log_bin= 0; > +install soname 'ha_tokudb'; > +set sql_log_bin= 1; > + > +create table t1 (a int) engine=TokuDB; > +create table t2 (a int); > +create table t3 (a int) engine=TokuDB; > + > +--connection slave > +let $slave_sql_errno= 1286; > +source include/wait_for_slave_sql_error.inc; > +--error ER_NO_SUCH_TABLE > +show create table t1; > +show create table t2; > +--error ER_NO_SUCH_TABLE > +show create table t3; > +SET GLOBAL SQL_SLAVE_SKIP_COUNTER=1; > +START SLAVE; > +--connection master > +drop table t1, t2, t3; > +set sql_log_bin= 0; > +uninstall soname 'ha_tokudb'; > +set sql_log_bin= 1; > +--sync_slave_with_master > +CALL mtr.add_suppression('Slave: Unknown storage engine .* Error_code: 1286'); > +--source include/rpl_end.inc > diff --git a/sql/sql_class.h b/sql/sql_class.h > index 6640e02147a..4361700ee6c 100644 > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -2802,6 +2802,7 @@ class THD :public Statement, > uint8 password; /* 0, 1 or 2 */ > uint8 failed_com_change_user; > bool slave_thread; > + char *rpl_unavailable_storage_plugin= NULL; > bool extra_port; /* If extra connection */ > > bool no_errors; > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc > index 6649c60f827..5f8c833c76f 100644 > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -2656,6 +2656,13 @@ mysql_execute_command(THD *thd) > /* we warn the slave SQL thread */ > my_message(ER_SLAVE_IGNORED_TABLE, ER_THD(thd, ER_SLAVE_IGNORED_TABLE), > MYF(0)); > + thd->rpl_unavailable_storage_plugin= NULL; > + DBUG_RETURN(0); > + } > + if (thd->rpl_unavailable_storage_plugin) > + { > + my_error(ER_UNKNOWN_STORAGE_ENGINE, MYF(0), thd->rpl_unavailable_storage_plugin); > + thd->rpl_unavailable_storage_plugin= NULL; Here the check would go for each thd->lex.mi.flag[i] when set call "flag_func[i]"; I index flag[] but I don't insist on implementing it like that. Here is the function's body { if (thd->variables.sql_mode & MODE_NO_ENGINE_SUBSTITUTION) my_yyabort_error((ER_UNKNOWN_STORAGE_ENGINE, MYF(0), $1.str)); $$= 0; push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN, ER_UNKNOWN_STORAGE_ENGINE, ER_THD(thd, ER_UNKNOWN_STORAGE_ENGINE), $1.str); } and a flag function also might need some parameters like $1.str in above. > DBUG_RETURN(0); > } > /* > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index 5111f0690ab..0756028c34a 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -2476,7 +2476,7 @@ create: > LEX *lex= thd->lex; > lex->current_select= &lex->select_lex; > if ((lex->create_info.used_fields & HA_CREATE_USED_ENGINE) && > - !lex->create_info.db_type) > + !lex->create_info.db_type && !thd->rpl_unavailable_storage_plugin) > { > lex->create_info.use_default_db_type(thd); This is the 2nd block which depends on the 1st one. if ((lex->create_info.used_fields & HA_CREATE_USED_ENGINE) && !lex->create_info.db_type) { lex->create_info.use_default_db_type(thd); if (!thd->slave_thread || !lex->mi.flag[0]) print_warn_using_other_handler(...) } where print_warn_using_other_handler() is { push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN, ER_WARN_USING_OTHER_HANDLER, ER_THD(thd, ER_WARN_USING_OTHER_HANDLER), hton_name(lex->create_info.db_type)->str, $5->table.str); } To call it deferredly we seem need two args to memorize (reserve fields too) in Lex->mi. I hope my suggestions make sense and feasible. Cheers, Andrei > push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN, > @@ -5791,9 +5791,15 @@ storage_engines: > else > { > if (thd->variables.sql_mode & MODE_NO_ENGINE_SUBSTITUTION) > - my_yyabort_error((ER_UNKNOWN_STORAGE_ENGINE, MYF(0), $1.str)); > + { > + if (!thd->slave_thread) > + my_yyabort_error((ER_UNKNOWN_STORAGE_ENGINE, MYF(0), $1.str)); > + else > + thd->rpl_unavailable_storage_plugin= $1.str; > + } > $$= 0; > - push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN, > + if (!thd->rpl_unavailable_storage_plugin) > + push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN, > ER_UNKNOWN_STORAGE_ENGINE, > ER_THD(thd, ER_UNKNOWN_STORAGE_ENGINE), > $1.str); > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits

1 0

Re: [Maria-developers] [Commits] 8ae7a5eb037: MDEV-17260: Memory leaks in mysqlbinlog
by andrei.elkin＠pp.inet.fi 24 Apr '19

24 Apr '19

Sujatha, hello. The patch looks good, I only have a suggestion to the test for better readability, within the quoted patch below. Thanks for a good piece of work! Cheers, Andrei >commit 8ae7a5eb03725441393c353e4e203f80b666c035 >Author: Sujatha Sivakumar <sujatha.sivakumar(a)mariadb.com> >Date: Tue Apr 23 19:47:59 2019 +0530 > > MDEV-17260: Memory leaks in mysqlbinlog > > Problem: > ======== > The mysqlbinlog tool is leaking memory, causing failures in various tests when > compiling and testing with AddressSanitizer or LeakSanitizer like this: > > cmake -DCMAKE_BUILD_TYPE=Debug -DWITH_ASAN:BOOL=ON /path/to/source > make -j$(nproc) > cd mysql-test > ASAN_OPTIONS=abort_on_error=1 ./mtr --parallel=auto > > Analysis: > ========= > Two types of leaks were observed during above execution. > > 1) Leak in Log_event::read_log_event(char const*, unsigned int, char const**, > Format_description_log_event const*, char) > File: sql/log_event.cc:2150 > > For all row based replication events the memory which is allocated during > read_log_event is not freed after the event is processed. The event specific > memory has to be retained only when flashback option is enabled with > mysqlbinlog tool. In this case all the events are retained till the end > statement is received and they are processed in reverse order and they are > destroyed. But in the existing code all events are retained irrespective of > flashback mode. Hence the memory leaks are observed. > > 2) read_remote_annotate_event(unsigned char*, unsigned long, char const**) > File: client/mysqlbinlog.cc:194 > > In general the Annotate event is not printed immediately because all > subsequent rbr-events can be filtered away. Instead it will be printed > together with the first not filtered away Table map or the last rbr will be > processed. While reading remote annotate events memory is allocated for event > buffer and event's temp_buf is made to point to the allocated buffer as shown > below. The TRUE flag is used for doing proper cleanup using free_temp_buf(). > i.e at the time of deletion of annotate event its destructor takes care of > clearing the temp_buf. > > /* > Ensure the event->temp_buf is pointing to the allocated buffer. > (TRUE = free temp_buf on the event deletion) > */ > event->register_temp_buf((char*)event_buf, TRUE); > > But existing code does the following when it receives a remote annotate_event. > > if (remote_opt) > ev->temp_buf= 0; > > That is code immediately sets temp_buf=0, because of which free_temp_buf() > call will return empty handed as it has lost the reference to the allocated > temporary buffer. This results in memory leak > > Fix: > ==== > 1) If not in flashback mode, destroy the memory for events once they are > processed. > > 2) Remove the ev->temp_buf=0 code for remote option. Let the proper cleanup to > be done as part of free_temp_buf(). > >diff --git a/client/mysqlbinlog.cc b/client/mysqlbinlog.cc >index cfc05cbf794..3dad4fef67b 100644 >--- a/client/mysqlbinlog.cc >+++ b/client/mysqlbinlog.cc >@@ -227,8 +227,7 @@ void print_annotate_event(PRINT_EVENT_INFO *print_event_info) > if (annotate_event) > { > annotate_event->print(result_file, print_event_info); >- delete annotate_event; // the event should not be printed more than once >- annotate_event= 0; >+ free_annotate_event(); > } > } > >@@ -1465,7 +1464,7 @@ Exit_status process_event(PRINT_EVENT_INFO *print_event_info, Log_event *ev, > if (print_row_event(print_event_info, ev, e->get_table_id(), > e->get_flags(Rows_log_event::STMT_END_F))) > goto err; >- if (!is_stmt_end) >+ if (opt_flashback && !is_stmt_end) > destroy_evt= FALSE; > break; > } >@@ -1478,7 +1477,7 @@ Exit_status process_event(PRINT_EVENT_INFO *print_event_info, Log_event *ev, > if (print_row_event(print_event_info, ev, e->get_table_id(), > e->get_flags(Old_rows_log_event::STMT_END_F))) > goto err; >- if (!is_stmt_end) >+ if (opt_flashback && !is_stmt_end) > destroy_evt= FALSE; > break; > } >@@ -1539,8 +1538,6 @@ Exit_status process_event(PRINT_EVENT_INFO *print_event_info, Log_event *ev, > } > } > >- if (remote_opt) >- ev->temp_buf= 0; > if (destroy_evt) /* destroy it later if not set (ignored table map) */ > delete ev; > } >diff --git a/mysql-test/suite/binlog/r/flashback.result b/mysql-test/suite/binlog/r/flashback.result >index c96eaebe838..047c032b5df 100644 >--- a/mysql-test/suite/binlog/r/flashback.result >+++ b/mysql-test/suite/binlog/r/flashback.result >@@ -674,6 +674,28 @@ world.city 563256876 > DROP TABLE test.test; > DROP TABLE world.city; > DROP DATABASE world; >+# < CASE 7 > >+# Test Case for MDEV-17260 >+# >+RESET MASTER; >+CREATE TABLE t1 ( f INT PRIMARY KEY ) ENGINE=innodb; >+INSERT INTO t1 VALUES (1),(2),(3),(4),(5),(6); >+# 6- Rows must be present >+SELECT COUNT(*) FROM t1; >+COUNT(*) >+6 >+FLUSH LOGS; >+DELETE FROM t1; >+FLUSH LOGS; >+# 0- Rows must be present >+SELECT COUNT(*) FROM t1; >+COUNT(*) >+0 >+# 6- Rows must be present upon restoring from flashback >+SELECT COUNT(*) FROM t1; >+COUNT(*) >+6 >+DROP TABLE t1; > SET binlog_format=statement; > Warnings: > Warning 1105 MariaDB Galera and flashback do not support binlog format: STATEMENT >diff --git a/mysql-test/suite/binlog/t/flashback.test b/mysql-test/suite/binlog/t/flashback.test >index 3fc8c44c60c..fb80669ca51 100644 >--- a/mysql-test/suite/binlog/t/flashback.test >+++ b/mysql-test/suite/binlog/t/flashback.test >@@ -335,8 +335,29 @@ DROP TABLE test.test; > DROP TABLE world.city; > DROP DATABASE world; > >-## Clear >+--echo # < CASE 7 > >+--echo # Test Case for MDEV-17260 >+--echo # >+RESET MASTER; >+ >+CREATE TABLE t1 ( f INT PRIMARY KEY ) ENGINE=innodb; >+INSERT INTO t1 VALUES (1),(2),(3),(4),(5),(6); >+--echo # 6- Rows must be present >+SELECT COUNT(*) FROM t1; SELECT COUNT(*) as six FROM t1; would read better though it's far from being the optimal automation. (The optimal one clearly must also count 'six' or whatever number itself, as well as bail out with an error when count(*) won't match the expected # - sure not for the current patch). >+FLUSH LOGS; >+DELETE FROM t1; >+FLUSH LOGS; >+--echo # 0- Rows must be present >+SELECT COUNT(*) FROM t1; >+--exec $MYSQL_BINLOG -vv -B --read-from-remote-server --user=root --host=127.0.0.1 --port=$MASTER_MYPORT master-bin.000002> $MYSQLTEST_VARDIR/tmp/mysqlbinlog_row_flashback_7.sql >+--exec $MYSQL -e "SET binlog_format= ROW; source $MYSQLTEST_VARDIR/tmp/mysqlbinlog_row_flashback_7.sql;" >+ >+--echo # 6- Rows must be present upon restoring from flashback >+SELECT COUNT(*) FROM t1; > >+DROP TABLE t1; >+ >+## Clear > SET binlog_format=statement; > --error ER_FLASHBACK_NOT_SUPPORTED > SET GLOBAL binlog_format=statement; >

1 0