September 2020 - developers - lists.mariadb.org

Re: [Maria-developers] New patch on the JSON_TABLE.
by Sergey Petrunia 23 Sep '20

23 Sep '20

One more thing: On Mon, Sep 14, 2020 at 03:38:38PM +0400, Alexey Botchkov wrote: > CREATE TABLE t1(id INT, f1 JSON); > INSERT INTO t1 VALUES (1, '{\"1\": 1}'), (2, '{\"1\": 2}'), (3, '{\"1\": > 3}'), > (4, '{\"1\": 4}'), (5, '{\"1\": 5}'), (6, '{\"1\": 6}'); > > SELECT * FROM t1 WHERE id IN > (SELECT id FROM t1 as tt2, > JSON_TABLE(f1, "$" COLUMNS (jf FOR ORDINALITY)) AS tbl); I've noticed that Optimizer Trace is not saved for this query. I get MariaDB [test]> select * from information_schema.optimizer_trace\G *************************** 1. row *************************** QUERY: TRACE: MISSING_BYTES_BEYOND_MAX_MEM_SIZE: 0 INSUFFICIENT_PRIVILEGES: 1 1 row in set (0.001 sec) INSUFFICIENT_PRIVILEGES: 1! This is only supposed to happen when the query uses VIEWs which definition cannot be viewed by the user executing the query. Could you investigate and fix this please? BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

[Maria-developers] On bb-10.6-monty work
by Nikita Malyavin 16 Sep '20

16 Sep '20

Hello, Monty! I've been asked to review your recent work regarding memory footprint optimizations. I went through all the work done, and mostly liked the changes done. So here are only two commits i'd like to highlight > commit bf8bd0573440dc39490eb795b5455735dd63132b > Author: Monty <monty(a)mariadb.org> > Date: Tue Jul 14 18:36:05 2020 +0300 > > MDEV-23001 Precreate static Item_bool() to simplify code > > The following changes where done: > - Create global Item: Item_false and Item_true > - Replace all creation if 'FALSE' and 'TRUE' top level items used for > WHERE/HAVING/ON clauses to use Item_false and Item_true. > > The benefit are: > - Less and faster code > - No test needed if we where able to create the new item. > - Fixed possible errors if 'new' would have failed for the Item_bool's > > diff --git a/sql/item.h b/sql/item.h > index c4fbf8f9c0a..4cdee637415 100644 > --- a/sql/item.h > +++ b/sql/item.h > @@ -4236,6 +4239,23 @@ class Item_bool :public Item_int > }; > > > +class Item_bool_static :public Item_bool > +{ > +public: > + Item_bool_static(const char *str_arg, longlong i): > + Item_bool(NULL, str_arg, i) {}; > + > + /* Create dummy functions for things that may change the static item */ > + void fix_length_and_charset(uint32 max_char_length_arg, CHARSET_INFO *cs) > + {} > + void fix_char_length(size_t max_char_length_arg) > + {} > + void set_join_tab_idx(uint join_tab_idx_arg) > + { DBUG_ASSERT(0); } > +}; > + > +extern const Item_bool_static Item_false, Item_true; > + > class Item_uint :public Item_int > { > public: Do we really need an additional class for static Item_bool's? I know that we have constructors hidden for Item, so maybe just unhide it in Item_bool, and that's it. Besides, I see fix_char_length is not virtual at all, so reimplementing it has no efect. And why can't we do all that in Item_bool itself? Note that adding new class increases a virtual method table, and it's huge already, for Item hierarchy. We could also add some other frequently used Items, like 0, 1, '', NULL, to a static memory. > commit 6ca3ab6c2c18cdbf614984cb9d5312d58e718372 > Author: Monty <monty(a)mariadb.org> > Date: Wed Sep 2 03:13:32 2020 +0300 > > Split item->flags into base_flags and with_flags > > This was done to simplify copying of with_* flags > > Other things: > - Changed Flags to C++ enums, which enables gdb to print > out bit values for the flags. This also enables compiler > errors if one tries to manipulate a non existing bit in > a variable. > - Added set_maybe_null() as a shortcut as setting the > MAYBE_NULL flags was used in a LOT of places. > - Renamed PARAM flag to SP_VAR to ensure it's not confused with persistent > statement parameters. > > diff --git a/sql/item.h b/sql/item.h > index 09618bbb52c..9a794d96982 100644 > --- a/sql/item.h > +++ b/sql/item.h > @@ -717,28 +717,94 @@ class Item_const > > #define STOP_PTR ((void *) 1) > > -/* Bits used in Item.flags */ > -/* If item may be null */ > -#define ITEM_FLAG_MAYBE_NULL_SHIFT 0 > -#define ITEM_FLAG_MAYBE_NULL (1<<ITEM_FLAG_MAYBE_NULL_SHIFT) > -/* If used in GROUP BY list of a query with ROLLUP */ > -#define ITEM_FLAG_IN_ROLLUP (1<<1) > -/* If Item contains an SP parameter */ > -#define ITEM_FLAG_WITH_PARAM (1<<2) > -/* If item contains a window func */ > -#define ITEM_FLAG_WITH_WINDOW_FUNC (1<<3) > -/* True if any item except Item_sum contains a field. Set during parsing. */ > -#define ITEM_FLAG_WITH_FIELD (1<<4) > -/* If item was fixed with fix_fields */ > -#define ITEM_FLAG_FIXED (1<<5) > -/* Indicates that name of this Item autogenerated or set by user */ > -#define ITEM_FLAG_IS_AUTOGENERATED_NAME (1 << 6) > -/* Indicates that this item is in CYCLE clause of WITH */ > -#define ITEM_FLAG_IS_IN_WITH_CYCLE (1<<7) > -/* True if item contains a sum func */ > -#define ITEM_FLAG_WITH_SUM_FUNC (1<< 8) > -/* True if item containts a sub query */ > -#define ITEM_FLAG_WITH_SUBQUERY (1<< 9) > + > +/* Base flags (including IN) for an item */ > + > + > +typedef uint8 item_flags_t; > + > +enum class item_base_t : item_flags_t > +{ > + NONE= 0, > +#define ITEM_FLAGS_MAYBE_NULL_SHIFT 0 // Must match MAYBE_NULL > + MAYBE_NULL= (1<<0), // May be NULL. > + IN_ROLLUP= (1<<1), // Appears in GROUP BY list > + // of a query with ROLLUP. > + FIXED= (1<<2), // Was fixed with fix_fields(). > + IS_AUTOGENERATED_NAME= (1<<3), // The name if this Item was > + // generated or set by the user. > + IS_IN_WITH_CYCLE= (1<<4) // This item is in CYCLE clause > + // of WITH. > +}; > + > + > +/* Flags that tells us what kind of items the item contains */ > + > +enum class item_with_t : item_flags_t > +{ > + NONE= 0, > + SP_VAR= (1<<0), // If Item contains a stored procedure variable > + WINDOW_FUNC= (1<<1), // If item contains a window func > + FIELD= (1<<2), // If any item except Item_sum contains a field. > + SUM_FUNC= (1<<3), // If item contains a sum func > + SUBQUERY= (1<<4) // If item containts a sub query > +}; > + > + > +/* Make operations in item_base_t and item_with_t work like 'int' */ > +static inline item_base_t operator&(const item_base_t a, const item_base_t b) > +{ > + return (item_base_t) (((item_flags_t) a) & ((item_flags_t) b)); > +} > + > +static inline item_base_t & operator&=(item_base_t &a, item_base_t b) > +{ > + a= (item_base_t) (((item_flags_t) a) & (item_flags_t) b); > + return a; > +} > + > +static inline item_base_t operator|(const item_base_t a, const item_base_t b) > +{ > + return (item_base_t) (((item_flags_t) a) | ((item_flags_t) b)); > +} > + > +static inline item_base_t & operator|=(item_base_t &a, item_base_t b) > +{ > + a= (item_base_t) (((item_flags_t) a) | (item_flags_t) b); > + return a; > +} > + > +static inline item_base_t operator~(const item_base_t a) > +{ > + return (item_base_t) ~(item_flags_t) a; > +} > + > +static inline item_with_t operator&(const item_with_t a, const item_with_t b) > +{ > + return (item_with_t) (((item_flags_t) a) & ((item_flags_t) b)); > +} > + > +static inline item_with_t & operator&=(item_with_t &a, item_with_t b) > +{ > + a= (item_with_t) (((item_flags_t) a) & (item_flags_t) b); > + return a; > +} > + > +static inline item_with_t operator|(const item_with_t a, const item_with_t b) > +{ > + return (item_with_t) (((item_flags_t) a) | ((item_flags_t) b)); > +} > + > +static inline item_with_t & operator|=(item_with_t &a, item_with_t b) > +{ > + a= (item_with_t) (((item_flags_t) a) | (item_flags_t) b); > + return a; > +} > + > +static inline item_with_t operator~(const item_with_t a) > +{ > + return (item_with_t) ~(item_flags_t) a; > +} > > > class Item :public Value_source, > @@ -932,8 +998,8 @@ class Item :public Value_source, > const char *orig_name; > > /* All common bool variables for an Item is stored here */ > - typedef uint16 item_flags_t; > - item_flags_t flags; > + item_base_t base_flags; > + item_with_t with_flags; > > /* Marker is used in some functions to temporary mark an item */ > int16 marker; Nice experimentation! I wanted to suggest using constexprs inside Item, but this is even better, because you cant freely add a bit that not in the enum. Though I think it is too bloaty to specify all the operators each time, so I'd prefer to see duplications metaprogrammed out somehow. There is no sane way in C++ (even in modern one) to specify traits, which extend the class functionality (see Golang's traits), so I'd suggest to use good old preprocessor way, like following: #define T item_base_t #include "setup_bitwise_operators.inc" #define T item_with_t #include "setup_bitwise_operators.inc" and then in setup_bitwise_operators.inc: static inline T operator&(const T a, const T b) { return (T) (((item_flags_t) a) & ((item_flags_t) b)); } And so on. Another variant I've just googled is to make a template operator for all types, but restrict its applicatory type set with std::enable_if<T>: https://www.justsoftwaresolutions.co.uk/cplusplus/using-enum-classes-as-bit… So we would only need to add a structure: template<> struct enable_bitmask_operators<item_base_t>{ static constexpr bool enable=true; }; To work. Thereby, the first approach is: + Good old way + Works fast - not a C++ way, though we work in a C++-only file The second approach is: + modern + C++ way - may compile slow - may produce uncomfortable errors when compiler tries to deduce a function/operator to apply Up to you whic way to use, but I think it should be done, since we have many other flag sets, which are likely to be prettified with the same style. -- Yours truly, Nikita Malyavin

2 3

[Maria-developers] MDEV-21829:Followup: On Unique object and variable-size keys
by Sergey Petrunia 15 Sep '20

15 Sep '20

Hi Varun, I was looking at the code for MDEV-21829, trying to understand whether there are any issues with how Unique object was extended to handle variable-size keys. == TREE object (the RB tree) == TREE (the structure and functions that operate on it) support variable-sized keys, however the keys "are expected to know" their size. tree_insert() has "uint key_size" argument. This way, tree_insert() knows how many bytes to allocate and copy when putting the key into the tree. The rest of the TREE code doesn't seem to care about the size of the keys. When TREE code needs to compare a couple of keys, it will call TREE::compare callack. The callback only receives key pointers as arguments, that is, it is expected to infer key size from the contents of the key. The same happens when when walking the tree. Only "uchar *key" is passed to tree_walk_action callback. The walk function has to figure out they key size on its own. == Unique == Unique class cannot do the same what TREE functions function do. It needs to be aware of the sizes of the values it is storing: - It will need to pass value size to tree_insert() - It will need to know value size to write it into tmp file. - It will need to read it back when merging. How does it know it? Unique::merge() and Unique::walk() with merge_walk() functions do make certain assumptions about the data format being used. == The review points == Is this bad? I think tight coupling between the Unique object and the specifics of the data format it is storing is bad. What is worse is that currently this coupling is implicit, it is not immediately apparent to somebody just looking at the code. Possible ways out: 1. Explicitly declare in Unique class that variable-sized records must use certain data format. 2. Isolate the Unique from the specifics of the data format used. 2A. Let Unique get the size of the key as argument of unique_add() and then keep it together with the key. This creates some RAM/storage overhead. 2B. Pass a read_size() callback function which would get the size from the data? BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

2 1

Re: [Maria-developers] 099596296ce: Avoid mallocs when using LONGLONG_BUFFER_SIZE
by Sergei Golubchik 13 Sep '20

13 Sep '20

Hi, Michael! This is a good catch! But note that LONGLONG_BUFFER_SIZE already reserves +1 for '\0', it's defined as /* Max length needed for a buffer to hold a longlong or ulonglong + end \0 */ #define LONGLONG_BUFFER_SIZE 21 The problem is that String::set_int doesn't use LONGLONG_BUFFER_SIZE, it uses 20*cs->mbmaxlen+1 This +1 is supposed to reserve one byte for '\0'. Which is incorrect, because alloc() does it too, so a better fix would be to remove +1 from String::set_int(). On Sep 08, Michael Widenius wrote: > revision-id: 099596296ce (mariadb-10.5.2-272-g099596296ce) > parent(s): 2de0c74f94c > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2020-09-03 15:17:56 +0300 > message: > > Avoid mallocs when using LONGLONG_BUFFER_SIZE > > When using String::alloc() to allocate a string, the String ensures that > there is space for an extra NULL byte in the buffer and if not, reallocates > the string. This is a problem with the String::set_int() that calls > alloc(LONGLONG_BUFFER_SIZE), which forces an extra malloc/free to > happen. > > Fixed by using LONGLONG_BUFFER_SIZE+1 for StringBuffer's. > > diff --git a/sql/field.cc b/sql/field.cc > index ab7254e483d..2be4da8d4a8 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -1085,7 +1085,7 @@ Field_longstr::make_packed_sort_key_part(uchar *buff, > uchar* > Field_longstr::pack_sort_string(uchar *to, const SORT_FIELD_ATTR *sort_field) > { > - StringBuffer<LONGLONG_BUFFER_SIZE> buf; > + StringBuffer<LONGLONG_BUFFER_SIZE+1> buf; > val_str(&buf, &buf); > return to + sort_field->pack_sort_string(to, &buf, field_charset()); > } > diff --git a/sql/item.cc b/sql/item.cc > index 4a26cb6c140..0d947749e01 100644 > --- a/sql/item.cc > +++ b/sql/item.cc > @@ -3612,7 +3612,7 @@ String *Item_int::val_str(String *str) > > void Item_int::print(String *str, enum_query_type query_type) > { > - StringBuffer<LONGLONG_BUFFER_SIZE> buf; > + StringBuffer<LONGLONG_BUFFER_SIZE+1> buf; > // my_charset_bin is good enough for numbers > buf.set_int(value, unsigned_flag, &my_charset_bin); > str->append(buf); > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] 23814b75241: Report memory leaks from mariadbd if -T or --debug is used
by Sergei Golubchik 11 Sep '20

11 Sep '20

Hi, Michael! ok to push, of course On Sep 08, Michael Widenius wrote: > revision-id: 23814b75241 (mariadb-10.5.2-273-g23814b75241) > parent(s): 099596296ce > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2020-09-03 15:17:56 +0300 > message: > > Report memory leaks from mariadbd if -T or --debug is used > > Before memory leaks was only reported if server stopped normally. > This made it harder to find out where the leaks happened when > debugging test cases. > > diff --git a/sql/mysqld.cc b/sql/mysqld.cc > index 331684ff728..7468f436a03 100644 > --- a/sql/mysqld.cc > +++ b/sql/mysqld.cc > @@ -1939,7 +1939,7 @@ static void mysqld_exit(int exit_code) > { > fprintf(stderr, "Warning: Memory not freed: %lld\n", > (longlong) global_status_var.global_memory_used); > - if (exit_code == 0) > + if (exit_code == 0 || opt_endinfo) > SAFEMALLOC_REPORT_MEMORY(0); > } > DBUG_LEAVE; > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 2de0c74f94c: Optimize Sql_alloc
by Sergei Golubchik 11 Sep '20

11 Sep '20

Hi, Michael! ok to push On Sep 08, Michael Widenius wrote: > revision-id: 2de0c74f94c (mariadb-10.5.2-271-g2de0c74f94c) > parent(s): 21eb8969ce9 > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2020-09-03 15:17:56 +0300 > message: > > Optimize Sql_alloc > > - Remove 'dummy_for_valgrind' overrun marker as this doesn't help much. > The element also distorts the sizes of objects a bit, which makes it > harder to calculate gain in object sizes when doing size optimizations. > - Avoid one extra call indirection when using thd_get_current_thd(), which > is used by Sql_alloc. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] c3ecf0d6243: Optimize size of lex structures
by Sergei Golubchik 09 Sep '20

09 Sep '20

Hi, Michael! On Sep 08, Michael Widenius wrote: > revision-id: c3ecf0d6243 (mariadb-10.5.2-269-gc3ecf0d6243) > parent(s): c79c6f78205 > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2020-09-02 20:58:33 +0300 > message: > > Optimize size of lex structures > > LEX, st_select_lex, st_select_unit optimized for space: > - Use bit fields for bool variables > - Ensure that all bit fields are initialized (improves > performance for init functions as all bit fields can be > initalized with one memory access) > - Move members around in above structures to remove alignment > gaps > > diff --git a/sql/sql_lex.h b/sql/sql_lex.h > index f516219c01a..1f6e50d8b60 100644 > --- a/sql/sql_lex.h > +++ b/sql/sql_lex.h > @@ -725,12 +725,15 @@ class st_select_lex_node { > st_select_lex_node *next, **prev, /* neighbor list */ > *master, *slave, /* vertical links */ > *link_next, **link_prev; /* list of whole SELECT_LEX */ > + enum sub_select_type linkage; > > void init_query_common(); > -public: > > +public: > ulonglong options; > - > + uint8 uncacheable; > + bool distinct:1; > + bool no_table_names_allowed:1; /* used for global order by */ I thought you didn't like this approach and preferred flags & DISTINCT instead. Either you forgot to change LEX to flags or you forgot to squash your commits. > /* > result of this query can't be cached, bit field, can be : Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] c79c6f78205: Added typedef decimal_digits_t (uint16) for number of decimals
by Sergei Golubchik 09 Sep '20

09 Sep '20

Hi, Michael! On Sep 08, Michael Widenius wrote: > revision-id: c79c6f78205 (mariadb-10.5.2-268-gc79c6f78205) > parent(s): bf8bd057344 > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2020-09-02 20:58:33 +0300 > message: > > Added typedef decimal_digits_t (uint16) for number of decimals > > For fields and Item's uint8 should be good enough. After > discussions with Alexander Barkov we choose uint16 (for now) > as some format functions may accept +256 digits. > > The reason for this patch was to make the storage of decimal > digits simlar. Before this patch decimals was stored/used as > uint8, int and uint. > > Changed most decimal variables and functions to use the new > typedef. > > diff --git a/include/decimal.h b/include/decimal.h > index cab18f99348..f713409ede1 100644 > --- a/include/decimal.h > +++ b/include/decimal.h > @@ -53,10 +53,11 @@ int decimal2double(const decimal_t *from, double *to); > int double2decimal(double from, decimal_t *to); > int decimal_actual_fraction(const decimal_t *from); > int decimal2bin(const decimal_t *from, uchar *to, int precision, int scale); > -int bin2decimal(const uchar *from, decimal_t *to, int precision, int scale); > +int bin2decimal(const uchar *from, decimal_t *to, int precision, > + decimal_digits_t scale); I don't understand. Is it the type for precision (max number of digits in a decimal number) or for scale (mux number of digits after the dot)? If the former - why did you not change the type of the precision argument? If the latter - why is it called not decimal_scale_t? I personally think the former, using decimal_digits_t for both precision and scale, should be fine. > -int decimal_size(int precision, int scale); > -int decimal_bin_size(int precision, int scale); > +int decimal_size(int precision, decimal_digits_t scale); > +int decimal_bin_size(int precision, decimal_digits_t scale); > int decimal_result_size(decimal_t *from1, decimal_t *from2, char op, > int param); > > diff --git a/include/m_ctype.h b/include/m_ctype.h > index 59ac7814aee..34821f76474 100644 > --- a/include/m_ctype.h > +++ b/include/m_ctype.h > @@ -79,6 +79,7 @@ typedef const struct my_collation_handler_st MY_COLLATION_HANDLER; > typedef const struct unicase_info_st MY_UNICASE_INFO; > typedef const struct uni_ctype_st MY_UNI_CTYPE; > typedef const struct my_uni_idx_st MY_UNI_IDX; > +typedef uint16 decimal_digits_t; A bit strange to see it here and not in decimal.h above. > > typedef struct unicase_info_char_st > { > diff --git a/plugin/type_inet/sql_type_inet.h b/plugin/type_inet/sql_type_inet.h > index 8c42431ccaa..df74b91172a 100644 > --- a/plugin/type_inet/sql_type_inet.h > +++ b/plugin/type_inet/sql_type_inet.h > @@ -402,19 +402,19 @@ class Type_handler_inet6: public Type_handler > bool can_return_time() const override { return false; } > bool convert_to_binary_using_val_native() const override { return true; } > > - uint Item_time_precision(THD *thd, Item *item) const override > + decimal_digits_t Item_time_precision(THD *thd, Item *item) const override here you return the precision using the decimal_digits_t type. So it seems that it should apply to both precision and scale. > { > return 0; > } > - uint Item_datetime_precision(THD *thd, Item *item) const override > +decimal_digits_t Item_datetime_precision(THD *thd, Item *item) const override > { > return 0; > } > - uint Item_decimal_scale(const Item *item) const override > + decimal_digits_t Item_decimal_scale(const Item *item) const override > { > return 0; > } > - uint Item_decimal_precision(const Item *item) const override > + decimal_digits_t Item_decimal_precision(const Item *item) const override > { > /* > This will be needed if we ever allow cast from INET6 to DECIMAL. > diff --git a/sql/field.cc b/sql/field.cc > index f50ddec1c80..a5d00dd6fd5 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -3242,10 +3242,11 @@ Field *Field_decimal::make_new_field(MEM_ROOT *root, TABLE *new_table, > ** Field_new_decimal > ****************************************************************************/ > > -static uint get_decimal_precision(uint len, uint8 dec, bool unsigned_val) > +static decimal_digits_t get_decimal_precision(uint len, decimal_digits_t dec, > + bool unsigned_val) > { > uint precision= my_decimal_length_to_precision(len, dec, unsigned_val); shouldn't my_decimal_length_to_precision() return decimal_digits_t result? > - return MY_MIN(precision, DECIMAL_MAX_PRECISION); > + return (decimal_digits_t) MY_MIN(precision, DECIMAL_MAX_PRECISION); > } > > Field_new_decimal::Field_new_decimal(uchar *ptr_arg, > diff --git a/sql/item.h b/sql/item.h > index 4cdee637415..948bb85f253 100644 > --- a/sql/item.h > +++ b/sql/item.h > @@ -1644,14 +1644,14 @@ class Item: public Value_source, > return type_handler()->Item_decimal_precision(this); > } > /* Returns the number of integer part digits only */ > - inline int decimal_int_part() const > - { return my_decimal_int_part(decimal_precision(), decimals); } > + inline decimal_digits_t decimal_int_part() const > + { return (decimal_digits_t) my_decimal_int_part(decimal_precision(), decimals); } again, shouldn't my_decimal_int_part() return decimal_digits_t? > /* > Returns the number of fractional digits only. > NOT_FIXED_DEC is replaced to the maximum possible number > of fractional digits, taking into account the data type. > */ > - uint decimal_scale() const > + decimal_digits_t decimal_scale() const > { > return type_handler()->Item_decimal_scale(this); > } > @@ -4856,7 +4856,7 @@ class Item_temporal_literal :public Item_literal > Item_literal(thd) > { > collation= DTCollation_numeric(); > - decimals= dec_arg; > + decimals= (decimal_digits_t) dec_arg; shouldnt dec_arg be decimal_digits_t? > cached_time= *ltime; > } > > diff --git a/sql/item_func.cc b/sql/item_func.cc > index 8a75d2c9946..805647d2a04 100644 > --- a/sql/item_func.cc > +++ b/sql/item_func.cc > @@ -2713,7 +2713,7 @@ my_decimal *Item_func_round::decimal_op(my_decimal *decimal_value) > dec= INT_MIN; > > if (!(null_value= (value.is_null() || args[1]->null_value || > - value.round_to(decimal_value, (uint) dec, > + value.round_to(decimal_value, (int) dec, You don't need to cast uint16 to int explicitly > truncate ? TRUNCATE : HALF_UP) > 1))) > return decimal_value; > return 0; > diff --git a/sql/item_func.h b/sql/item_func.h > index 5b4acdce1c6..d5aa1477ff3 100644 > --- a/sql/item_func.h > +++ b/sql/item_func.h > @@ -1369,10 +1369,10 @@ class Item_decimal_typecast :public Item_func > { > my_decimal decimal_value; > public: > - Item_decimal_typecast(THD *thd, Item *a, uint len, uint dec) > + Item_decimal_typecast(THD *thd, Item *a, uint len, decimal_digits_t dec) > :Item_func(thd, a) > { > - decimals= (uint8) dec; > + decimals= (decimal_digits_t) dec; you don't need a cast here > collation= DTCollation_numeric(); > fix_char_length(my_decimal_precision_to_length_no_truncation(len, dec, > unsigned_flag)); > diff --git a/sql/my_decimal.cc b/sql/my_decimal.cc > index edf3e82de40..ac86ff71b64 100644 > --- a/sql/my_decimal.cc > +++ b/sql/my_decimal.cc > @@ -198,7 +198,8 @@ str_set_decimal(uint mask, const my_decimal *val, > E_DEC_OVERFLOW > */ > > -int my_decimal::to_binary(uchar *bin, int prec, int scale, uint mask) const > +int my_decimal::to_binary(uchar *bin, int prec, decimal_digits_t scale, again, shouldn't prec be decimal_digits_t too? > + uint mask) const > { > int err1= E_DEC_OK, err2; > my_decimal rounded; > @@ -329,7 +330,7 @@ my_decimal *date2my_decimal(const MYSQL_TIME *ltime, my_decimal *dec) > } > > > -void my_decimal_trim(ulonglong *precision, uint *scale) > +void my_decimal_trim(ulonglong *precision, decimal_digits_t *scale) and here? > { > if (!(*precision) && !(*scale)) > { > diff --git a/sql/my_decimal.h b/sql/my_decimal.h > index 2db08bf01e3..9f7232f94b4 100644 > --- a/sql/my_decimal.h > +++ b/sql/my_decimal.h > @@ -50,7 +50,7 @@ typedef struct st_mysql_time MYSQL_TIME; > #define DECIMAL_MAX_FIELD_SIZE DECIMAL_MAX_PRECISION > > > -inline uint my_decimal_size(uint precision, uint scale) > +inline uint my_decimal_size(uint precision, decimal_digits_t scale) and here? (and in many places below, that I'll stop commenting on) > { > /* > Always allocate more space to allow library to put decimal point > diff --git a/sql/sql_type.cc b/sql/sql_type.cc > index 9e0f9b013c0..6569ea1a415 100644 > --- a/sql/sql_type.cc > +++ b/sql/sql_type.cc > @@ -1197,11 +1198,12 @@ Type_numeric_attributes::aggregate_numeric_attributes_decimal(Item **item, > uint nitems, > bool unsigned_arg) > { > - int max_int_part= find_max_decimal_int_part(item, nitems); > + decimal_digits_t max_int_part= find_max_decimal_int_part(item, nitems); > decimals= find_max_decimals(item, nitems); > - int precision= MY_MIN(max_int_part + decimals, DECIMAL_MAX_PRECISION); > + decimal_digits_t precision= (decimal_digits_t) why do you need a cast here? > + MY_MIN(max_int_part + decimals, DECIMAL_MAX_PRECISION); > max_length= my_decimal_precision_to_length_no_truncation(precision, > - (uint8) decimals, > + decimals, > unsigned_flag); > } > > @@ -6910,20 +6912,20 @@ bool Type_handler_string_result:: > > /***************************************************************************/ > > -uint Type_handler::Item_time_precision(THD *thd, Item *item) const > +decimal_digits_t Type_handler::Item_time_precision(THD *thd, Item *item) const > { > return MY_MIN(item->decimals, TIME_SECOND_PART_DIGITS); note, no cast here (good) > } > > > -uint Type_handler::Item_datetime_precision(THD *thd, Item *item) const > +decimal_digits_t Type_handler::Item_datetime_precision(THD *thd, Item *item) const > { > return MY_MIN(item->decimals, TIME_SECOND_PART_DIGITS); and not here > } > > > -uint Type_handler_string_result::Item_temporal_precision(THD *thd, Item *item, > - bool is_time) const > +decimal_digits_t Type_handler_string_result:: > +Item_temporal_precision(THD *thd, Item *item, bool is_time) const > { > StringBuffer<64> buf; > String *tmp; > @@ -6939,34 +6941,34 @@ uint Type_handler_string_result::Item_temporal_precision(THD *thd, Item *item, > Datetime(thd, &status, tmp->ptr(), tmp->length(), tmp->charset(), > Datetime::Options(TIME_FUZZY_DATES, TIME_FRAC_TRUNCATE)). > is_valid_datetime())) > - return MY_MIN(status.precision, TIME_SECOND_PART_DIGITS); > - return MY_MIN(item->decimals, TIME_SECOND_PART_DIGITS); > + return (decimal_digits_t) MY_MIN(status.precision, TIME_SECOND_PART_DIGITS); > + return (decimal_digits_t) MY_MIN(item->decimals, TIME_SECOND_PART_DIGITS); and here again. why? (and below too, I'll stop commenting on casts now) > } > > diff --git a/sql/sql_type.h b/sql/sql_type.h > index a6d85b5bb47..7b1a4f730db 100644 > --- a/sql/sql_type.h > +++ b/sql/sql_type.h > @@ -2955,9 +2955,10 @@ class Type_numeric_attributes > class Type_temporal_attributes: public Type_numeric_attributes > { > public: > - Type_temporal_attributes(uint int_part_length, uint dec, bool unsigned_arg) > + Type_temporal_attributes(uint32 int_part_length, decimal_digits_t dec, bool unsigned_arg) > :Type_numeric_attributes(int_part_length + (dec ? 1 : 0), > - MY_MIN(dec, TIME_SECOND_PART_DIGITS), > + MY_MIN(dec, > + (decimal_digits_t) TIME_SECOND_PART_DIGITS), this should not be necessary > unsigned_arg) > { > max_length+= decimals; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] bf8bd057344: MDEV-23001 Precreate static Item_bool() to simplify code
by Sergei Golubchik 08 Sep '20

08 Sep '20

Hi, Michael! On Sep 08, Michael Widenius wrote: > revision-id: bf8bd057344 (mariadb-10.5.2-267-gbf8bd057344) > parent(s): 32a29afea77 > author: Michael Widenius <michael.widenius(a)gmail.com> > committer: Michael Widenius <michael.widenius(a)gmail.com> > timestamp: 2020-09-02 20:58:32 +0300 > message: > > MDEV-23001 Precreate static Item_bool() to simplify code > > The following changes where done: > - Create global Item: Item_false and Item_true > - Replace all creation if 'FALSE' and 'TRUE' top level items used for > WHERE/HAVING/ON clauses to use Item_false and Item_true. > > The benefit are: > - Less and faster code > - No test needed if we where able to create the new item. > - Fixed possible errors if 'new' would have failed for the Item_bool's > > diff --git a/sql/item.cc b/sql/item.cc > index dbf20f31d7a..b24103f2330 100644 > --- a/sql/item.cc > +++ b/sql/item.cc > @@ -55,7 +55,8 @@ const char *item_empty_name=""; > const char *item_used_name= "\0"; > > static int save_field_in_field(Field *, bool *, Field *, bool); > - > +const Item_bool_static Item_false("FALSE", 0); > +const Item_bool_static Item_true("TRUE", 1); The maain question, that Bar asked in his email - we have tons of modifiable Item members. How do you know none of them will modified in your Item_bool_static objects? > /** > Compare two Items for List<Item>::add_unique() > @@ -412,7 +413,6 @@ Item::Item(THD *thd): > is_expensive_cache(-1), rsize(0), name(null_clex_str), orig_name(0), > common_flags(IS_AUTO_GENERATED_NAME) > { > - DBUG_ASSERT(thd); > marker= 0; > maybe_null= null_value= with_window_func= with_field= false; > in_rollup= 0; > @@ -421,6 +421,9 @@ Item::Item(THD *thd): > /* Initially this item is not attached to any JOIN_TAB. */ > join_tab_idx= MAX_TABLES; > > + /* thd is NULL in case of static items like Item_true */ > + if (!thd) > + return; I don't like that. thd can be NULL only twice, for statically initialized Item_true and Item_false. For that you removed a useful assert and added an if() that is completely unnecessary for billions of items created runtime. Better to have special contructor just for these two very special items. Or to create a THD temporarily when constructing them. > /* Put item in free list so that we can free all items at end */ > next= thd->free_list; > thd->free_list= this; > @@ -3614,6 +3618,19 @@ void Item_int::print(String *str, enum_query_type query_type) > } > > > +/* > + This function is needed to ensure that Item_bool_static doesn't change > + the value of the member str_value. > +*/ > + > +void Item_bool::print(String *str, enum_query_type query_type) > +{ > + // my_charset_bin is good enough for numbers > + String tmp(value ? (char*) "1" : (char*) "0" , 1, &my_charset_bin); > + str->append(tmp); > +} This is not needed anymore. Item_bool inherits from Item_int, and Item_int::print is void Item_int::print(String *str, enum_query_type query_type) { StringBuffer<LONGLONG_BUFFER_SIZE> buf; // my_charset_bin is good enough for numbers buf.set_int(value, unsigned_flag, &my_charset_bin); str->append(buf); } You removed str_value from Item_int::print in July. > + > + > Item *Item_bool::neg_transformer(THD *thd) > { > value= !value; > diff --git a/sql/item.h b/sql/item.h > index c4fbf8f9c0a..4cdee637415 100644 > --- a/sql/item.h > +++ b/sql/item.h > @@ -1969,7 +1969,7 @@ class Item: public Value_source, > virtual bool limit_index_condition_pushdown_processor(void *arg) { return 0; } > virtual bool exists2in_processor(void *arg) { return 0; } > virtual bool find_selective_predicates_list_processor(void *arg) { return 0; } > - bool cleanup_is_expensive_cache_processor(void *arg) > + virtual bool cleanup_is_expensive_cache_processor(void *arg) You've added a new virtual method? You tried so hard to remove virtual methods that it's a bit strange to see a new virtual method added specifically for these two static items. Why does it matter what cleanup_is_expensive_cache_processor will do with them? You overwrite is_expensive() anyway. > { > is_expensive_cache= (int8)(-1); > return 0; > @@ -3225,6 +3225,8 @@ class Item_literal: public Item_basic_constant > bool check_partition_func_processor(void *int_arg) { return false;} > bool const_item() const { return true; } > bool basic_const_item() const { return true; } > + bool is_expensive() { return false; } > + bool cleanup_is_expensive_cache_processor(void *arg) { return 0; } > }; > > diff --git a/sql/opt_index_cond_pushdown.cc b/sql/opt_index_cond_pushdown.cc > index 360ae028f36..d66f2f2e14e 100644 > --- a/sql/opt_index_cond_pushdown.cc > +++ b/sql/opt_index_cond_pushdown.cc > @@ -185,8 +185,8 @@ bool uses_index_fields_only(Item *item, TABLE *tbl, uint keyno, > static Item *make_cond_for_index(THD *thd, Item *cond, TABLE *table, uint keyno, > bool other_tbls_ok) > { > - if (!cond) > - return NULL; > + if (!cond || cond->basic_const_item()) > + return cond; The effect of this change is that you don't set cond->marker= ICP_COND_USES_INDEX_ONLY for basic_const_item's anymore. This looked suspicious at first. But then I noticed (and Sergey Petrunia confirmed) that ICP_COND_USES_INDEX_ONLY is not used at all. Since 5.3. So here I'd suggest to revert this your change and completely remove any ICP_COND_USES_INDEX_ONLY manipulations together with the ICP_COND_USES_INDEX_ONLY definition. > if (cond->type() == Item::COND_ITEM) > { > uint n_marked= 0; > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc > index 144b86e8fc9..1140f32c9c9 100644 > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -2313,6 +2313,7 @@ bool dispatch_command(enum enum_server_command command, THD *thd, > thd->update_server_status(); > thd->protocol->end_statement(); > query_cache_end_of_result(thd); > + /* Check that the query didn't change const items */ what does that mean? Is it a TODO comment? Or it documents what happens below? > } > if (drop_more_results) > thd->server_status&= ~SERVER_MORE_RESULTS_EXISTS; > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index 22bf2eb37b5..fdd609a96ce 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -22560,6 +22560,8 @@ make_cond_for_table_from_pred(THD *thd, Item *root_cond, Item *cond, > return new_cond; > } > } > + else if (cond->basic_const_item()) > + return cond; this is rather fragile, isn't it? You skip item->set_join_tab_idx(join_tab_idx_arg), quite understandably. And currently, as far as I can see, item->get_join_tab_idx() is always used under if (!item->const_item()), so what you did is safe. But can it change in the future? Or is join_tab_idx something that conceptually doesn't make sense for const items? > > if (is_top_and_level && used_table == rand_table_bit && > (cond->used_tables() & ~OUTER_REF_TABLE_BIT) != rand_table_bit) Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

[Maria-developers] 5d9386892b5: MDEV-4633: multi_source.simple test fails sporadically
by sujatha 08 Sep '20

08 Sep '20

revision-id: 5d9386892b58477c4e28de3e40d930b44f5b42d4 (mariadb-10.1.43-274-g5d9386892b5) parent(s): 3ee2422624ffb3d7ffefff8db7ef9398816299bc author: Sujatha committer: Sujatha timestamp: 2020-09-08 13:10:36 +0530 message: MDEV-4633: multi_source.simple test fails sporadically Analysis: ======== Writes to 'rli->log_space_total' needs to be synchronized, otherwise both SQL_THREAD and IO_THREAD can try to modify the variable simultaneously resulting in incorrect rli->log_space_total. In the current test scenario SQL_THREAD is trying to decrement 'rli->log_space_total' in 'purge_first_log' and IO_THREAD is trying to increment the 'rli->log_space_total' in 'queue_event' simultaneously. Hence test occasionally fails with result mismatch. Fix: === Convert 'rli->log_space_total' variable to atomic type. --- sql/log.cc | 4 +++- sql/log.h | 4 +++- sql/rpl_rli.cc | 14 ++++++++------ sql/slave.cc | 19 +++++++++++++++---- 4 files changed, 29 insertions(+), 12 deletions(-) diff --git a/sql/log.cc b/sql/log.cc index 8049b94bab1..90d5c46a15f 100644 --- a/sql/log.cc +++ b/sql/log.cc @@ -4400,7 +4400,9 @@ int MYSQL_BIN_LOG::purge_first_log(Relay_log_info* rli, bool included) 0, 0, &log_space_reclaimed); mysql_mutex_lock(&rli->log_space_lock); - rli->log_space_total-= log_space_reclaimed; + my_atomic_add64_explicit((volatile int64*)(&rli->log_space_total), + (-(ulonglong)log_space_reclaimed), + MY_MEMORY_ORDER_RELAXED); mysql_cond_broadcast(&rli->log_space_cond); mysql_mutex_unlock(&rli->log_space_lock); diff --git a/sql/log.h b/sql/log.h index 277e5c6f69c..9e3807e9939 100644 --- a/sql/log.h +++ b/sql/log.h @@ -712,7 +712,9 @@ class MYSQL_BIN_LOG: public TC_LOG, private MYSQL_LOG char buf1[22],buf2[22]; #endif DBUG_ENTER("harvest_bytes_written"); - (*counter)+=bytes_written; + + my_atomic_add64_explicit((volatile int64*)(counter), bytes_written, + MY_MEMORY_ORDER_RELAXED); DBUG_PRINT("info",("counter: %s bytes_written: %s", llstr(*counter,buf1), llstr(bytes_written,buf2))); bytes_written=0; diff --git a/sql/rpl_rli.cc b/sql/rpl_rli.cc index c196a65809a..8815af4ea4a 100644 --- a/sql/rpl_rli.cc +++ b/sql/rpl_rli.cc @@ -140,8 +140,8 @@ int init_relay_log_info(Relay_log_info* rli, rli->slave_skip_counter=0; rli->abort_pos_wait=0; rli->log_space_limit= relay_log_space_limit; - rli->log_space_total= 0; - + my_atomic_store64_explicit((volatile int64*)(&rli->log_space_total), 0, + MY_MEMORY_ORDER_RELAXED); char pattern[FN_REFLEN]; (void) my_realpath(pattern, slave_load_tmpdir, 0); if (fn_format(pattern, PREFIX_SQL_LOAD, pattern, "", @@ -467,7 +467,8 @@ static inline int add_relay_log(Relay_log_info* rli,LOG_INFO* linfo) linfo->log_file_name); DBUG_RETURN(1); } - rli->log_space_total += s.st_size; + my_atomic_add64_explicit((volatile int64*)(&rli->log_space_total), + s.st_size, MY_MEMORY_ORDER_RELAXED); DBUG_PRINT("info",("log_space_total: %llu", rli->log_space_total)); DBUG_RETURN(0); } @@ -477,7 +478,8 @@ static int count_relay_log_space(Relay_log_info* rli) { LOG_INFO linfo; DBUG_ENTER("count_relay_log_space"); - rli->log_space_total= 0; + my_atomic_store64_explicit((volatile int64*)(&rli->log_space_total), 0, + MY_MEMORY_ORDER_RELAXED); if (rli->relay_log.find_log_pos(&linfo, NullS, 1)) { sql_print_error("Could not find first log while counting relay log space"); @@ -1202,8 +1204,8 @@ int purge_relay_logs(Relay_log_info* rli, THD *thd, bool just_reset, strmake_buf(rli->group_relay_log_name, rli->relay_log.get_log_fname()); strmake_buf(rli->event_relay_log_name, rli->relay_log.get_log_fname()); rli->group_relay_log_pos= rli->event_relay_log_pos= BIN_LOG_HEADER_SIZE; - rli->log_space_total= 0; - + my_atomic_store64_explicit((volatile int64*)(&rli->log_space_total), 0, + MY_MEMORY_ORDER_RELAXED); if (count_relay_log_space(rli)) { *errmsg= "Error counting relay log space"; diff --git a/sql/slave.cc b/sql/slave.cc index 06f2b0d955a..9f4b0f53fb4 100644 --- a/sql/slave.cc +++ b/sql/slave.cc @@ -2371,7 +2371,10 @@ static bool wait_for_relay_log_space(Relay_log_info* rli) &rli->log_space_lock, &stage_waiting_for_relay_log_space, &old_stage); - while (rli->log_space_limit < rli->log_space_total && + while (rli->log_space_limit < + (ulonglong)my_atomic_load64_explicit((volatile int64*) + (&rli->log_space_total), + MY_MEMORY_ORDER_RELAXED) && !(slave_killed=io_slave_killed(mi)) && !rli->ignore_log_space_limit) mysql_cond_wait(&rli->log_space_cond, &rli->log_space_lock); @@ -2912,7 +2915,10 @@ static bool send_show_master_info_data(THD *thd, Master_info *mi, bool full, protocol->store(mi->rli.last_error().message, &my_charset_bin); protocol->store((uint32) mi->rli.slave_skip_counter); protocol->store((ulonglong) mi->rli.group_master_log_pos); - protocol->store((ulonglong) mi->rli.log_space_total); + protocol->store((ulonglong) + my_atomic_load64_explicit((volatile int64*) + (&mi->rli.log_space_total), + MY_MEMORY_ORDER_RELAXED)); protocol->store( mi->rli.until_condition==Relay_log_info::UNTIL_NONE ? "None": @@ -4428,7 +4434,9 @@ Stopping slave I/O thread due to out-of-memory error from master"); #endif if (rli->log_space_limit && rli->log_space_limit < - rli->log_space_total && + (ulonglong) my_atomic_load64_explicit((volatile int64*) + (&rli->log_space_total), + MY_MEMORY_ORDER_RELAXED) && !rli->ignore_log_space_limit) if (wait_for_relay_log_space(rli)) { @@ -7031,7 +7039,10 @@ static Log_event* next_event(rpl_group_info *rgi, ulonglong *event_size) is are able to rotate and purge sometime soon. */ if (rli->log_space_limit && - rli->log_space_limit < rli->log_space_total) + rli->log_space_limit < + (ulonglong) my_atomic_load64_explicit((volatile int64*) + (&rli->log_space_total), + MY_MEMORY_ORDER_RELAXED)) { /* force rotation if not in an unfinished group */ rli->sql_force_rotate_relay= !rli->is_in_group();

1 0