- developers - lists.mariadb.org

[Maria-developers] (no subject)
by Игорь Балашенко 13 Jun '20

13 Jun '20

Hello mariaDB developers! I am a participant of GSOC 2020 mentored by Nikita Malyavin and Sergey Golubchik. Currently I am working on mysqltest parser improvements and I have a question to discuss with everyone: To be short: Currently there are expressions in if and while to be calculated i.e if(<expr>) code But it is written using switch and works only in condition block. Code like: let $a= 1 + 2 Makes $a = "1 + 2" Obviously mysqltest syntax needs proper expressions thats why I decided to make them. There are following questions: What syntax do you want to see? let $a = 1 + 2 $a = 3 Is a bad idea because we will break lots of previous taest cases. let $a = $((1 + 2)) That is done in a bash style, so it seems to be more comfortable to everyone. Another question is what operators should be in expressions? To begin with I want to implement: +, -. *, /, ++, --, <, >, ==, != To implement expressions I want to use Byson to make it easier to rewrite parser if needed I would be glad to read feedback and critique. ------------------------------ GSOC2020 participant Balashenko Igor

3 2

Re: [Maria-developers] 2849076672d: MDEV-16470: switch off user variables (and fixes of its support)
by Sergei Golubchik 12 Jun '20

12 Jun '20

Hi, Oleksandr! On Jun 12, Oleksandr Byelkin wrote: > revision-id: 2849076672d (mariadb-10.5.2-322-g2849076672d) > parent(s): bb47050e1fd > author: Oleksandr Byelkin <sanja(a)mariadb.com> > committer: Oleksandr Byelkin <sanja(a)mariadb.com> > timestamp: 2020-06-12 08:32:56 +0200 > message: > > MDEV-16470: switch off user variables (and fixes of its support) > > diff --git a/client/mysqltest.cc b/client/mysqltest.cc > index 55c4a66a89d..24e0cd25108 100644 > --- a/client/mysqltest.cc > +++ b/client/mysqltest.cc > &data, &data_length)) > { > dynstr_append(ds, "-- "); > - switch (type) > + if (type <= SESSION_TRACK_END) > { > - case SESSION_TRACK_SYSTEM_VARIABLES: > - dynstr_append(ds, "Tracker : SESSION_TRACK_SYSTEM_VARIABLES\n"); > - break; > - case SESSION_TRACK_SCHEMA: > - dynstr_append(ds, "Tracker : SESSION_TRACK_SCHEMA\n"); > - break; > - case SESSION_TRACK_STATE_CHANGE: > - dynstr_append(ds, "Tracker : SESSION_TRACK_STATE_CHANGE\n"); > - break; > - case SESSION_TRACK_GTIDS: > - dynstr_append(ds, "Tracker : SESSION_TRACK_GTIDS\n"); > - break; > - case SESSION_TRACK_TRANSACTION_CHARACTERISTICS: > - dynstr_append(ds, "Tracker : SESSION_TRACK_TRANSACTION_CHARACTERISTICS\n"); > - break; > - case SESSION_TRACK_TRANSACTION_TYPE: > - dynstr_append(ds, "Tracker : SESSION_TRACK_TRANSACTION_TYPE\n"); > - break; > - default: > - DBUG_ASSERT(0); > - dynstr_append(ds, "\n"); > + dynstr_append(ds, trking_info_desc[type]); > + } > + else > + { > + DBUG_ASSERT(0); > + dynstr_append(ds, "Trecker???\n"); "Tracker" > } > - > > dynstr_append(ds, "-- "); > dynstr_append_mem(ds, data, data_length); > diff --git a/libmariadb b/libmariadb > index ce74fd0c400..db7adf63fbf 160000 > --- a/libmariadb > +++ b/libmariadb > @@ -1 +1 @@ > -Subproject commit ce74fd0c4009ed9f4bcbdb4a01e96c823e961dc3 > +Subproject commit db7adf63fbf242a07c355f192a4d8c60323b5d89 No, I don't see any need for that now. C/C does not know anything about SESSION_TRACK_USER_VARIABLES, let's just keep it that way. > diff --git a/sql/item_func.cc b/sql/item_func.cc > index da185f9878b..448bfd380a2 100644 > --- a/sql/item_func.cc > +++ b/sql/item_func.cc > @@ -4702,10 +4702,12 @@ update_hash(user_var_entry *entry, bool set_null, void *ptr, size_t length, > entry->unsigned_flag= unsigned_arg; > } > entry->type=type; > +#ifdef USER_VAR_TACKING TRACKING > #ifndef EMBEDDED_LIBRARY > THD *thd= current_thd; > thd->session_tracker.user_variables.mark_as_changed(thd, entry); > #endif > +#endif // USER_VAR_TACKING > return 0; > } > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] [Commits] 4f528bb514c: MDEV-22011: DISTINCT with JSON_ARRAYAGG gives wrong results
by Sergey Petrunia 12 Jun '20

12 Jun '20

Hi Varun, I only have input about comments/variable names. Please find it below. On Tue, Mar 24, 2020 at 10:14:38AM +0530, Varun wrote: > revision-id: 4f528bb514c607d848b30119cea135e37e0d9c69 (mariadb-10.5.0-425-g4f528bb514c) > parent(s): 1cd8e8213b6019794ca7f01e8276cfd1ba6408cf > author: Varun Gupta > committer: Varun Gupta > timestamp: 2020-03-24 10:13:28 +0530 > message: > > MDEV-22011: DISTINCT with JSON_ARRAYAGG gives wrong results > > For JSON_ARRAYAGG with DISTINCT also store the null bytes for the record > inside the Unique object > > --- > mysql-test/main/func_json.result | 4 +- > sql/item_jsonfunc.cc | 36 ++++++++++++-- > sql/item_jsonfunc.h | 9 ++-- > sql/item_sum.cc | 101 ++++++++++++++++++++++++++++++++++----- > sql/item_sum.h | 13 +++-- > 5 files changed, 133 insertions(+), 30 deletions(-) > > diff --git a/sql/item_jsonfunc.cc b/sql/item_jsonfunc.cc > index a3b5d3b7fac..ed496df8169 100644 > --- a/sql/item_jsonfunc.cc > +++ b/sql/item_jsonfunc.cc > @@ -1452,6 +1452,31 @@ static int append_json_value(String *str, Item *item, String *tmp_val) > } > > > +bool append_json_value(String *str, String *res, Item *item, bool null_value) There is another append_json_value right above this one. That function has no comments but let's not follow that bad pattern. - Please add a function comment describing this function: @brief should describe this function @detail should describe the difference from the one above. It is very counter-intuitive that the parameter named "res" does not hold the result of this function. Please rename it (to value?) (can it be changed to be const String* ?) > +{ > + if (null_value) > + return str->append("null", 4); > + > + if (item->type_handler()->is_bool_type()) > + { > + return ((*res)[0] == '1') ? > + str->append("true", 4) : > + str->append("false", 5); > + } > + > + if (item->is_json_type()) > + return str->append(res->ptr(), res->length()); > + > + if (item->result_type() == STRING_RESULT) > + { > + return str->append("\"", 1) || > + st_append_escaped(str, res) || > + str->append("\"", 1); > + } > + return st_append_escaped(str, res); This made me wonder why something is escaped if it is not enclosed in quotes, but now I see this is just copied from above. > +} > + > + > static int append_json_keyname(String *str, Item *item, String *tmp_val) > { > String *sv= item->val_str(tmp_val); > diff --git a/sql/item_sum.cc b/sql/item_sum.cc > index 5c732095573..a04dbdc19e2 100644 > --- a/sql/item_sum.cc > +++ b/sql/item_sum.cc > @@ -3552,6 +3552,54 @@ int group_concat_key_cmp_with_distinct(void* arg, const void* key1, > } > > > +int group_concat_key_cmp_with_distinct_with_nulls(void* arg, > + const void* key1_arg, > + const void* key2_arg) > +{ > + Item_func_group_concat *item_func= (Item_func_group_concat*)arg; > + > + uchar *key1= (uchar*)key1_arg + item_func->table->s->null_bytes; > + uchar *key2= (uchar*)key2_arg + item_func->table->s->null_bytes; > + > + for (uint i= 0; i < item_func->arg_count_field; i++) This loop is actually not needed, right? JSON_ARRAYAGG only accepts one argument. Please add a note. > + { > + Item *item= item_func->args[i]; > + /* > + If item is a const item then either get_tmp_table_field returns 0 > + or it is an item over a const table. > + */ > + if (item->const_item()) > + continue; > + /* > + We have to use get_tmp_table_field() instead of > + real_item()->get_tmp_table_field() because we want the field in > + the temporary table, not the original field > + */ > + Field *field= item->get_tmp_table_field(); > + > + if (!field) > + continue; > + > + if (field->is_null_in_record((uchar*)key1_arg) && > + field->is_null_in_record((uchar*)key2_arg)) > + continue; > + > + if (field->is_null_in_record((uchar*)key1_arg)) > + return -1; > + > + if (field->is_null_in_record((uchar*)key2_arg)) > + return 1; > + > + uint offset= (field->offset(field->table->record[0]) - > + field->table->s->null_bytes); > + int res= field->cmp(key1 + offset, key2 + offset); > + if (res) > + return res; > + } > + return 0; > +} > + > + > /** > function of sort for syntax: GROUP_CONCAT(expr,... ORDER BY col,... ) > */ > @@ -3650,9 +3699,11 @@ int dump_leaf_key(void* key_arg, element_count count __attribute__((unused)), > result->append(*item->separator); > > > + bool is_null= false; > for (; arg < arg_end; arg++) > { > String *res; > + is_null= false; (1) > /* > We have to use get_tmp_table_field() instead of > real_item()->get_tmp_table_field() because we want the field in > @@ -3661,7 +3712,11 @@ int dump_leaf_key(void* key_arg, element_count count __attribute__((unused)), > because it contains both order and arg list fields. > */ > if ((*arg)->const_item()) > + { > res= (*arg)->val_str(&tmp); > + if ((*arg)->null_value) > + is_null= TRUE; (2) Please either use true/false (preferred) or TRUE / FALSE. > + } > else > { > Field *field= (*arg)->get_tmp_table_field(); ... > diff --git a/sql/item_sum.h b/sql/item_sum.h > index b10d9f5974d..908dbe3cce9 100644 > --- a/sql/item_sum.h > +++ b/sql/item_sum.h > @@ -1897,14 +1899,19 @@ class Item_func_group_concat : public Item_sum > */ > Item_func_group_concat *original; > > + bool exclude_nulls; > + > /* > Used by Item_func_group_concat and Item_func_json_arrayagg. The latter > needs null values but the former doesn't. > */ Is this comment relevant here? I think it's better to remove this and instead add a comment for the exclude_nulls above. > - bool add(bool exclude_nulls); > + bool add(); > > friend int group_concat_key_cmp_with_distinct(void* arg, const void* key1, > const void* key2); > + friend int group_concat_key_cmp_with_distinct_with_nulls(void* arg, > + const void* key1, > + const void* key2); > friend int group_concat_key_cmp_with_order(void* arg, const void* key1, > const void* key2); > friend int dump_leaf_key(void* key_arg, BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

[Maria-developers] Improving `source` in mysqltest
by Nikita Malyavin 11 Jun '20

11 Jun '20

Hello mariadb developers! This year we have a student on mysqltest parser improvements, mentored by me and @serg. The major improvement will be proper expressions support, which you'll soon receive an email about. Another a bit more minor-ish improvement we want our student Igor to make is parameters support for source command, which I want to ask you about, which way is better to implement. It's quite spread in the tests to parametrize source with variables, for example > --let SEARCH_FILE=$MYSQLTEST_VARDIR/log/mysqld.1.err > --let SEARCH_PATTERN= This connection closed normally without > authentication > --source include/search_pattern_in_file.inc Another example is period/create_triggers.inc. So, we want make it possible to write > --source do_something.inc A=1 B=foobar And the question is, how should it behave. One way is to make it a strict equivalent of > let A=1; > let B=foobar; > source do_something.inc; This allows to reuse previously set values, for example: > --source do_something.inc A=1 B=foobar > ... > --source do_something.inc A=2 Which literally means > let A=1; > let B=foobar; > --source do_something.inc; ... let A=2; > --source do_something.inc; Another way is to make it work like local arguments, so the arguments are put in the stack, and as a result > let A=qwe; > --source do_something.inc A=1 B=foobar > --echo $A; outputs qwe. What do you think of this, what's more suitable for us? This is an important question, since all of us are using mysqltest, and we probably wouldn't change the behavior ever later. -- Yours truly, Nikita Malyavin

1 0

Re: [Maria-developers] MDEV-21831: Assertion `length == pack_length()' failed in Field_inet6::sort_string upon INSERT into RocksDB table
by Sergey Petrunia 11 Jun '20

11 Jun '20

On Thu, Jun 11, 2020 at 03:41:24PM +0530, Varun Gupta wrote: > > /* > > pack_length() returns size (in bytes) used to store field data in > > memory > > (i.e. it returns the maximum size of the field in a row of the table, > > which is located in RAM). > > */ > > virtual uint32 pack_length() const { return (uint32) field_length; } > > > > so, it is size in bytes, while strnxfrmlen() accepts length *in > > characters*. > > That is, strnxfrmlen(field->pack_length()) is certainly not correct. > > > > > This is not correct, strnxfrmlen accepts length in bytes and there we > change the length to characters and then calculate the weights. You're right. I've missed this part. The patch is ok to push, then. BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

[Maria-developers] Progress report week 5
by Mohammed Hammaad Mateen 11 Jun '20

11 Jun '20

Greetings, Hope you are safe and doing great, This post describes the things I've done during 1-7 June under the mentor-ship of Sergei Golubchik and Oleksandr Byelkin for GSoC-20. The tasks taken up for this week Greetings mentors, I've realized that to extend the parser for multi delete, I should be changing the sql_yacc.yy file, for incorporating opt_returning rule in multi-delete. opt_returning rule works completely fine with single table delete, but to tweak it to adapt for multi-delete, required me to learn some bison grammar syntax and semantics and that took me a couple of days. Moreover everytime I made changes to the grammar, the server used to crash rendering all changes invalid. I'll try to push the code that was triggering crashes, shortly. Thank you Regards, Mohammed Hammaad Mateen

1 0

Re: [Maria-developers] 5ff9525ff86: MDEV-22722 Assertion "inited==NONE" failed in handler::ha_index_init on the slave during UPDATE
by Sergei Golubchik 10 Jun '20

10 Jun '20

Hi, Sachin! On Jun 10, Sachin wrote: > revision-id: 5ff9525ff86 (mariadb-10.5.2-290-g5ff9525ff86) > parent(s): ff881056b8d > author: Sachin <sachin.setiya(a)mariadb.com> > committer: Sachin <sachin.setiya(a)mariadb.com> > timestamp: 2020-06-07 16:45:01 +0530 > message: > > diff --git a/sql/log_event_server.cc b/sql/log_event_server.cc > index 5910ece12f0..ffda352c866 100644 > --- a/sql/log_event_server.cc > +++ b/sql/log_event_server.cc > @@ -5570,6 +5570,12 @@ int Rows_log_event::do_apply_event(rpl_group_info *rgi) > { > master_had_triggers= table->master_had_triggers; > bool transactional_table= table->file->has_transactions_and_rollback(); > + if (get_general_type_code() == WRITE_ROWS_EVENT) > + table->file->prepare_for_insert(0); > + else if (get_general_type_code() == UPDATE_ROWS_EVENT || > + get_general_type_code() == DELETE_ROWS_EVENT) > + table->file->prepare_for_insert(1); > + You can simplify it as table->file->prepare_for_insert(get_general_type_code() != WRITE_ROWS_EVENT); otherwise ok to push Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] MDEV-21831: Assertion `length == pack_length()' failed in Field_inet6::sort_string upon INSERT into RocksDB table
by Sergey Petrunia 09 Jun '20

09 Jun '20

Hi Varun, > MDEV-21831: Assertion `length == pack_length()' failed in Field_inet6::sort_string upon INSERT into RocksDB table > > For INET6 columns the values are stored as BINARY columns and returned to the client in TEXT format. > For rocksdb the indexes store mem-comparable images for columns, so use the pack_length() to store > the mem-comparable form for INET6 columns. This would also remain consistent with CHAR columns. > > diff --git a/storage/rocksdb/rdb_datadic.cc b/storage/rocksdb/rdb_datadic.cc > index 21a9b57b555e..6e73535d1de7 100644 > --- a/storage/rocksdb/rdb_datadic.cc > +++ b/storage/rocksdb/rdb_datadic.cc > @@ -3317,7 +3317,9 @@ bool Rdb_field_packing::setup(const Rdb_key_def *const key_descr, > field->field_length = field->char_length() * cs->mbmaxlen. > */ > const CHARSET_INFO *cs = field->charset(); > - m_max_image_len = cs->strnxfrmlen(field->field_length); > + m_max_image_len = cs->strnxfrmlen(type == MYSQL_TYPE_STRING ? > + field->pack_length() : > + field->field_length); Field::pack_length is defined as: /* pack_length() returns size (in bytes) used to store field data in memory (i.e. it returns the maximum size of the field in a row of the table, which is located in RAM). */ virtual uint32 pack_length() const { return (uint32) field_length; } so, it is size in bytes, while strnxfrmlen() accepts length *in characters*. That is, strnxfrmlen(field->pack_length()) is certainly not correct. Rdb_field_packing::m_max_image_len is defined as: /* Length of mem-comparable image of the field, in bytes */ int m_max_image_len; while pack_length() seems to describe table->record[0] format (or is it KeyTupleFormat) ? Field::sort_length() doesn't have a definition but it seems to be a closer match as it describes the length of the mem-comparable form? BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] 06942363ebc: MDEV-14347 CREATE PROCEDURE returns no error when using an unknown variable
by Sergei Golubchik 09 Jun '20

09 Jun '20

Hi, Alexander! On Jun 07, Alexander Barkov wrote: > revision-id: 06942363ebc (mariadb-10.5.2-311-g06942363ebc) > parent(s): 0d6d63e1505 > author: Alexander Barkov <bar(a)mariadb.com> > committer: Alexander Barkov <bar(a)mariadb.com> > timestamp: 2020-06-03 12:11:07 +0400 > message: > > MDEV-14347 CREATE PROCEDURE returns no error when using an unknown variable > > diff --git a/sql/item.cc b/sql/item.cc > index 8ea6366e6c4..5800b9bc95e 100644 > --- a/sql/item.cc > +++ b/sql/item.cc > @@ -764,6 +764,29 @@ bool Item_field::collect_item_field_processor(void *arg) > } > > > +void Item_ident::undeclared_spvar_error() const Currently it's used only in Item_field::unknown_splocal_processor(). Do you plan to use this somewhere else? Why is it a separate method and in Item_ident? > +{ > + /* > + We assume this is an unknown SP variable, possibly a ROW variable. > + Print the leftmost name in the error: > + SET var=a; -> a > + SET var=a.b; -> a > + SET var=a.b.c; -> a > + */ > + my_error(ER_SP_UNDECLARED_VAR, MYF(0), db_name.str ? db_name.str : > + table_name.str ? table_name.str : > + field_name.str); > +} > + > +bool Item_field::unknown_splocal_processor(void *arg) > +{ > + DBUG_ENTER("Item_field::unknown_splocal_processor"); > + DBUG_ASSERT(type() == FIELD_ITEM); > + undeclared_spvar_error(); > + DBUG_RETURN(true); > +} > + > + > bool Item_field::add_field_to_set_processor(void *arg) > { > DBUG_ENTER("Item_field::add_field_to_set_processor"); > @@ -8012,6 +8035,27 @@ void Item_ref::cleanup() > } > > > +bool Item_ref::unknown_splocal_processor(void *arg) > +{ > + DBUG_ENTER("Item_ref::unknown_splocal_processor"); > + DBUG_ASSERT(type() == REF_ITEM); > + DBUG_ASSERT(ref_type() == REF); > + undeclared_spvar_error(); > + DBUG_RETURN(true); > +} > + > + > +bool Item_ref::walk(Item_processor processor, bool walk_subquery, void *arg) > +{ > + if (processor == &Item::unknown_splocal_processor) > + return unknown_splocal_processor(arg); Why is that? This is very unusual, normally walk() should not change its behavior based on the processor. > + if (ref && *ref) > + return (*ref)->walk(processor, walk_subquery, arg) || > + (this->*processor)(arg); > + return false; > +} > + > + > /** > Transform an Item_ref object with a transformer callback function. > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 2

Re: [Maria-developers] JSON aggregate functions working on JSON documents
by Sergey Petrunia 09 Jun '20

09 Jun '20

On Mon, Jun 08, 2020 at 04:05:48PM +0400, Alexey Botchkov wrote: > So the problem is that we (MariaDB) doesn't consider the JSON field content > as a JSON, but as a plain text. > That affects not only aggregate functions, but any other expecting JSON > arguments. > To me it's the bug, should be fixed. Will you report it, Sergei? Filed: https://jira.mariadb.org/browse/MDEV-22837 BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0