- developers - lists.mariadb.org

Re: [Maria-developers] da70f190279: MDEV-25985 Spider handle ">=" as ">" in some cases
by Sergei Golubchik 13 Jul '21

13 Jul '21

Hi, Nayuta! The code change is good, no comments about it at all. But why did you create a new suite spider/bugfix and all those files in it? I know that some later spider version has it. Still, I'd expect it'd be much simpler and a smaller change to drop the test file into the exising spider suite. On Jul 12, Nayuta Yanagisawa wrote: > revision-id: da70f190279 (mariadb-10.2.39-44-gda70f190279) > parent(s): 99f700a820e > author: Nayuta Yanagisawa <nayuta.yanagisawa(a)hey.com> > committer: Nayuta Yanagisawa <nayuta.yanagisawa(a)hey.com> > timestamp: 2021-07-08 14:12:45 +0000 > message: > > MDEV-25985 Spider handle ">=" as ">" in some cases > > The function spider_db_append_key_where_internal() converts > HA_READ_AFTER_KEY to '>'. The conversion seems to be correct for > single-column indexes because HA_READ_AFTER_KEY means "read the > key after the provided value." > > However, how about multi-column indexes? Assume that there is a > multi-column index on c1 and c2 and we search with the condition > 'c1 >= 100 AND c2 > 200'. The key_range.flag corresponds to the > search condition could be HA_READ_AFTER_KEY. In such a case, > we could not simply convert HA_READ_AFTER_KEY to '>'. > > The correct conversion is to convert HA_READ_AFTER_KEY to '>' > only for the last column in key_part_map and to convert > HA_READ_AFTER_KEY to '>=' for the other column. > > The similar discussion also applies to the conversion from > key_range.flag to a sign of inequality. > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] a6e96ce3c96: MDEV-21916: COM_STMT_BULK_EXECUTE with RETURNING insert wrong values
by Sergei Golubchik 12 Jul '21

12 Jul '21

Hi, Oleksandr! ok to push! On Jul 12, Oleksandr Byelkin wrote: > revision-id: a6e96ce3c96 (mariadb-10.5.4-666-ga6e96ce3c96) > parent(s): f4425d3a3db > author: Oleksandr Byelkin <sanja(a)mariadb.com> > committer: Oleksandr Byelkin <sanja(a)mariadb.com> > timestamp: 2021-06-09 10:43:24 +0200 > message: > > MDEV-21916: COM_STMT_BULK_EXECUTE with RETURNING insert wrong values > > The problem is that array binding uses net buffer to read parameters for each > execution while each execiting with RETURNING write in the same buffer. > > Solution is to allocate new net buffer to avoid changing buffer we are reading > from. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] b1881e07797: MDEV-20131 Assertion `!pk->has_virtual ()' failed in instant_alter_column_possible
by Sergei Golubchik 12 Jul '21

12 Jul '21

Hi, Sachin! On Jul 12, Sachin Kumar wrote: > revision-id: b1881e07797 (mariadb-10.4.20-9-gb1881e07797) > parent(s): 8a2b4d531dc > author: Sachin Kumar <sachin.setiya(a)mariadb.com> > committer: Sachin Kumar <sachin.setiya(a)mariadb.com> > timestamp: 2021-06-11 14:26:19 +0100 > message: > > MDEV-20131 Assertion `!pk->has_virtual ()' failed in instant_alter_column_possible > > Problem:- Primary key on long unique columns is not allowed but when > `CREATE TABLE t2 (a TEXT, PRIMARY KEY(a(1871))) ENGINE=InnoDB;` > is executed with innodb_page_size=8k or 4k, primary key on DB_ROW_HASH_1 column > is created. Reason of this is in mysql_prepare_create_table we check against > max_key_part_length() to see whether key_part length is more then engine supported > key_part length , and if it is true error is thrown for primary key and long > unique index is created for unique key. But in this case max_key_part_length() > returns 3072 which is more the max_key_length() , which later in code triggers > creating of long unique index. > > Solution:- max_supported_key_part_length() should return according to innodb > page size. > > diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc > index ce2e4b6990f..e5e864bffc5 100644 > --- a/storage/innobase/handler/ha_innodb.cc > +++ b/storage/innobase/handler/ha_innodb.cc > @@ -6537,14 +6537,21 @@ ha_innobase::clone( > DBUG_RETURN(new_handler); > } > > - > -uint > -ha_innobase::max_supported_key_part_length() const > -/*==============================================*/ > -{ > - /* A table format specific index column length check will be performed > - at ha_innobase::add_index() and row_create_index_for_mysql() */ > - return(REC_VERSION_56_MAX_INDEX_COL_LEN); > +uint ha_innobase::max_supported_key_part_length() const > +{ > + /* A table format specific index column length check will be performed > + at ha_innobase::add_index() and row_create_index_for_mysql() */ > + /* FIXME: rewrite this as well as ha_innobase::max_supported_key_length() > + using an API that considers the PRIMARY KEY as well as secondary index > + metadata and the ROW_FORMAT and KEY_BLOCK_SIZE */ > + switch (srv_page_size) { > + case 4096: > + return 1173; > + case 8192: > + return 1536; > + default: > + return REC_VERSION_56_MAX_INDEX_COL_LEN; > + } dunno, why would you duplicate the whole switch logic? can you just - return(REC_VERSION_56_MAX_INDEX_COL_LEN); - return MY_MIN(max_supported_key_length(), REC_VERSION_56_MAX_INDEX_COL_LEN); > } > > /******************************************************************//** > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 85186734823: MDEV-24248: my_print_defaults is not taking all the values when using -e
by Sergei Golubchik 12 Jul '21

12 Jul '21

Hi, Rucha! On Jul 12, Rucha Deodhar wrote: > revision-id: 85186734823 (mariadb-10.5.4-665-g85186734823) > parent(s): bf5c050fd28 > author: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > committer: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > timestamp: 2021-06-14 10:27:13 +0530 > message: > > MDEV-24248: my_print_defaults is not taking all the values when using -e > option which is called from mysql.server (extra_args). > > Fix: change mysql.server script to use --defaults-extra-file instead of -e > > diff --git a/mysql-test/main/my_print_defaults.test b/mysql-test/main/my_print_defaults.test > --- /dev/null > +++ b/mysql-test/main/my_print_defaults.test > @@ -0,0 +1,29 @@ > +--echo # > +--echo # Beginning of 10.5 Test > +--echo # > +--echo # > +--echo # MDEV-24248: my_print_defaults is not taking all the values when using > +--echo # -e option which is called from mysql.server (extra_args). > +--echo # > + > +--echo # checking for mysql.server > + > +--write_file $MYSQLTEST_VARDIR/tmp/tmp1.cnf > + > +[mariadb-10.5] don't use the version number in the test - you're forcing some poor soul to update the test on merges for every new version. > +key_buffer_size=20M > +max_allowed_packet=250M > +table_open_cache=1000 > +table_definition_cache=2000 > +read_buffer_size=1M > +thread_cache_size=8 > +max_connections=1024 > +long_query_time=60 > +slow_query_log=1 > +EOF > + > +--exec $MYSQL_MY_PRINT_DEFAULTS --defaults-extra-file=$MYSQLTEST_VARDIR/tmp/tmp1.cnf --mysqld mysql.server > + > +--remove_file $MYSQLTEST_VARDIR/tmp/tmp1.cnf > + > +--echo # End of 10.5 test > diff --git a/support-files/mysql.server.sh b/support-files/mysql.server.sh > index cefb3225e9e..8504e4def4b 100644 > --- a/support-files/mysql.server.sh > +++ b/support-files/mysql.server.sh > @@ -200,11 +200,11 @@ su_kill() { > extra_args="" > if test -r "$basedir/my.cnf" > then > - extra_args="-e $basedir/my.cnf" > + extra_args="--defaults-file-name=$basedir/my.cnf" eh. -e is --defaults-extra-file same below. > else > if test -r "$datadir/my.cnf" > then > - extra_args="-e $datadir/my.cnf" > + extra_args="--defaults-file-name=$datadir/my.cnf" > fi > fi > > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] b29ec61724d: MDEV-25906: SIGSEGV in flush_tables_with_read_lock on FTWRL or FTFE | SIGSEGV in ha_maria::extra
by Sergei Golubchik 01 Jul '21

01 Jul '21

Hi, Oleksandr! On Jul 01, Oleksandr Byelkin wrote: > revision-id: b29ec61724d (mariadb-10.6.2-41-gb29ec61724d) > parent(s): 83684fc9a4d > author: Oleksandr Byelkin <sanja(a)mariadb.com> > committer: Oleksandr Byelkin <sanja(a)mariadb.com> > timestamp: 2021-07-01 11:20:29 +0200 > message: > > MDEV-25906: SIGSEGV in flush_tables_with_read_lock on FTWRL or FTFE | SIGSEGV in ha_maria::extra > > Fixed check of derived tables of all kinds (view, derived, information schema). > > diff --git a/sql/sql_reload.cc b/sql/sql_reload.cc > index 0fa2fa10df8..e41da9680b0 100644 > --- a/sql/sql_reload.cc > +++ b/sql/sql_reload.cc > @@ -592,10 +592,19 @@ bool flush_tables_with_read_lock(THD *thd, TABLE_LIST *all_tables) > for (TABLE_LIST *table_list= all_tables; table_list; > table_list= table_list->next_global) > { > - if (!(table_list->is_view() || > - table_list->table->file->ha_table_flags() & HA_CAN_EXPORT)) > + /* > + We allow all deriveds and view here, but prohibit information_schema > + tables (explicitly) and performance_schema tables (implicetly: > + engine do not suport export) > + */ > + if (table_list->is_non_derived() && > + (!table_list->is_base_table() || > + !(table_list->table->file->ha_table_flags() & HA_CAN_EXPORT))) > { > - my_error(ER_ILLEGAL_HA, MYF(0),table_list->table->file->table_type(), > + my_error(ER_ILLEGAL_HA, MYF(0), > + (table_list->is_base_table() ? > + table_list->table->file->table_type(): > + "information_schema"), I'd say if you want to detect I_S tables, you should test for table_list->schema_table. It's not clear what your condition actually does. And there is no such engine "information_schema", so the message is misleading. In fact, this should be access denied. For both. Like: mysqltest: At line 12: query 'flush table information_schema.collations for export' failed: ER_DBACCESS_DENIED_ERROR (1044): Access denied for user 'foo'@'localhost' to database 'information_schema' this is from a test file that only does flush table information_schema.collations for export. > table_list->db.str, table_list->table_name.str); > goto error_reset_bits; > } > @@ -607,7 +616,8 @@ bool flush_tables_with_read_lock(THD *thd, TABLE_LIST *all_tables) > for (auto table_list= all_tables; table_list; > table_list= table_list->next_global) > { > - if (!table_list->is_view() && > + if (table_list->table && why do you check for table_list->table ? Can it happen that table_list->table == NULL and table_list->is_base_table() == true? > + table_list->is_base_table() && > table_list->table->file->extra(HA_EXTRA_FLUSH)) > goto error_reset_bits; > } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

[Maria-developers] MDEV-25325 built-in documentation for performance_schema tables
by Haidong Ji 24 Jun '21

24 Jun '21

Hi, I've grabbed this JIRA story, https://jira.mariadb.org/browse/MDEV-25325 . The coding and test cases creation were all done. I've also submitted a PR here: https://github.com/MariaDB/server/pull/1830 The PR was submitted more than 1 month ago. I'm curious about next steps. Thanks, Haidong

1 0

[Maria-developers] JSON_TABLE: on default values
by Sergey Petrunia 16 Jun '21

16 Jun '21

Hi Alexey, At the moment MariaDB requires that the values in DEFAULT clauses are quoted. Example: select * from json_table( '{"intval": 1000}', '$' columns( col1 int path '$.intval_' default '100' on empty ) ) as T; here, "100" must be quoted, otherwise one gets a parse error. However, the quoted value is interpreted as an SQL literal. This looks puzzling. MySQL-8 also requires that the default value is quoted, but they have a (very odd) reason for it: they interpret the default value as JSON: https://docs.oracle.com/cd/E17952_01/mysql-8.0-en/json-table-functions.html says: DEFAULT json_string ON EMPTY: the provided json_string is parsed as JSON, as long as it is valid, and stored instead of the missing value. Column type rules also apply to the default value. I am not sure why MySQL chose to do this. Looking into the SQL Standard, one can see: <JSON table regular column definition> ::= <column name> <data type> [ PATH <JSON table column path specification> ] [ <JSON table column empty behavior> ON EMPTY ] [ <JSON table column error behavior> ON ERROR ] <JSON table column empty behavior> ::= ERROR | NULL | DEFAULT <value expression> ... This doesn't say whether the <value expression> should be interepreted as JSON or just as a value. But one can find this passage: <quote> Without Feature T826, “General value expression in ON ERROR or ON EMPTY clauses”, the <value expression> contained in <JSON table column empty behavior> or <JSON table column error behavior> contained in a <JSON table regular column definition> JTRCD shall be a <literal> that can be cast to the data type specified by the <data type> contained in JTRCD without raising an exception condition according to the General Rules of Subclause 6.13, “<cast specification>”. </quote> The important part is: ... shall be a <literal> that can be cast to the data type specified ... which means it is not JSON. It is just a literal, and literal can be a string literal (in quotes, 'string') or an integer literal (without quotes) or other kind of literal. Btw, Oracle Database allows non-string literals in the default clause: https://dbfiddle.uk/?rdbms=oracle_18&fiddle=9af7e43ede77ee285e1a65f1f419d3bd What are your thoughts on this? Is MariaDB's behavior intentional? Should we follow the standard and allow all kinds of literals? What was the reason for the limitation that default values are quoted? BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

2 4

[Maria-developers] MDEV-25875: RE 02469b: MDEV-17399 JSON_TABLE Accept JSON values for the JSON fields
by Sergey Petrunia 16 Jun '21

16 Jun '21

Hi Alexey, This is review input re https://github.com/MariaDB/server/commit/02469bdead5753eccb5d70c98a158a0702…. First, our workflow dictates that this patch should have its own MDEV entry. I've filed it https://jira.mariadb.org/browse/MDEV-25875. Please use it in commit descriptions. == Updates mixed with another patch == The patch has .result changes like: > @@ -551,10 +551,12 @@ JSON_TABLE('{}', '$' COLUMNS (x INT PATH '$.x' DEFAULT NULL ON ERROR)) jt; > ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'NULL ON ERROR)) jt' at line 2 > SELECT * FROM > JSON_TABLE('{}', '$' COLUMNS (x INT PATH '$.x' DEFAULT 0 ON EMPTY)) jt; > -ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '0 ON EMPTY)) jt' at line 2 > +x > +0 > SELECT * FROM > JSON_TABLE('{}', '$' COLUMNS (x INT PATH '$.x' DEFAULT 0 ON ERROR)) jt; > -ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '0 ON ERROR)) jt' at line 2 > +x > +NULL > SELECT * FROM > JSON_TABLE('{}', '$' COLUMNS (x DATE > PATH '$.x' These are obviously from another patch (the one about "accepting non-string literals as default"). Please move them to that patch. == Comments == > @@ -377,6 +378,25 @@ static void store_json_in_field(Field *f, const json_engine_t *je) > } > > > +static int store_json_in_json(Field *f, json_engine_t *je) > +{ Please add a comment for the function. Something like "Store the current JSON element pointed by je into field f, as a separate JSON document" The name "store_json_in_json" doesn't look good to me. Do you think store_element_as_json_doc() would be better? > + const uchar *from= je->value_begin; > + const uchar *to; > + > + if (json_value_scalar(je)) > + to= je->value_end; > + else > + { > + int error; > + if ((error= json_skip_level(je))) > + return error; > + to= je->s.c_str; > + } > + f->store((const char *) from, (uint32) (to - from), je->s.cs); > + return 0; > +} > + > + > bool Json_table_nested_path::check_error(const char *str) > { > if (m_engine.s.error) > @@ -541,7 +561,12 @@ int ha_json_table::fill_column_values(THD *thd, uchar * buf, uchar *pos) > } > else > { > - if (!(error= !json_value_scalar(&je))) > + if (jc->m_format_json) > + { > + if (!(error= store_json_in_json(*f, &je))) > + error= er_handler.errors; Can there be a case when this assignment assigns any value other than 0? (My logic - er_handler.errors already had some error status, why did we call store_json_in_json? If there was an error inside store_json_in_json, we do not get to this assignment, as store_json_in_json() returned non-zero error. Is this correct?) > + } > + else if (!(error= !json_value_scalar(&je))) > { > store_json_in_field(*f, &je); > error= er_handler.errors; > @@ -868,6 +893,10 @@ int Json_table_column::set(THD *thd, enum_type ctype, const LEX_CSTRING &path) > anctual content. Not sure though if we should. > */ > m_path.s.c_str= (const uchar *) path.str; > + > + if (ctype == PATH) > + m_format_json= m_field->type_handler() == &type_handler_json_longtext; This will probably cause warnings on some compiler. Please wrap into "MY_TEST(...)" or just brackets, "z= (x == y)". == A discrepancy between this and MySQL == I've found this case: select * from json_table('{"foo": null}', '$' columns( jscol json path '$.foo') ) as T; with the above patch, it produces a JSON null value: +-------+ | jscol | +-------+ | null | +-------+ while in MySQL-8 it produces an SQL NULL: +-------+ | jscol | +-------+ | NULL | +-------+ I'm not yet sure what we should do about this. Any thoughts? BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

2 1

[Maria-developers] New Member (GSoC MDEV-25015)
by Alan Cueva 15 Jun '21

15 Jun '21

Hello everyone, I am Alan Cueva, during the summer I will be working on the Custom formatting of strings in MariaDB queries (MDEV-25015 <https://jira.mariadb.org/browse/MDEV-25015>) project, but I would like to continue contributing in this community. I hope to be useful and learn from everyone. Thanks!

1 0

Re: [Maria-developers] 27060eb6ba5: MDEV-21916: COM_STMT_BULK_EXECUTE with RETURNING insert wrong values
by Sergei Golubchik 10 Jun '21

10 Jun '21

Hi, Oleksandr! On Oct 12, Oleksandr Byelkin wrote: > revision-id: 27060eb6ba5 (mariadb-10.5.4-220-g27060eb6ba5) > parent(s): 861cd4ce286 > author: Oleksandr Byelkin <sanja(a)mariadb.com> > committer: Oleksandr Byelkin <sanja(a)mariadb.com> > timestamp: 2020-10-07 15:22:41 +0200 > message: > > MDEV-21916: COM_STMT_BULK_EXECUTE with RETURNING insert wrong values > > To allocate new net buffer to avoid changing bufer we are reading. You still didn't clarify the commit comment > diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc > index 7280236e43f..5aaff3cf623 100644 > --- a/sql/sql_delete.cc > +++ b/sql/sql_delete.cc > @@ -685,8 +685,14 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list, COND *conds, > !table->prepare_triggers_for_delete_stmt_or_event()) > will_batch= !table->file->start_bulk_delete(); > > - if (returning) > + /* > + thd->get_stmt_da()->is_set() means first iteration of prepared statement > + with array binding operation execution (non optimized so it is not > + INSERT) > + */ > + if (returning && !thd->get_stmt_da()->is_set()) > { > + DBUG_ASSERT(thd->lex->sql_command != SQLCOM_INSERT); a strange assert to see in sql_delete.cc :) can one even reach mysql_delete() with SQLCOM_INSERT? not just with returning and !thd->get_stmt_da()->is_set(), anywhere? > if (result->send_result_set_metadata(returning->item_list, > Protocol::SEND_NUM_ROWS | Protocol::SEND_EOF)) > goto cleanup; > diff --git a/sql/sql_error.cc b/sql/sql_error.cc > index b3ef0d89a98..a753af2b34d 100644 > --- a/sql/sql_error.cc > +++ b/sql/sql_error.cc > @@ -380,16 +380,33 @@ Diagnostics_area::set_eof_status(THD *thd) > if (unlikely(is_error() || is_disabled())) > return; > > + if (m_status == DA_EOF_BULK) > + { > /* > If inside a stored procedure, do not return the total > number of warnings, since they are not available to the client > anyway. > */ > + if (!thd->spcont) > + m_statement_warn_count+= current_statement_warn_count(); > + } > + else > + { > + /* > + If inside a stored procedure, do not return the total > + number of warnings, since they are not available to the client > + anyway. > + */ I don't think it helps to duplicate the comment. You could just put it once before the if. > if (thd->spcont) > { > m_statement_warn_count= 0; > + m_affected_rows= 0; why do you reset m_affected_rows too? > } > else > m_statement_warn_count= current_statement_warn_count(); > + m_status= (is_bulk_op() ? DA_EOF_BULK : DA_EOF); > + } do we have tests for bulk operations and CALL? > > - m_status= DA_EOF; > DBUG_VOID_RETURN; > } > > diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc > index ecb56e70f88..c144d3a8d7e 100644 > --- a/sql/sql_prepare.cc > +++ b/sql/sql_prepare.cc > @@ -4357,24 +4361,37 @@ Prepared_statement::execute_bulk_loop(String *expanded_query, > > + /* > + Here second buffer for not optimized commands, > + optimized commands do it inside thier internal loop. > + */ > + if (!(sql_command_flags[lex->sql_command] & CF_SP_BULK_OPTIMIZED) && why "SP" and not "PS" ? > + this->lex->has_returning()) what about CALL? It won't have lex->has_returning(). What about SELECT? Can you bind an array to a parameter in SELECT or CALL? > + { > + // Above check can be true for SELECT in future > + DBUG_ASSERT(lex->sql_command != SQLCOM_SELECT); how can SQLCOM_SELECT have lex->has_returning() ? > + readbuff= thd->net.buff; // old buffer > + if (net_allocate_new_packet(&thd->net, thd, MYF(MY_THREAD_SPECIFIC))) > + { > + readbuff= NULL; // failure, net_allocate_new_packet keeps old buffer > + goto err; > + } > } > > #ifndef EMBEDDED_LIBRARY Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 2