- developers - lists.mariadb.org

[Maria-developers] Fwd: mixing of user-defined data types with other data types
by Vicențiu Ciorbaru 28 Dec '16

28 Dec '16

Hi Alexander! Comments inline. Most are stylistic comments. I think the patch is great. The Item_type_holder is really bugging me though. It feels poorly designed in the current place. > diff --git a/sql/field.cc b/sql/field.cc > index 6f273e6..7876a6f 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -84,9 +84,13 @@ const char field_separator=','; > */ > #define FIELDTYPE_TEAR_FROM (MYSQL_TYPE_BIT + 1) > #define FIELDTYPE_TEAR_TO (MYSQL_TYPE_NEWDECIMAL - 1) > -#define FIELDTYPE_NUM (FIELDTYPE_TEAR_FROM + (255 - FIELDTYPE_TEAR_TO)) > +#define FIELDTYPE_LAST 254 > +#define FIELDTYPE_NUM (FIELDTYPE_TEAR_FROM + (FIELDTYPE_LAST - FIELDTYPE_TEAR_TO)) > These defines here look sketchy. I know it's not from your patch but can we not turn them to const ints instead? Also, the naming is horibly inconsistent, one uses 2 and others use to. I vote for _to_. We can have the full function definition as: const int FIELDTYPE_TEAR_FROM= MYSQL_TYPE_BIT + 1; const int FIELDTYPE_TEAR_TO= MYSQL_TYPE_NEWDECIMAL - 1; const int FIELDTYPE_LAST= 254; const int FIELDTYPE_NUM= FIELDTYPE_TEAR_FROM + (FIELDTYPE_LAST - FIELDTYPE_TEAR_TO); static inline int field_type_to_index (enum_field_types field_type) { DBUG_ASSERT(real_type_to_type(field_type) < FIELDTYPE_TEAR_FROM || real_type_to_type(field_type) > FIELDTYPE_TEAR_TO); DBUG_ASSERT(field_type <= FIELDTYPE_LAST); field_type= real_type_to_type(field_type); if (field_type < FIELDTYPE_TEAR_FROM) return field_type; return FIELDTYPE_TEAR_FROM + (field_type - FIELDTYPE_TEAR_TO) - 1; } The ternary operator provides no benefit in this case, let's replace it with regular if statement. Same number of lines but the code is cleaner I'd say. > > static inline int field_type2index (enum_field_types field_type) > { > + DBUG_ASSERT(real_type_to_type(field_type) < FIELDTYPE_TEAR_FROM || > + real_type_to_type(field_type) > FIELDTYPE_TEAR_TO); > + DBUG_ASSERT(field_type <= FIELDTYPE_LAST); > field_type= real_type_to_type(field_type); > return (field_type < FIELDTYPE_TEAR_FROM ? > field_type : > @@ -94,6 +98,12 @@ static inline int field_type2index (enum_field_types field_type) > } > > > +/** > + Implements data type merge rules for the built-in traditional data types. > + - UNION > + - CASE and its abbreviations COALESCE, IF, IFNULL > + - LEAST/GREATEST > +*/ > I don't like this comment here as it's not complete. I'd much rather have it also explain how to interpret this table. I had to look up a calling spot to figure out exactly how it's done. I'd also have it say: Ex: Given Fields A and B of real_types a and b, we find the result type of COALESCE(A, B) by querying: field_types_merge_rules[field_type_to_index(a)][field_type_to_index(b)]. > > > static enum_field_types field_types_merge_rules [FIELDTYPE_NUM][FIELDTYPE_NUM]= > { > /* MYSQL_TYPE_DECIMAL -> */ > @@ -948,46 +927,18 @@ static enum_field_types field_types_merge_rules [FIELDTYPE_NUM][FIELDTYPE_NUM]= > enum_field_types Field::field_type_merge(enum_field_types a, > enum_field_types b) > { > - DBUG_ASSERT(real_type_to_type(a) < FIELDTYPE_TEAR_FROM || > - real_type_to_type(a) > FIELDTYPE_TEAR_TO); > - DBUG_ASSERT(real_type_to_type(b) < FIELDTYPE_TEAR_FROM || > - real_type_to_type(b) > FIELDTYPE_TEAR_TO); > return field_types_merge_rules[field_type2index(a)] > [field_type2index(b)]; > } > > - > -static Item_result field_types_result_type [FIELDTYPE_NUM]= > +const Type_handler * > +Type_handler::aggregate_for_result_traditional(const Type_handler *a, > + const Type_handler *b) > { > - //MYSQL_TYPE_DECIMAL MYSQL_TYPE_TINY > - DECIMAL_RESULT, INT_RESULT, > - //MYSQL_TYPE_SHORT MYSQL_TYPE_LONG > - INT_RESULT, INT_RESULT, > - //MYSQL_TYPE_FLOAT MYSQL_TYPE_DOUBLE > - REAL_RESULT, REAL_RESULT, > - //MYSQL_TYPE_NULL MYSQL_TYPE_TIMESTAMP > - STRING_RESULT, STRING_RESULT, > - //MYSQL_TYPE_LONGLONG MYSQL_TYPE_INT24 > - INT_RESULT, INT_RESULT, > - //MYSQL_TYPE_DATE MYSQL_TYPE_TIME > - STRING_RESULT, STRING_RESULT, > - //MYSQL_TYPE_DATETIME MYSQL_TYPE_YEAR > - STRING_RESULT, INT_RESULT, > - //MYSQL_TYPE_NEWDATE MYSQL_TYPE_VARCHAR > - STRING_RESULT, STRING_RESULT, > - //MYSQL_TYPE_BIT <16>-<245> > - STRING_RESULT, > - //MYSQL_TYPE_NEWDECIMAL MYSQL_TYPE_ENUM > - DECIMAL_RESULT, STRING_RESULT, > - //MYSQL_TYPE_SET MYSQL_TYPE_TINY_BLOB > - STRING_RESULT, STRING_RESULT, > - //MYSQL_TYPE_MEDIUM_BLOB MYSQL_TYPE_LONG_BLOB > - STRING_RESULT, STRING_RESULT, > - //MYSQL_TYPE_BLOB MYSQL_TYPE_VAR_STRING > - STRING_RESULT, STRING_RESULT, > - //MYSQL_TYPE_STRING MYSQL_TYPE_GEOMETRY > - STRING_RESULT, STRING_RESULT > -}; > + enum_field_types ta= a->real_field_type(); > + enum_field_types tb= b->real_field_type(); > + return Type_handler::get_handler_by_real_type(Field::field_type_merge(ta, tb)); Nit: this line is over 80 characters. Do we care? Most code tends to comply to it. It adds one extra line to split so it might not be worth it to adhere to a full 100% 80 character limit. > +} > > > /* > diff --git a/sql/field.h b/sql/field.h > index 541da5a..fa84e7d 100644 > --- a/sql/field.h > +++ b/sql/field.h > @@ -835,7 +835,6 @@ class Field: public Value_source > virtual Item_result cmp_type () const { return result_type(); } > static bool type_can_have_key_part(enum_field_types); > The field_type_merge function breaks all the other naming patterns. We have result_type, cmp_type real_type and now field_type_merge. Wouldn't it be better to name it field_merge_type, to be consistent? Or since this is a lookup kind of operation, perhaps name it (get|lookup)_merge_field_type? I know it's not _exactly_ like result_type and compare_type, but it generally is used in a simillar context. > > static enum_field_types field_type_merge(enum_field_types, enum_field_types); > - static Item_result result_merge_type(enum_field_types); > virtual bool eq(Field *field) > { > return (ptr == field->ptr && null_ptr == field->null_ptr && > diff --git a/sql/item.cc b/sql/item.cc > index c97f41f..c9b6155 100644 > --- a/sql/item.cc > +++ b/sql/item.cc > @@ -9657,20 +9657,8 @@ Item_type_holder::Item_type_holder(THD *thd, Item *item) > maybe_null= item->maybe_null; > collation.set(item->collation); > get_full_info(item); > - /** > - Field::result_merge_type(real_field_type()) should be equal to > - result_type(), with one exception when "this" is a Item_field for > - a BIT field: > - - Field_bit::result_type() returns INT_RESULT, so does its Item_field. > - - Field::result_merge_type(MYSQL_TYPE_BIT) returns STRING_RESULT. > - Perhaps we need a new method in Type_handler to cover these type > - merging rules for UNION. > - */ > - DBUG_ASSERT(real_field_type() == MYSQL_TYPE_BIT || > - Item_type_holder::result_type() == > - Field::result_merge_type(Item_type_holder::real_field_type())); > /* fix variable decimals which always is NOT_FIXED_DEC */ > - if (Field::result_merge_type(real_field_type()) == INT_RESULT) > Alright so this seems to be fixed here, looking at Type_handler_bit, inheriting from Type_handler_int_result. Do we test this somewhere though? I couldn't find it in the test case, perhaps you can point it out to me. > > + if (Item_type_holder::result_type() == INT_RESULT) > decimals= 0; > prev_decimal_int_part= item->decimal_int_part(); > #ifdef HAVE_SPATIAL > @@ -9785,12 +9773,21 @@ bool Item_type_holder::join_types(THD *thd, Item *item) > DBUG_PRINT("info:", ("in type %d len %d, dec %d", > get_real_type(item), > item->max_length, item->decimals)); > - set_handler_by_real_type(Field::field_type_merge(real_field_type(), > - get_real_type(item))); > + const Type_handler *item_type_handler= > + Type_handler::get_handler_by_real_type(get_real_type(item)); > + if (aggregate_for_result(item_type_handler)) > + { > + my_error(ER_CANT_AGGREGATE_2TYPES, MYF(0), > + Item_type_holder::type_handler()->name().ptr(), > + item_type_handler->name().ptr(), > + "UNION"); > + DBUG_RETURN(true); > + } > + > { > uint item_decimals= item->decimals; > /* fix variable decimals which always is NOT_FIXED_DEC */ > - if (Field::result_merge_type(real_field_type()) == INT_RESULT) > + if (Item_type_holder::result_type() == INT_RESULT) > Ok, we're avoiding a virtual function call here. What I don't like is that this Item_type_holder class basically "abuses" the Item interface to such a degree that it even has to directly declare functions such as: double Item_type_holder::val_real() { DBUG_ASSERT(0); // should never be called return 0.0; } Let's discuss about cleaning this up later. To me it feels like this Item does not really belong in the Item class and should be factored out. Probably a whole project on its own :) > > > item_decimals= 0; > decimals= MY_MAX(decimals, item_decimals); > } > diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc > index 98b179b..e5e366e 100644 > --- a/sql/item_cmpfunc.cc > +++ b/sql/item_cmpfunc.cc > @@ -180,32 +179,40 @@ static int agg_cmp_type(Item_result *type, Item **items, uint nitems) > @return aggregated field type. > */ > > -enum_field_types agg_field_type(Item **items, uint nitems, > - bool treat_bit_as_number) > Why is this function in item_cmpfunc.cc and not in sql_type.cc? > +bool > +Type_handler_hybrid_field_type::aggregate_for_result(const char *funcname, > + Item **items, uint nitems, > + bool treat_bit_as_number) > { I would move uint i to be a local for variable. This is a C-style loop. Is there a compiler that doesn't support this in one of our builders? Also, how about size_t instead of uint? (Probably not necessary but wlad made a point of prefering to use that for iterators and such). > uint i; > if (!nitems || items[0]->result_type() == ROW_RESULT) > { > DBUG_ASSERT(0); > - return MYSQL_TYPE_NULL; > + set_handler(&type_handler_null); > + return true; > } > - enum_field_types res= items[0]->field_type(); > + set_handler(items[0]->type_handler()); > uint unsigned_count= items[0]->unsigned_flag; > for (i= 1 ; i < nitems ; i++) > { > - enum_field_types cur= items[i]->field_type(); > + const Type_handler *cur= items[i]->type_handler(); > if (treat_bit_as_number && > - ((res == MYSQL_TYPE_BIT) ^ (cur == MYSQL_TYPE_BIT))) > + ((type_handler() == &type_handler_bit) ^ (cur == &type_handler_bit))) > { > - if (res == MYSQL_TYPE_BIT) > - res= MYSQL_TYPE_LONGLONG; // BIT + non-BIT > + if (type_handler() == &type_handler_bit) > + set_handler(&type_handler_longlong); // BIT + non-BIT > else > - cur= MYSQL_TYPE_LONGLONG; // non-BIT + BIT > + cur= &type_handler_longlong; // non-BIT + BIT > + } > + if (aggregate_for_result(cur)) > + { > + my_error(ER_CANT_AGGREGATE_2TYPES, MYF(0), > + type_handler()->name().ptr(), cur->name().ptr(), funcname); > + return true; > } > - res= Field::field_type_merge(res, cur); > unsigned_count+= items[i]->unsigned_flag; > } > - switch (res) { > + switch (field_type()) { > case MYSQL_TYPE_TINY: > case MYSQL_TYPE_SHORT: > case MYSQL_TYPE_LONG: > diff --git a/sql/sql_type.cc b/sql/sql_type.cc > index 8746595..397b5cf 100644 > --- a/sql/sql_type.cc > +++ b/sql/sql_type.cc > @@ -54,6 +51,41 @@ static Type_handler_set type_handler_set; > Type_handler_null type_handler_null; > Type_handler_row type_handler_row; > Type_handler_varchar type_handler_varchar; > +Type_handler_newdecimal type_handler_newdecimal; > +Type_handler_longlong type_handler_longlong; > +Type_handler_bit type_handler_bit; > + > + I'm sure there's a better way to write this so that it gets initialized at compile time instead of at runtime (before main). Perhaps we can define the Type_aggregator differently. I need to look this Up. For now it will work. Standard offers no guarantees regarding the order, but it shouldn't matter for us as the address shouldn't change for global objects during initialization. "It is implementation-defined whether or not the dynamic initialization (8.5, 9.4, 12.1, 12.6.1) of an object of namespace scope is done before the first statement of main. If the initialization is deferred to some point in time after the first statement of main, it shall occur before the first use of any function or object defined in the same translation unit as the object to be initialized." > +Type_aggregator type_aggregator_for_result; > + > + > +class Static_data_initializer > +{ > +public: > + static Static_data_initializer m_singleton; > + Static_data_initializer() > + { > +#ifdef HAVE_SPATIAL > + type_aggregator_for_result.add(&type_handler_geometry, > + &type_handler_null, > + &type_handler_geometry); > + type_aggregator_for_result.add(&type_handler_geometry, > + &type_handler_geometry, > + &type_handler_geometry); > + type_aggregator_for_result.add(&type_handler_geometry, > + &type_handler_blob, > + &type_handler_long_blob); > + type_aggregator_for_result.add(&type_handler_geometry, > + &type_handler_varchar, > + &type_handler_long_blob); > + type_aggregator_for_result.add(&type_handler_geometry, > + &type_handler_string, > + &type_handler_long_blob); > +#endif > + } > +}; > + > +Static_data_initializer Static_data_initializer::m_singleton; > > > /** > diff --git a/sql/sql_type.h b/sql/sql_type.h > index 92dee61..4bc5542 100644 > --- a/sql/sql_type.h > +++ b/sql/sql_type.h > @@ -214,6 +214,31 @@ class Type_std_attributes > }; > > I don't like this class too much. One can easily break it by either changing LEX_CSTRING::str or LEX_CSTRING::length without changing the other one. I suggest we make the inheritance private so that the only way to access the members is through the methods available. A suggestion I have is to create a generic "String" class that has this same behaviour, without calling it Name. Afterwards typedefing it to Name. > > +class Name: public LEX_CSTRING > +{ > +public: > + Name(const char *str_arg, uint length_arg) > + { > + LEX_CSTRING::str= str_arg; > + LEX_CSTRING::length= length_arg; > + } Also, these ought to be removed if they are commented out. > + /* > + Name() > + { > + LEX_CSTRING::str= NULL; > + LEX_CSTRING::length= 0; > + } > + Name(const LEX_STRING &name) > + { > + LEX_CSTRING::str= name.str; > + LEX_CSTRING::length= name.length; > + } > + */ > + const char *ptr() const { return LEX_CSTRING::str; } > + uint length() const { return LEX_CSTRING::length; } > +}; > + > + > class Type_handler > { > protected: > @@ -235,6 +260,11 @@ class Type_handler > DBUG_ASSERT(type != TIME_RESULT); > return get_handler_by_cmp_type(type); > } > + static const > + Type_handler *aggregate_for_result_traditional(const Type_handler *h1, > + const Type_handler *h2); > + This function can return a const reference from a const static object within the class's namespace. Why create an object every time this is called? Compiler might optimize it, but why risk it? This goes for all the implementations. > + virtual const Name name() const= 0; > virtual enum_field_types field_type() const= 0; > virtual enum_field_types real_field_type() const { return field_type(); } > virtual Item_result result_type() const= 0; > @@ -884,5 +958,63 @@ class Type_handler_hybrid_real_field_type: > extern Type_handler_row type_handler_row; > extern Type_handler_null type_handler_null; > extern Type_handler_varchar type_handler_varchar; > +extern Type_handler_newdecimal type_handler_newdecimal; > +extern Type_handler_longlong type_handler_longlong; > +extern Type_handler_bit type_handler_bit; > + > +class Type_aggregator > +{ I'm not sure if Element is a good name for this. Aggregate_result_pair maybe? > + class Element > + { > + public: > + const Type_handler *m_handler1; > + const Type_handler *m_handler2; > + const Type_handler *m_result; > + Element() { } > + Element(const Type_handler *handler1, > + const Type_handler *handler2, > + const Type_handler *result) > + :m_handler1(handler1), m_handler2(handler2), m_result(result) > + { } > + bool eq(const Type_handler *handler1, const Type_handler *handler2) const > + { > + return m_handler1 == handler1 && m_handler2 == handler2; > + } > + }; So, we've discussed this before to change to a dynamic array. I prefer the template Dynamic_array over DYNAMIC_ARRAY. > + Element m_element_array[256]; > + uint m_element_count; > + const Element *element(uint i) const { return &m_element_array[i]; } > +public: > + Type_aggregator() > + :m_element_count(0) > + { } > + bool add(const Type_handler *handler1, > + const Type_handler *handler2, > + const Type_handler *result) > + { > + m_element_array[m_element_count]= Element(handler1, handler2, result); > + m_element_count++; > + return false; > + } > + const Element *find_element(const Type_handler *handler1, > + const Type_handler *handler2) const > + { > + for (uint i= 0; i < m_element_count; i++) > + { > + const Element *el= element(i); > + if (el->eq(handler1, handler2) || el->eq(handler2, handler1)) > + return el; > + } > + return NULL; > + } > + const Type_handler *find_handler(const Type_handler *handler1, > + const Type_handler *handler2) const > + { > + const Element *el= find_element(handler1, handler2); > + return el ? el->m_result : NULL; > + } > +}; > + > +extern Type_aggregator type_aggregator_for_result; > > #endif /* SQL_TYPE_H_INCLUDED */

2 2

Re: [Maria-developers] 33b01257190: MDEV-11584: GRANT inside an SP does not work well on 2nd execution
by Sergei Golubchik 28 Dec '16

28 Dec '16

Hi, Oleksandr! On Dec 21, Oleksandr Byelkin wrote: > revision-id: 33b01257190cef414a4b0f00bf36ee6a3ceda0a7 (mariadb-10.1.20-6-g33b01257190) > parent(s): 9e032d6150b39cf36a22e7c1503e06fae9c4016d > committer: Oleksandr Byelkin > timestamp: 2016-12-21 20:11:14 +0100 > message: > > MDEV-11584: GRANT inside an SP does not work well on 2nd execution > > Allocate password hash in statment memory > > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index 3f26bc2e08a..b44b120cf04 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -1067,7 +1067,14 @@ static bool fix_lex_user(THD *thd, LEX_USER *user) > make_scramble= my_make_scrambled_password; > } > > - char *buff= (char *) thd->alloc(scramble_length + 1); > + char *buff; > + { > + Query_arena *arena, backup; > + arena= thd->activate_stmt_arena_if_needed(&backup); > + buff= (char *) thd->alloc(scramble_length + 1); > + if (arena) > + thd->restore_active_arena(arena, &backup); > + } where is it called from, why does it happen during the parsing? > if (buff == NULL) > return true; > make_scramble(buff, user->pwtext.str, user->pwtext.length); Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] Please review MDEV-11672 mysql_list_field() returns wrong default values for VIEW
by Alexander Barkov 28 Dec '16

28 Dec '16

Sanja, On 12/27/2016 09:39 PM, Oleksandr Byelkin wrote: > Hi, Alexander! > > It is OK to push after fixing in the comment of the patch > mysql_list_fields with mysqld_list_fields (IMHO it is server function > do not set type etc.) Thanks for review! I fixed the comment. But then I realized that it's a good idea to add tests for other field types (e.g. DECIMAL, DATETIME, ENUM), and found that my fix was slightly incomplete: Item_ident_for_show::Item_ident_for_show() should copy not only collation from the field, but the entire Type_std_attributes (i.e. unsigned_flag, decimals and max_length). So I also fixed this, with some additional minor changes to avoid code duplication. This is an updated version. Please have a look. Thanks! > > On Tue, Dec 27, 2016 at 4:46 PM, Alexander Barkov <bar(a)mariadb.org > <mailto:bar@mariadb.org>> wrote: > > Hello Sanja, > > Can you please review a patch for MDEV-11672? > > Thanks! > >

1 0

Re: [Maria-developers] [Commits] 9416779: MDEV-11636 Extra persistent columns on slave always gets NULL in RBR
by Kristian Nielsen 28 Dec '16

28 Dec '16

abc <sachin.setiya(a)mariadb.com> writes: > I have updates the patch, please have a look, > revision-id: 941677928aa40aa1f5abf981b74d2c3c80441459 (mariadb-galera-10.0.28-8-g9416779) > parent(s): be430b80df0cdd4eba32df1570195721dbfd1b39 > author: Sachin Setiya > committer: Sachin Setiya > timestamp: 2016-12-27 15:40:28 +0530 > message: > > MDEV-11636 Extra persistent columns on slave always gets NULL in RBR The patch looks ok now. > Problem:- In replication if slave has extra persistent column then these > column are not computed while applying write-set from master. > > Solution:- While applying write-set from server, we will generate values > for extra persistent columns. "write-set" is a Galera term. In replication, they are refered to as binlog events, or row events in the specific case of row-based replication events. > diff --git a/mysql-test/suite/rpl/t/rpl_alter_extra_persistent.test b/mysql-test/suite/rpl/t/rpl_alter_extra_persistent.test > +--echo #SELECT quert > +--connection master > +select * from t1 where a > 2 and a < 4; > + > +--sync_slave_with_master > +select * from t1 where a > 2 and a < 4; You do not need to sync_slave_with_master for SELECT. > diff --git a/sql/rpl_record.cc b/sql/rpl_record.cc > index df8036c..21b8a80 100644 > --- a/sql/rpl_record.cc > +++ b/sql/rpl_record.cc > @@ -408,6 +408,12 @@ unpack_row(rpl_group_info *rgi, > } > > /* > + Add Extra slave persistent columns > + */ > + if ((error= fill_extra_persistent_columns(table, cols->n_bits))) > + DBUG_RETURN(error); You moved this from write_row() to unpack_row(), so I assume this was to fix the similar bug for updates? - Kristian.

2 1

[Maria-developers] check_sql_mode
by jerome brauge 27 Dec '16

27 Dec '16

Hello, I'm trying to add an option to sql_mode. For this, I'm adding in sql_class.h : #define MODE_CONCAT_NULL_YIELDS_NULL (1ULL << 32) But now, my serveur doesn't start and show "Sysvar 'sql_mode' failed 'def_val <= my_set_bits(typelib.count)'" The problem seems to be in sys_vars.ic (Sys_var_set) : { option.var_type|= GET_SET; global_var(ulonglong)= def_val; SYSVAR_ASSERT(typelib.count > 0); SYSVAR_ASSERT(typelib.count <= 64); SYSVAR_ASSERT(def_val <= my_set_bits(typelib.count)); SYSVAR_ASSERT(size == sizeof(ulonglong)); } Function my_set_bits works on 32bit and typelib.count may be up to 64. In my case typelib.count is 33 and my_set_bits(33) returns 1, and the assert is false. What is my mistake ? Best regards, Jérôme.

3 6

[Maria-developers] Please review MDEV-11672 mysql_list_field() returns wrong default values for VIEW
by Alexander Barkov 27 Dec '16

27 Dec '16

Hello Sanja, Can you please review a patch for MDEV-11672? Thanks!

1 0

[Maria-developers] MDEV-11636 Extra persistent columns on slave always gets NULL in RBR
by Sachin Setiya 27 Dec '16

27 Dec '16

Hi Kristian, I have updates the patch, please have a look, http://lists.askmonty.org/pipermail/commits/2016-December/010338.html. I also created a branch bb-10-0-gal-sachin , where I pushed this commit . Build bot status:- https://buildbot.askmonty.org/buildbot/grid?category=main&branch=bb-10.0-ga… -- Regards Sachin Setiya Software Engineer at MariaDB

1 0

Re: [Maria-developers] [Commits] b60fb6d: MDEV-11636 Extra persistent columns on slave always gets NULL in RBR
by Kristian Nielsen 22 Dec '16

22 Dec '16

sachin.setiya(a)mariadb.com writes: > revision-id: b60fb6daee2fcc2f433a50b5a5639065f5a46fe8 (mariadb-galera-10.0.28-8-gb60fb6d) > parent(s): be430b80df0cdd4eba32df1570195721dbfd1b39 > author: Sachin Setiya > committer: Sachin Setiya > timestamp: 2016-12-22 19:22:36 +0530 > message: > > MDEV-11636 Extra persistent columns on slave always gets NULL in RBR > > Problem:- In replication if slave has extra persistent column then these > column are not computed while applying write-set from master. Generally, when slave and master differs there will be situations where things will not fully work. However, in this case, the result is really corrupt data (a persistent column with wrong values), and I agree this should be fixed (and the patch looks simple). I am not familiar with the code for virtual columns. I assume you looked at the normal code path for virtual columns and made similar functionality for row-based replication applier. Your code looks reasonable to me. You add code in Rows_log_event::write_row(). What about update_row? It is not tested in the testcase, you should add test of the UPDATE case (and fix the code, if needed). The testcase needs more work, see comments inline. Getting replication tests to work reliably (not generate occasional spurious errors on buildbot etc.) is quite tricky (spoken from painful experience :-). Please push this to a feature tree and make sure the test passes on all builders before pushing to main. (A useful trick also is to run on your own machine something like: ./mysql-test-run.pl rpl.rpl_alter_extra_persistent --repeat=1000 to catch at least some timing-dependent errors). > diff --git a/mysql-test/suite/rpl/t/rpl_alter_extra_persistent.test b/mysql-test/suite/rpl/t/rpl_alter_extra_persistent.test > +--sync_slave_with_master > + > +connection slave; > +select * from t1; Use ORDER BY a (here and below), to avoid any risk of getting different output order. > +--connection master > +insert into t1 values(5); > +insert into t1 values(6); > + > +--connection slave > +sleep 2; Never use sleep in test cases. Use an appropriate mechanism to sync the slave to the master. > +select * from t1; > + > +--echo #Break Unique Constraint > +alter table t1 add column z4 int as (a % 6) persistent unique; > + > +--connection master > + > +--echo #entering duplicate value for slave persistent column > +insert into t1 values(7); > +select * from t1; > + > +--connection slave > +select * from t1; > +--query_vertical show slave status; This will not work reliably. You cannot put SHOW SLAVE STATUS output in .result, it will not be stable between test runs. And there is a race on whether the slave has time to hit the duplicate error. For this, use the existing include/wait_for_slave_sql_error.inc file, which can reliably detect a slave failure. See comments in the file and example usage in other test files. > + > +--connection master > +drop table t1; > + > +--connection slave > +alter table t1 drop column z4 ; > +start slave; > +drop table t1; This drop table should not be there (the drop table from the master will be replicated). > diff --git a/sql/log_event.cc b/sql/log_event.cc > index 1cc58cd..a8f035b 100644 > --- a/sql/log_event.cc > +++ b/sql/log_event.cc > @@ -11541,6 +11541,9 @@ Rows_log_event::write_row(rpl_group_info *rgi, > DBUG_RETURN(error); > } > > + if ((error= fill_extra_persistent_columns(table, &m_cols)) > + DBUG_RETURN(error); > + Is similar code needed for update_row? At least, this should be tested in the test case. - Kristian.

1 0

[Maria-developers] aria engine crash at recovery
by Roberto Spadim 22 Dec '16

22 Dec '16

hi guys i got a crash and aria engine don't startup, any help? 161222 03:41:13 mysqld_safe Starting mysqld daemon with databases from /home/mysql/data/local/ 2016-12-22 3:41:13 139878188566400 [Warning] 'THREAD_CONCURRENCY' is deprecated and will be removed in a future release. 2016-12-22 3:41:13 139878188566400 [Note] /usr/mariadb/mariadb/bin//mysqld (mysqld 10.1.5-MariaDB-log) starting as process 19217 ... 2016-12-22 3:41:13 139878188566400 [Note] mysqld: Aria engine: starting recovery recovered pages: 0% 3082852397% 0%161222 3:41:13 [ERROR] mysqld got signal 11 ; This could be because you hit a bug. It is also possible that this binary or one of the libraries it was linked against is corrupt, improperly built, or misconfigured. This error can also be caused by malfunctioning hardware. To report this bug, see http://kb.askmonty.org/en/reporting-bugs We will try our best to scrape up some info that will hopefully help diagnose the problem, but since we have already crashed, something is definitely wrong and this may fail. Server version: 10.1.5-MariaDB-log key_buffer_size=629145600 read_buffer_size=4194304 max_used_connections=0 max_threads=52 thread_count=0 It is possible that mysqld could use up to key_buffer_size + (read_buffer_size + sort_buffer_size)*max_threads = 1041422 K bytes of memory Hope that's ok; if not, decrease some variables in the equation. Thread pointer: 0x0x0 Attempting backtrace. You can use the following information to find out where mysqld died. If you see no messages after this, something went terribly wrong... stack_bottom = 0x0 thread_stack 0x48000 mysys/stacktrace.c:247(my_print_stacktrace)[0x7f37ee645b0b] sql/signal_handler.cc:153(handle_fatal_signal)[0x7f37ee1dfdb5] /lib64/libpthread.so.0(+0x10d00)[0x7f37ecd72d00] maria/ma_recovery.c:2142(exec_REDO_LOGREC_CLR_END)[0x7f37ee5683b7] maria/ma_recovery.c:588(display_and_apply_record)[0x7f37ee56985d] maria/ma_recovery.c:350(maria_apply_log)[0x7f37ee56d45c] maria/ma_recovery.c:242(maria_recovery_from_log)[0x7f37ee56df1e] maria/ha_maria.cc:3594(ha_maria_init)[0x7f37ee546c2d] sql/handler.cc:512(ha_initialize_handlerton(st_plugin_int*))[0x7f37ee1e2518] sql/sql_plugin.cc:1403(plugin_initialize)[0x7f37ee089e65] sql/sql_plugin.cc:1675(plugin_init(int*, char**, int))[0x7f37ee08add2] sql/mysqld.cc:5064(init_server_components)[0x7f37edfc37e8] sql/mysqld.cc:5659(mysqld_main(int, char**))[0x7f37edfc61ca] /lib64/libc.so.6(__libc_start_main+0xf0)[0x7f37ec3a79f0] /usr/mariadb/mariadb/bin//mysqld(+0x39dbad)[0x7f37edfbabad]

1 4

[Maria-developers] How correctly allocate memory to avoid problems with 2nd execution (instruction)
by Oleksandr Byelkin 22 Dec '16

22 Dec '16

Hi! It is for all who decide to write or already is writing something for server. There is 2 ways to execute query/statement: 1) conventional (one time) 2) with storing the statement and executing it many times (Prepared statements (PS)/ Stored Procedures(SP)) Also there is so called Query_arena, which is a MEM_ROOT and list of allocated Item-s in it. (we need list of items because they could require not trivial destruction/cleanup procedure before freeing memory, see cleanup() methods of Item-s) For conventional execution there is only one (run-time) query arena (part of THD in this case) where all memory allocated, and all Item-s linked. After execution Item-s cleaned up and memory freed. For case with stored statements (PS/SP) there is one Query_arena where Parsing result allocated (statement arena), and the other one so called run-time arena (which freed after execution as for conventional execution). During parsing statement arena is active (default), during all execution steps the run-time arena is active (if nothing changed it specially). So if some one decided allocate some memory during execution one should think if it is: I) only for execution II) should live with the statement All references on the memory allocated as case I) should be returned in the state as it was before execution or taken other measures to prevent using them during the next execution. All object which allocated as persistent (for statement of course) should be a) allocated only once (can be first execution or prepare phase, it does not matter, what does matter: do not make memory leak by allocating on each execution) b) reference on them should not be lost (because it supposed to be reused during next execution) c) procedure of allocation during run-time should looks like this: Query_arena *arena, backup; // backup run-time arena and set as default statement arena if needed // (if it is not conventional execution) arena= thd->activate_stmt_arena_if_needed(&backup);\ <MAKE ALLOOCATION HERE USING thd->mem_root> // return old default arena if it was changed before if (arena) thd->restore_active_arena(arena, &backup);

1 0