- developers - lists.mariadb.org

Re: [Maria-developers] MDEV-10000 Add EXPLAIN FOR CONNECTION syntax support
by Sergey Petrunia 29 Jan '22

29 Jan '22

Hi Oleg, > commit 53d4d2852463714678b6e753ecffc1ecdde13891 > Author: Oleg Smirnov <olernov(a)gmail.com> > Date: Thu Jan 13 19:24:22 2022 +0300 > > MDEV-10000 Add support for FORMAT=JSON > > Makes possible calls to SHOW EXPLAIN and EXPLAIN FOR CONNECTION > providing output in JSON format: > - SHOW EXPLAIN FORMAT=JSON FOR $con > - EXPLAIN FORMAT=JSON FOR CONNECTION $con > The patch is mostly ok. I have a few cosmetic comments. Please follow the coding style and make sure the lines are no longer than 80 chars. > diff --git a/sql/sql_show.h b/sql/sql_show.h > index 3d7a4d1146c..8c8ae416699 100644 > --- a/sql/sql_show.h > +++ b/sql/sql_show.h > @@ -164,6 +164,9 @@ class Show_explain_request : public Apc_target::Apc_call > THD *target_thd; /* thd that we're running SHOW EXPLAIN for */ > THD *request_thd; /* thd that run SHOW EXPLAIN command */ > > + bool is_json_format= false; /* set to TRUE if you need the result in JSON > + format, FALSE - in traditional tabular */ > + Multi-line comments must precede the variable: /* Comment here... */ bool is_json_format; > /* If true, there was some error when producing EXPLAIN output. */ > bool failed_to_produce; > > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index 74757970411..a7188762cbd 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -13945,12 +13945,13 @@ show_param: > Lex->spname= $3; > Lex->sql_command = SQLCOM_SHOW_CREATE_EVENT; > } > - | describe_command FOR_SYM expr > + | describe_command opt_format_json FOR_SYM expr > { > Lex->sql_command= SQLCOM_SHOW_EXPLAIN; > - if (unlikely(prepare_schema_table(thd, Lex, 0, SCH_EXPLAIN))) > + if (unlikely(prepare_schema_table(thd, Lex, 0, > + Lex->explain_json ? SCH_EXPLAIN_JSON : SCH_EXPLAIN_TABULAR))) > MYSQL_YYABORT; > - add_value_to_list(thd, $3); > + add_value_to_list(thd, $4); > } > | IDENT_sys remember_tok_start wild_and_where > { > @@ -14123,12 +14124,18 @@ opt_describe_column: > ; Please fix identation of this rule to match of identation of others. Please also add a comment about the alternative syntax, both here and in the SHOW EXPLAIN rule. > explain_for_connection: > - describe_command FOR_SYM CONNECTION_SYM expr > + describe_command opt_format_json FOR_SYM CONNECTION_SYM expr > { > + Lex->wild=0; > + Lex->ident= null_clex_str; > + mysql_init_select(Lex); > + Lex->current_select->parsing_place= SELECT_LIST; > + Lex->create_info.init(); > Lex->sql_command= SQLCOM_SHOW_EXPLAIN; > - if (unlikely(prepare_schema_table(thd, Lex, 0, SCH_EXPLAIN))) > + if (unlikely(prepare_schema_table(thd, Lex, 0, > + Lex->explain_json ? SCH_EXPLAIN_JSON : SCH_EXPLAIN_TABULAR))) > MYSQL_YYABORT; > - add_value_to_list(thd, $4); > + add_value_to_list(thd, $5); > } > ; BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

1 0

Re: [Maria-developers] MDEV-27021 Implement SHOW ANALYZE command
by Sergey Petrunia 28 Jan '22

28 Jan '22

Hi Oleg, > commit 92b85f455994ff9f18e30ad4b852109b3e3c5f04 > Author: Oleg Smirnov <olegs(a)teramind.co> > Date: Fri Dec 24 17:27:03 2021 +0300 > > MDEV-27021 Implement SHOW ANALYZE command > The patch looks good. A few cosmetic comments below. I've played with it and it looks usable, although I would expect that SHOW ANALYZE FORMAT=JSON would be the real killer. For SHOW ANALYZE FORMAT=JSON, I suspect we might need to add a few fields into it to make analysis easier (I'll elaborate later). Let's first get SHOW ANALYZE FORMAT=JSON to work without the extra columns. == Cosmetic comments == Please make sure there are no spaces at line ends. Some MariaDB developers feel strongly about not having them. (If your edior doesn't highlight them, "git citool" does). At risk of stating the obvious: * one should not remove the endspace if otherwise they are not changing the line of code. (such "dummy" changes make it hard to look at the revision history) * If one adds a line or changes an existing line, make sure the line doesnt have the end spaces. > diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt > index f31f21faeb0..e18f8c61067 100644 > --- a/sql/share/errmsg-utf8.txt > +++ b/sql/share/errmsg-utf8.txt > @@ -7224,7 +7224,7 @@ ER_NO_SUCH_TABLE_IN_ENGINE 42S02 > hindi "टेबल '%-.192s.%-.192s' इंजन में मौजूद नहीं है" > swe "Det finns ingen tabell som heter '%-.192s.%-.192s' i handlern" > ER_TARGET_NOT_EXPLAINABLE > - eng "Target is not running an EXPLAINable command" > + eng "Target is not running a command that allows to produce SHOW EXPLAIN or ANALYZE" . This will soon mean "SHOW EXPLAIN or ANALYZE or EXPLAIN FOR CONNECTION". Does "Target is not executing an operation with a query plan" sound better? > ER_CONNECTION_ALREADY_EXISTS > eng "Connection '%.*s' conflicts with existing connection '%.*s'" > ER_MASTER_LOG_PREFIX BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

2 1

[Maria-developers] Igor please "review" : 768b5f70b59: Fix Item_func::is_simplifed_cond_processor
by Sergey Petrunia 27 Jan '22

27 Jan '22

Hello Igor, Could you please checkout the below patch? I've stumbled upon behavior of Item_func::is_simplifed_cond_processor which looks bad but I'm not sure if this way to fix it is correct. revision-id: 9149fcb8d504d7c1b027024df63c8120a65a7751 (mariadb-10.4.22-82-g9149fcb8d50) parent(s): b915f79e4e004fde4f6ac8f341afee980e11792b author: Sergei Petrunia committer: Sergei Petrunia timestamp: 2022-01-27 22:58:11 +0300 message: Fix Item_func::is_simplifed_cond_processor The comment for that function says: TRUE if the function is knowingly TRUE or FALSE. I don't get why one would need to check for "!val_int()" in this case. --- mysql-test/main/having_cond_pushdown.result | 6 ------ sql/item_func.h | 2 +- 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/mysql-test/main/having_cond_pushdown.result b/mysql-test/main/having_cond_pushdown.result index 9b124296e3d..aade383a71f 100644 --- a/mysql-test/main/having_cond_pushdown.result +++ b/mysql-test/main/having_cond_pushdown.result @@ -3194,7 +3194,6 @@ EXPLAIN { "query_block": { "select_id": 1, - "const_condition": "1", "table": { "table_name": "t1", "access_type": "ALL", @@ -3536,7 +3535,6 @@ EXPLAIN { "query_block": { "select_id": 1, - "const_condition": "1 and 1", "table": { "table_name": "t1", "access_type": "ALL", @@ -3663,7 +3661,6 @@ EXPLAIN { "query_block": { "select_id": 1, - "const_condition": "1", "table": { "table_name": "t1", "access_type": "ALL", @@ -3791,7 +3788,6 @@ EXPLAIN { "query_block": { "select_id": 1, - "const_condition": "1", "filesort": { "sort_key": "t1.c", "temporary_table": { @@ -3859,7 +3855,6 @@ EXPLAIN { "query_block": { "select_id": 1, - "const_condition": "1 and 1", "table": { "table_name": "t1", "access_type": "ALL", @@ -3977,7 +3972,6 @@ EXPLAIN { "query_block": { "select_id": 1, - "const_condition": "1", "table": { "table_name": "t3", "access_type": "ALL", diff --git a/sql/item_func.h b/sql/item_func.h index b368e5872fe..2b02971d940 100644 --- a/sql/item_func.h +++ b/sql/item_func.h @@ -399,7 +399,7 @@ class Item_func :public Item_func_or_sum, With_sum_func_cache* get_with_sum_func_cache() { return this; } Item_func *get_item_func() { return this; } bool is_simplified_cond_processor(void *arg) - { return const_item() && !val_int(); } + { return const_item() /*&& !val_int();*/; } }; BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

1 0

Re: [Maria-developers] MDEV-10000 Add EXPLAIN FOR CONNECTION syntax support
by Sergey Petrunia 21 Jan '22

21 Jan '22

Hi Oleg, > commit 8e6595cff0caa22d2bb81002505e52b5c52f5ab2 > Author: Oleg Smirnov <olernov(a)gmail.com> > Date: Tue Jan 11 20:25:11 2022 +0300 > > MDEV-10000 Add EXPLAIN FOR CONNECTION syntax support ... > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index 442644eddd6..74757970411 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -14121,6 +14122,15 @@ opt_describe_column: > } > ; > > +explain_for_connection: > + describe_command FOR_SYM CONNECTION_SYM expr > + { > + Lex->sql_command= SQLCOM_SHOW_EXPLAIN; > + if (unlikely(prepare_schema_table(thd, Lex, 0, SCH_EXPLAIN))) > + MYSQL_YYABORT; > + add_value_to_list(thd, $4); > + } > + ; > This fails to intialize the statement properly. For SHOW EXPLAIN, the following is done at statement start: show: SHOW { LEX *lex=Lex; lex->wild=0; lex->ident= null_clex_str; if (Lex->main_select_push()) MYSQL_YYABORT; mysql_init_select(lex); lex->current_select->parsing_place= SELECT_LIST; lex->create_info.init(); } If you don't do it, EXPLAIN FOR CONNECTION may produce crashes (e.g. on my machine it was crash in JOIN::prepare due to select_lex->inner_sum_func_list=0xa5a5a5a5a5). The rest of the code looks ok. Let's have another review iteration once the aboveis fixed. I've also found one difference with MySQL, which I am not sure if we should fix: In MariaDB, if the target is not running something that has an query plan, SHOW EXPLAIN (and now EXPLAIN FOR CONNECTION) will produce this error: MariaDB [test]> show explain for 5; ERROR 1933 (HY000): Target is not running an EXPLAINable command while in MySQL it can be one of two: 1. Target is idle: mysql> explain for connection 8; Query OK, 0 rows affected (0.00 sec) 2. Target is running a command that's not supported: mysql> explain for connection 8; ERROR 3012 (HY000): EXPLAIN FOR CONNECTION command is supported only for SELECT/UPDATE/INSERT/DELETE/REPLACE I'm not sure we should try to imitate this behavior. BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

1 0

Re: [Maria-developers] 673ea509e2c: MDEV-26870 --skip-symbolic-links does not disallow .isl file creation
by Sergei Golubchik 21 Jan '22

21 Jan '22

Hi, Marko, On Jan 20, Marko Mäkelä wrote: > revision-id: 673ea509e2c (mariadb-10.2.40-230-g673ea509e2c) > parent(s): d28d3aee108 > author: Marko Mäkelä > committer: Marko Mäkelä > timestamp: 2022-01-20 16:13:08 +0200 > message: > > MDEV-26870 --skip-symbolic-links does not disallow .isl file creation > > The InnoDB DATA DIRECTORY attribute is not implemented via > symbolic links but something similar, *.isl files that contain > the names of data files. > > InnoDB failed to reject the DATA DIRECTORY attribute even though > the server was started with --skip-symbolic-links. > > Because TRUNCATE TABLE cannot easily return an error, it will keep > rebuilding tables even if they carry the DATA DIRECTORY attribute. > > diff --git a/mysql-test/suite/encryption/t/innodb-first-page-read.test b/mysql-test/suite/encryption/t/innodb-first-page-read.test > index d661e4565d2..a0b3ca3f0ff 100644 > --- a/mysql-test/suite/encryption/t/innodb-first-page-read.test > +++ b/mysql-test/suite/encryption/t/innodb-first-page-read.test > @@ -1,6 +1,7 @@ > -- source include/have_innodb.inc > -- source include/have_file_key_management_plugin.inc > -- source include/not_embedded.inc > +-- source include/have_symlink.inc when can have_symlink be false? Why InnoDB tests depend on server's symlink support? InnoDB does not use symlinks, it uses isl files. > --disable_warnings > SET GLOBAL innodb_file_format = `Barracuda`; > diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc > index 5d78e64a06b..a8b16ba1eb0 100644 > --- a/storage/innobase/handler/ha_innodb.cc > +++ b/storage/innobase/handler/ha_innodb.cc > @@ -11710,9 +11710,15 @@ create_table_info_t::create_options_are_invalid() > break; > } > > - if (m_create_info->data_file_name > - && m_create_info->data_file_name[0] != '\0' > - && !create_option_data_directory_is_valid()) { > + if (!m_create_info->data_file_name > + || !m_create_info->data_file_name[0]) { > + } else if (!my_use_symdir) { > + push_warning( > + m_thd, Sql_condition::WARN_LEVEL_WARN, > + ER_ILLEGAL_HA_CREATE_OPTION, > + "InnoDB: DATA DIRECTORY requires HAVE_SYMLINK."); > + ret = "DATA DIRECTORY"; MyISAM does push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN, WARN_OPTION_IGNORED, ER_THD(thd, WARN_OPTION_IGNORED), "DATA DIRECTORY"); Archive does if (create_info->data_file_name) my_error(WARN_OPTION_IGNORED, MYF(ME_WARNING), "DATA DIRECTORY"); I suggest you do one of the above too. > + } else if (!create_option_data_directory_is_valid()) { > ret = "DATA DIRECTORY"; > } > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3

Re: [Maria-developers] 99e2a49acfc: MDEV-27018 IF and COALESCE lose "json" property
by Sergei Golubchik 20 Jan '22

20 Jan '22

Hi, Alexander! On Jan 13, Alexander Barkov wrote: > revision-id: 99e2a49acfc (mariadb-10.5.13-33-g99e2a49acfc) > parent(s): 2776635cb98 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2022-01-10 18:05:55 +0400 > message: > > MDEV-27018 IF and COALESCE lose "json" property > > Hybrid functions (IF, COALESCE, etc) did not preserve the JSON property > from their arguments. The same problem was repeatable for single row subselects. > > The problem happened because the method Item::is_json_type() was inconsistently > implemented across the Item hierarchy. For example, Item_hybrid_func > and Item_singlerow_subselect did not override is_json_type(). > > Solution: > > - Removing Item::is_json_type() > > - Implementing specific JSON type handlers: > Type_handler_string_json > Type_handler_varchar_json > Type_handler_tiny_blob_json > Type_handler_blob_json > Type_handler_medium_blob_json > Type_handler_long_blob_json > > - Reusing the existing data type infrastructure to pass JSON > type handlers across all item types, including classes Item_hybrid_func > and Item_singlerow_subselect. Note, these two classes themselves do not > need any changes! > > - Extending the data type infrastructure so data types can inherit > their properties (e.g. aggregation rules) from their base data types. > E.g. VARCHAR/JSON acts as VARCHAR, LONGTEXT/JSON acts as LONGTEXT > when mixed to a non-JSON data type. This is done by: > - adding virtual method Type_handler::type_handler_base() > - adding class Recursive_type_pair_iterator > - refactoring Type_handler_hybrid_field_type methods > aggregate_for_result(), aggregate_for_min_max(), > aggregate_for_num_op() to use Recursive_type_pair_iterator. > > This change also fixes: > > MDEV-27361 Hybrid functions with JSON arguments do not send format metadata > > diff --git a/sql/field.cc b/sql/field.cc > index 2c768527ced..8fa3bbd538c 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -7277,6 +7277,19 @@ bool Field_longstr::send(Protocol *protocol) > } > > > +const Type_handler *Field_string::type_handler() const > +{ > + if (is_var_string()) > + return &type_handler_var_string; shouldn't it be after json check? > + /* > + This is a temporary solution and will be fixed soon (in 10.9?). > + Type_handler_string_json will provide its own Field_string_json. > + */ > + if (Type_handler_json_common::has_json_valid_constraint(this)) > + return &type_handler_string_json; > + return &type_handler_string; > +} > + > /* Copy a string and fill with space */ > > int Field_string::store(const char *from, size_t length,CHARSET_INFO *cs) > diff --git a/sql/item_jsonfunc.cc b/sql/item_jsonfunc.cc > index ddf5fc32ea4..81003be4656 100644 > --- a/sql/item_jsonfunc.cc > +++ b/sql/item_jsonfunc.cc > @@ -1441,6 +1441,32 @@ longlong Item_func_json_contains_path::val_int() > } > > > +/* > + This reproduces behavior according to the former > + Item_func_conv_charset::is_json_type() which returned args[0]->is_json_type(). > + JSON functions with multiple string input with different character sets > + wrap some arguments into Item_func_conv_charset. So the former > + Item_func_conv_charset::is_json_type() took the JSON propery from args[0], > + i.e. from the original argument before the conversion. > + This is probably not always correct because an *explicit* > + `CONVERT(arg USING charset)` is actually a general purpose string > + expression, not a JSON expression. > +*/ > +static bool is_json_type(const Item *item) > +{ > + for ( ; ; ) > + { > + if (Type_handler_json_common::is_json_type_handler(item->type_handler())) > + return true; > + const Item_func_conv_charset *func; > + if (!(func= dynamic_cast<const Item_func_conv_charset*>(item))) > + return false; > + item= func->arguments()[0]; can you have nested CONVERT()'s ? > + } > + return false; > +} > + > + > static int append_json_value(String *str, Item *item, String *tmp_val) > { > if (item->type_handler()->is_bool_type()) > diff --git a/sql/sql_type_json.h b/sql/sql_type_json.h > index 6c4ee8cb2eb..4a394809a06 100644 > --- a/sql/sql_type_json.h > +++ b/sql/sql_type_json.h > @@ -21,18 +21,145 @@ ... > +template <class BASE, const Named_type_handler<BASE> &thbase> > +class Type_handler_general_purpose_string_to_json: > + public BASE, > + public Type_handler_json_common > { ... > + bool Item_hybrid_func_fix_attributes(THD *thd, > + const char *name, > + Type_handler_hybrid_field_type *hybrid, > + Type_all_attributes *attr, > + Item **items, uint nitems) > + const override > + { > + if (BASE::Item_hybrid_func_fix_attributes(thd, name, hybrid, attr, > + items, nitems)) > + return true; > + /* > + The above call can change the type handler on "hybrid", e.g. > + choose a proper BLOB type handler according to the calculated max_length. > + Convert general purpose string type handler to its JSON counterpart. > + This makes hybrid functions preserve JSON data types, e.g.: > + COALESCE(json_expr1, json_expr2) -> JSON > + */ > + hybrid->set_handler(json_type_handler_from_generic(hybrid->type_handler())); When this line would change hybrid->type_handler() ? > + return false; > + } > }; ..... > diff --git a/sql/sql_type.cc b/sql/sql_type.cc > index c1801c1ae3e..3b2753d80a6 100644 > --- a/sql/sql_type.cc > +++ b/sql/sql_type.cc > @@ -1755,19 +1755,110 @@ const Type_handler *Type_handler_typelib::cast_to_int_type_handler() const > > /***************************************************************************/ > > +class Recursive_type_pair_iterator > +{ > + const Type_handler *m_a; > + const Type_handler *m_b; > + uint m_switched_to_base_count; > +public: > + Recursive_type_pair_iterator(const Type_handler *a, > + const Type_handler *b, > + uint switched_to_base_count= 0) > + :m_a(a), m_b(b), m_switched_to_base_count(switched_to_base_count) > + { } > + const Type_handler *a() const { return m_a; } > + const Type_handler *b() const { return m_b; } > + Recursive_type_pair_iterator base() const > + { > + Recursive_type_pair_iterator res(m_a->type_handler_base(), > + m_b->type_handler_base()); > + res.m_switched_to_base_count= (res.m_a != NULL) + (res.m_b != NULL); > + if (res.m_a == NULL) > + res.m_a= m_a; > + if (res.m_b == NULL) > + res.m_b= m_b; > + return res; that's an unusual semantics, not what I would expect from an iterator. It'd expect it to iterare with it++ or it.next() but not it = it.base() > + } > + bool done() const > + { > + switch (m_switched_to_base_count) > + { > + case 2: I don't think case 2 is possible anymore. ... > + } > + } > +}; > + > + > bool > Type_handler_hybrid_field_type::aggregate_for_result(const Type_handler *other) > { > - const Type_handler *hres; > - const Type_collection *c; > - if (!(c= Type_handler::type_collection_for_aggregation(m_type_handler, other)) || > - !(hres= c->aggregate_for_result(m_type_handler, other))) > - hres= type_handler_data-> > - m_type_aggregator_for_result.find_handler(m_type_handler, other); > - if (!hres) > - return true; > - m_type_handler= hres; > - return false; > + Recursive_type_pair_iterator it(m_type_handler, other); > + for ( ; ; ) do you really still need to do recursive? in what cases? > + { > + const Type_handler *hres; > + const Type_collection *c; > + if (((c= Type_handler::type_collection_for_aggregation(it.a(), it.b())) && > + (hres= c->aggregate_for_result(it.a(), it.b()))) || > + (hres= type_handler_data-> > + m_type_aggregator_for_result.find_handler(it.a(), it.b()))) > + { > + m_type_handler= hres; > + return false; > + } > + if ((it= it.base()).done()) > + break; > + } > + return true; > } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 4

Re: [Maria-developers] e4ea1544097: MDEV-17124: mariadb 10.1.34, views and prepared statements: ERROR 1615 (HY000): Prepared statement needs to be re-prepared
by Sergei Golubchik 20 Jan '22

20 Jan '22

Hi, Oleksandr, On Jan 19, Oleksandr Byelkin wrote: > revision-id: e4ea1544097 (mariadb-10.2.40-198-ge4ea1544097) > parent(s): fd7a553cb80 > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2022-01-19 11:12:57 +0100 > message: > > MDEV-17124: mariadb 10.1.34, views and prepared statements: ERROR 1615 (HY000): Prepared statement needs to be re-prepared > > The problem is that if table definition cache (TDC) is full of real tables > which are in tables cache, view definition can not stay there so will be > removed by its own underlying tables. > In situation above old mechanism of detection matching definition in PS > and current version always require reprepare and so prevent executing > the PS. > > One work around is to increase TDC, other - improve version check for > views/triggers (which is done here). Now in suspicious cases we check: > - MD5 of the view to be sure that version really have changed; > - time of creation of a trigger related to time of statement preparation. > > diff --git a/mysql-test/r/ps_ddl.result b/mysql-test/r/ps_ddl.result > index e69c6e06193..005c78a0319 100644 > --- a/mysql-test/r/ps_ddl.result > +++ b/mysql-test/r/ps_ddl.result > @@ -214,7 +214,7 @@ new trigger: 10 > drop trigger t1_bd; > set @val=11; > execute stmt using @val; > -call p_verify_reprepare_count(1); > +call p_verify_reprepare_count(0); > SUCCESS > > select @message; > @@ -831,8 +831,8 @@ a b c > 20 40 100 > 30 60 150 > call p_verify_reprepare_count(1); > -SUCCESS > - > +ERROR > +Expected: 1, actual: 0 > # Check that we properly handle ALTER VIEW statements. > execute stmt; > a b c > @@ -882,9 +882,9 @@ a b c > 10 50 60 > 20 100 120 > 30 150 180 > -call p_verify_reprepare_count(1); > -SUCCESS > - > +call p_verify_reprepare_count(0); > +ERROR > +Expected: 0, actual: 1 > execute stmt; > a b c you forgot to update the expected number or reprepares in two places. > diff --git a/sql/parse_file.cc b/sql/parse_file.cc > index 999f53bd681..3c750f4df29 100644 > --- a/sql/parse_file.cc > +++ b/sql/parse_file.cc > @@ -145,6 +145,7 @@ write_parameter(IO_CACHE *file, uchar* base, File_option *parameter) > > switch (parameter->type) { > case FILE_OPTIONS_STRING: > + case FILE_OPTIONS_FIXSTRING: > { > LEX_STRING *val_s= (LEX_STRING *)(base + parameter->offset); > if (my_b_write(file, (const uchar *)val_s->str, val_s->length)) > @@ -830,6 +831,22 @@ File_parser::parse(uchar* base, MEM_ROOT *mem_root, > } > ptr= eol+1; > break; > + case FILE_OPTIONS_FIXSTRING: > + { > + /* string have to be allocated already and length set */ > + LEX_STRING *val= (LEX_STRING *)(base + parameter->offset); > + DBUG_ASSERT(val->length != 0); > + if (ptr[val->length] != '\n') > + { > + my_error(ER_FPARSER_ERROR_IN_PARAMETER, MYF(0), > + parameter->name.str, line); > + DBUG_RETURN(TRUE); > + } > + memcpy(val->str, ptr, val->length); > + val->str[val->length]= '\0'; > + ptr+= (val->length + 1); > + break; I'd suggest to use FILE_OPTIONS_STRING here, it's much simpler and it's known to work. This doesn't check that end - ptr < val->length, and it doesn't check that even if ptr[val->length] == '\n', then it's the first '\n' after ptr. It's all possible to fix, but using FILE_OPTIONS_STRING is just easier. > + } > case FILE_OPTIONS_TIMESTAMP: > { > /* string have to be allocated already */ > diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc > index bb1a99d9eef..b9198647f98 100644 > --- a/sql/sql_prepare.cc > +++ b/sql/sql_prepare.cc > @@ -4177,6 +4177,10 @@ bool Prepared_statement::prepare(const char *packet, uint packet_len) > Query_arena *old_stmt_arena; > DBUG_ENTER("Prepared_statement::prepare"); > DBUG_ASSERT(m_sql_mode == thd->variables.sql_mode); > + > + // The same format as for triggers to compare > + create_time= make_trigger_time(thd->query_start(), > + thd->query_start_sec_part()); Better use hrtime here, you might compare it with more than triggers in the future. For triggers, you can multiply it by 10000 for comparison. Or you can start storing trigger time with a microsecond precision - that's even better. > /* > If this is an SQLCOM_PREPARE, we also increase Com_prepare_sql. > However, it seems handy if com_stmt_prepare is increased always, > diff --git a/sql/sql_trigger.cc b/sql/sql_trigger.cc > index 52817ef65ae..38febce4fef 100644 > --- a/sql/sql_trigger.cc > +++ b/sql/sql_trigger.cc > @@ -929,7 +933,21 @@ bool Table_triggers_list::create_trigger(THD *thd, TABLE_LIST *tables, > > if (!sql_create_definition_file(NULL, &file, &triggers_file_type, > (uchar*)this, triggers_file_parameters)) > + { > + // delay if needed to be sure that time of creation is unique > + for (;;) > + { > + my_hrtime_t hrtime= my_hrtime(); > + ulonglong current_time= make_trigger_time(hrtime_to_my_time(hrtime), > + hrtime_sec_part(hrtime)); > + if (trigger->create_time == current_time) > + pthread_yield(); > + else > + break; > + } This shouldn't be needed. Better 1. store statement->create_time with microsecond precision 2. store trigger->create_time with microsecond precision 3. at the end of prepare wait (like above) to make sure statement->create_time is larger than all triggers' create_time. this will be a no-op unless someone created a trigger and prepared in the same microsecond. Safety check will ensure you won't wait longer than one microsecond. > DBUG_RETURN(false); > + } > > err_with_cleanup: > /* Delete .TRN file */ > diff --git a/sql/sql_view.cc b/sql/sql_view.cc > index 020830483c5..830a42b3cf3 100644 > --- a/sql/sql_view.cc > +++ b/sql/sql_view.cc > @@ -1131,7 +1141,31 @@ static int mysql_register_view(THD *thd, TABLE_LIST *view, > DBUG_RETURN(error); > } > > +/** > + Check is TABLE_LEST and SHARE match > + @param[in] view TABLE_LIST of the view > + @param[in] share Share object of view > + > + @return false on error or misspatch > +*/ > > +bool mariadb_view_version_get(TABLE_SHARE *share) > +{ > + DBUG_ASSERT(share->is_view); > + > + if (!(share->tabledef_version.str= > + (uchar*) alloc_root(&share->mem_root, VIEW_MD5_LEN + 1))) > + return TRUE; > + share->tabledef_version.length= VIEW_MD5_LEN; > + > + DBUG_ASSERT(share->view_def != NULL); > + if (share->view_def->parse((uchar *) &share->tabledef_version, NULL, > + view_md5_parameters, 1, > + &file_parser_dummy_hook)) > + return TRUE; > + DBUG_ASSERT(share->tabledef_version.length == VIEW_MD5_LEN); > + return FALSE; > +} I don't like using md5 as a uuid. md5 collisions are very much possible. Better options would be 1. use a timestamp, but store it with microsecond precisions, same logic as with triggers. 2. use a true tabledef uuid, new field in the view frm, same logic as with tables. timestamp is better, because it already exists in frms, and any old view will naturally have a timestamp before now(), so it doesn't matter that it won't have microseconds. uuid is better, because it's more universal, can be used for more than just reprepare checks. But for now I think timestamp benefits outweigh that. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 8f8e7ad5c80: MDEV-27217 DELETE partition selection doesn't work for history partitions
by Sergei Golubchik 13 Jan '22

13 Jan '22

Hi, Aleksey! ok to push. see one comment below. On Jan 13, Aleksey Midenkov wrote: > revision-id: 8f8e7ad5c80 (mariadb-10.3.31-80-g8f8e7ad5c80) > parent(s): 88f8aa20bba > author: Aleksey Midenkov > committer: Aleksey Midenkov > timestamp: 2022-01-12 21:47:03 +0300 > message: > > MDEV-27217 DELETE partition selection doesn't work for history partitions > > LIMIT history switching requires the number of history partitions to > be marked for read: from first to last non-empty plus one empty. The > least we can do is to fail with error message if the needed partition > was not marked for read. As this is handler interface we require new > handler error code to display user-friendly error message. > > diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc > index bd73642cd0d..2f945f29ed1 100644 > --- a/sql/ha_partition.cc > +++ b/sql/ha_partition.cc > @@ -9872,6 +9874,11 @@ void ha_partition::print_error(int error, myf errflag) > m_part_info->print_no_partition_found(table, errflag); > DBUG_VOID_RETURN; > } > + else if (error == HA_ERR_PARTITION_LIST) > + { > + handler::print_error(error, errflag); > + DBUG_VOID_RETURN; > + } this seems to be redundant, the else branch at the end covers it. > else if (error == HA_ERR_ROW_IN_WRONG_PARTITION) > { Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] ef70e809a40: MDEV-27217 DELETE partition selection doesn't work for history partitions
by Sergei Golubchik 12 Jan '22

12 Jan '22

Hi, Aleksey! On Jan 12, Aleksey Midenkov wrote: > revision-id: ef70e809a40 (mariadb-10.3.31-80-gef70e809a40) > parent(s): 88f8aa20bba > author: Aleksey Midenkov > committer: Aleksey Midenkov > timestamp: 2021-12-23 04:15:39 +0300 > message: > > MDEV-27217 DELETE partition selection doesn't work for history partitions > > LIMIT history switching requires the number of history partitions to > be marked for read: from first to last non-empty plus one empty. The > least we can do is to fail with error message if the needed partition > was not marked for read. As this is handler interface we require new > handler error code to display user-friendly error message. > > Switching by INTERVAL works out-of-the-box with > ER_ROW_DOES_NOT_MATCH_GIVEN_PARTITION_SET error. Code-wise it's pretty much fine. Two problems related to error messages: We cannot have part of the error message a hard-coded English phrase. It doesn't work with localized error messages: "Невозможно выбрать раздел: modifying system versioned table" Also we cannot add new error messages in 10.3. Or any old GA. Currently you can add new error messages to 10.6 or any later version. You can reuse ER_UNUSED_1 for your new error message, that'll work. And, because you cannot have English phrases in it, I'd suggest something more generic, like Not allowed for system-versioned table %`s.%`s In the context of delete from t1 partition (p1); ERROR HY000: Not allowed for system-versioned table `test`.`t1` it seems to be quite clear. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] 7a746072a5e: MDEV-26878: Query failing with syntax error sets ROW_NUMBER to non-zero
by Sergei Golubchik 12 Jan '22

12 Jan '22

Hi, Rucha! On Jan 12, Rucha Deodhar wrote: > On Tue, Jan 11, 2022 at 11:49 PM Sergei Golubchik <serg(a)mariadb.org> wrote: > > > +--error ER_PARSE_ERROR > > > +INSERT INTO t1 VALUES XXX (1),(2); > > > > > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > > > index 8ebefbb3d82..47ed3ded4a7 100644 > > > --- a/sql/sql_yacc.yy > > > +++ b/sql/sql_yacc.yy > > > @@ -8426,6 +8426,8 @@ query_specification_start: > > > { > > > SELECT_LEX *sel; > > > LEX *lex= Lex; > > > + if (lex->sql_command == SQLCOM_INSERT) > > > + thd->get_stmt_da()->inc_current_row_for_warning(); > > > if (!(sel= lex->alloc_select(TRUE)) || lex->push_select(sel)) > > > MYSQL_YYABORT; > > > > better not to have an if() in this rule that's used almost for > > everything. > > When we have INSERT...SELECT counter has to be 1 for errors like > ER_ILLEGAL_VALUE_FOR_TYPE which are caught during parsing. > Example: > INSERT INTO t1 SELECT a, b, 1.79769313486232e+308 FROM t2; > > So I added if() to increment the counter if we are parsing INSERT so > that ROW_NUMBER for statements like the one above gives ROW_NUMBER=1. I see. But you put a condition on every query's execution path to fix the case that, I believe, literally nobody cares about. I don't think any user ever will check the ROW_NUMBER after the syntax error :) And if someone will, we can say that in INSERT INTO t1 VALUES XXX (1),(2); the first row starts after VALUES, so ROW_NUMBER=1 is expected. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0