- developers - lists.mariadb.org

Re: [Maria-developers] 4156071ebe2: MDEV-26606: ERROR_INDEX property value isn't passed from inside a stored procedure
by Sergei Golubchik 28 Sep '21

28 Sep '21

Hi, Rucha! On Sep 28, Rucha Deodhar wrote: > revision-id: 4156071ebe2 (mariadb-10.6.1-91-g4156071ebe2) > parent(s): 8dd4794c4e1 > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2021-09-26 19:58:33 +0530 > message: > > MDEV-26606: ERROR_INDEX property value isn't passed from inside a stored > procedure > > Analysis: m_current_row_for_warning is reset to 1 during cleanup phase of > stored procedure. When we perform a copy because some statement of procedure > created warning, this reset value os passed to push _warning(). > Hence the output is always 1. > Fix: Add a parameter in relevant functions to pass correct value of > error index and don't use m_current_row_for_warning directly. first thought about test cases, ERROR_INDEX should show the same row number as in a warning, so let's check whether warning is correct or not too. In this case we have CREATE OR REPLACE TABLE t1 (pk TINYINT PRIMARY KEY); CREATE OR REPLACE PROCEDURE sp(a INT) INSERT INTO t1 VALUES (2),(a); INSERT INTO t1 VALUES(1); --error ER_WARN_DATA_OUT_OF_RANGE CALL sp(100000); prints ERROR 22003: Out of range value for column 'a' at row 2 which is fine. Would be good to have it in the test. Otherwise looks good. One question below. > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index 31f5faa72ed..4d7093763ae 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -1149,7 +1149,8 @@ Sql_condition* THD::raise_condition(uint sql_errno, > if (likely(!(is_fatal_error && (sql_errno == EE_OUTOFMEMORY || > sql_errno == ER_OUTOFMEMORY)))) > { > - cond= da->push_warning(this, sql_errno, sqlstate, level, ucid, msg); > + cond= da->push_warning(this, sql_errno, sqlstate, level, ucid, msg, > + da->current_row_for_warning()); > } > DBUG_RETURN(cond); > } > diff --git a/sql/sql_error.cc b/sql/sql_error.cc > index 7b6e6a83d2f..ca9da3dbf89 100644 > --- a/sql/sql_error.cc > +++ b/sql/sql_error.cc > @@ -663,7 +663,8 @@ void Warning_info::reserve_space(THD *thd, uint count) > > Sql_condition *Warning_info::push_warning(THD *thd, > const Sql_condition_identity *value, > - const char *msg) > + const char *msg, > + ulong current_error_index) > { > Sql_condition *cond= NULL; > > @@ -673,7 +674,7 @@ Sql_condition *Warning_info::push_warning(THD *thd, > m_warn_list.elements() < thd->variables.max_error_count) > { > cond= new (& m_warn_root) Sql_condition(& m_warn_root, *value, msg, > - m_current_row_for_warning); > + current_error_index); > if (cond) > m_warn_list.push_back(cond); > } > @@ -689,7 +690,8 @@ Sql_condition *Warning_info::push_warning(THD *thd, > const Sql_condition *sql_condition) > { > Sql_condition *new_condition= push_warning(thd, sql_condition, > - sql_condition->get_message_text()); > + sql_condition->get_message_text(), > + sql_condition->m_error_index); > > if (new_condition) > new_condition->copy_opt_attributes(sql_condition); > diff --git a/sql/sql_error.h b/sql/sql_error.h > index b6ed7f5061a..a3e00e8419d 100644 > --- a/sql/sql_error.h > +++ b/sql/sql_error.h > @@ -747,7 +747,8 @@ class Warning_info > */ > Sql_condition *push_warning(THD *thd, > const Sql_condition_identity *identity, > - const char* msg); > + const char* msg, > + ulong current_error_index); > > /** > Add a new SQL-condition to the current list and increment the respective > @@ -1178,10 +1179,12 @@ class Diagnostics_area: public Sql_state_errno, > const char* sqlstate, > Sql_condition::enum_warning_level level, > const Sql_user_condition_identity &ucid, > - const char* msg) > + const char* msg, > + ulong current_error_index) > { > Sql_condition_identity tmp(sql_errno_arg, sqlstate, level, ucid); > - return get_warning_info()->push_warning(thd, &tmp, msg); > + return get_warning_info()->push_warning(thd, &tmp, msg, > + current_error_index); > } > > Sql_condition *push_warning(THD *thd, > @@ -1191,7 +1194,7 @@ class Diagnostics_area: public Sql_state_errno, > const char* msg) > { > return push_warning(thd, sqlerrno, sqlstate, level, > - Sql_user_condition_identity(), msg); > + Sql_user_condition_identity(), msg, 0); what is this push_warning used for? does it never need a proper current_error_index? > } > void mark_sql_conditions_for_removal() > { get_warning_info()->mark_sql_conditions_for_removal(); } > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] INSTEAD OF triggers
by Daniel Black 27 Sep '21

27 Sep '21

Opps, meant to send to list too. On Sat, Sep 25, 2021 at 5:28 PM Daniel Black <daniel(a)mariadb.org> wrote: > > Hi Alexandre, > > Given the aws blog is written around that Oracle SE doesn't have > partitions and EE does, does it still make sense with MariaDB (that > includes partitions as standard - > https://mariadb.com/kb/en/partitioning-tables/)? > > Take a look at the way partitions are implemented, they may offer what > you plan to write in a slightly different way. > > If you are sure you want to embark on this journey, there is a > feature request for this issue > https://jira.mariadb.org/browse/MDEV-12151 > > Recommend starting by describing exactly what you plan to implement in > this issue. Reference existing implementations like > https://www.postgresql.org/docs/current/sql-createtrigger.html make > notes of differences and what you plan to cover (and not cover) > > I assume you've come across the generic > https://mariadb.org/get-involved/getting-started-for-developers/ . > Start with a fork from the 10.7 branch > > First bit is sql/sql_yacc.yy trg_action_time that is the parsing syntax. > > If you `git grep TRG_ACTION_AFTER` the code you should contain the > places that will need a TRG_ACTION_INSTEAD_OF handling. > > Also look at the set of tests in $ find mysql-test -name \*trigger\* > > Any problems, find us on > https://mariadb.zulipchat.com/#narrow/stream/118759-general for some > more realtime help. > > On Sat, Sep 25, 2021 at 1:44 AM Alexandre hadjinlian guerra > <alexhguerra(a)gmail.com> wrote: > > > > Hello > > > > Im looking forward to implement this feature in MariaDB, and will look after trigger definition code/modules in MariaDB, but any wisdom that could be shared to speed up my efforts would be greatly appreciated. > > > > The idea is to provide the same functionality as describe here > > https://aws.amazon.com/pt/blogs/database/implementing-table-partitioning-in… > > > > Thanks > > > > _______________________________________________ > > Mailing list: https://launchpad.net/~maria-developers > > Post to : maria-developers(a)lists.launchpad.net > > Unsubscribe : https://launchpad.net/~maria-developers > > More help : https://help.launchpad.net/ListHelp

1 0

Re: [Maria-developers] b248988c8b6: MDEV-19275 Provide SQL service to plugins.
by Sergei Golubchik 14 Sep '21

14 Sep '21

Hi, Alexey! see questions below On Sep 07, Alexey Botchkov wrote: > revision-id: b248988c8b6 (mariadb-10.6.1-74-gb248988c8b6) > parent(s): 42ad4fa3464 > author: Alexey Botchkov > committer: Alexey Botchkov > timestamp: 2021-09-07 14:08:13 +0400 > message: > > MDEV-19275 Provide SQL service to plugins. > > SQL service added. > It provides the limited set of client library functions > to be used by plugin. > > diff --git a/include/mysql/service_sql.h b/include/mysql/service_sql.h > new file mode 100644 > index 00000000000..5050793f655 > --- /dev/null > +++ b/include/mysql/service_sql.h > @@ -0,0 +1,99 @@ > +/* Copyright (C) 2021 MariaDB Corporation > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; version 2 of the License. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program; if not, write to the Free Software > + Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02111-1301 USA */ > + > +#if defined(MYSQL_SERVER) && !defined MYSQL_SERVICE_SQL > +#define MYSQL_SERVICE_SQL > + > +#include <mysql.h> > + > +/** > + @file > + SQL service > + > + Interface for plugins to execute SQL queries on the local server. > + > + Functions of the service are the 'server-limited' client library: > + mysql_init > + mysql_real_connect_local > + mysql_real_connect > + mysql_errno > + mysql_error > + mysql_real_query > + mysql_affected_rows > + mysql_num_rows > + mysql_store_result > + mysql_free_result > + mysql_fetch_row > + mysql_close > +*/ > + > + > +#ifdef __cplusplus > +extern "C" { > +#endif > + > +extern struct sql_service_st { > + MYSQL * STDCALL (*mysql_init)(MYSQL *mysql); > + MYSQL *(*mysql_real_connect_local)(MYSQL *mysql, > + const char *host, const char *user, const char *db, > + unsigned long clientflag); > + MYSQL * STDCALL (*mysql_real_connect)(MYSQL *mysql, const char *host, > + const char *user, const char *passwd, const char *db, unsigned int port, > + const char *unix_socket, unsigned long clientflag); > + unsigned int STDCALL (*mysql_errno)(MYSQL *mysql); > + const char * STDCALL (*mysql_error)(MYSQL *mysql); > + int STDCALL (*mysql_real_query)(MYSQL *mysql, const char *q, > + unsigned long length); > + my_ulonglong STDCALL (*mysql_affected_rows)(MYSQL *mysql); > + my_ulonglong STDCALL (*mysql_num_rows)(MYSQL_RES *res); > + MYSQL_RES * STDCALL (*mysql_store_result)(MYSQL *mysql); > + void STDCALL (*mysql_free_result)(MYSQL_RES *result); > + MYSQL_ROW STDCALL (*mysql_fetch_row)(MYSQL_RES *result); > + void STDCALL (*mysql_close)(MYSQL *sock); > +} *sql_service; > + > +#ifdef MYSQL_DYNAMIC_PLUGIN > + > +#define mysql_init sql_service->mysql_init > +#define mysql_real_connect_local sql_service->mysql_real_connect_local > +#define mysql_real_connect sql_service->mysql_real_connect > +#define mysql_errno(M) sql_service->mysql_errno(M) > +#define mysql_error(M) sql_service->mysql_error(M) > +#define mysql_real_query sql_service->mysql_real_query > +#define mysql_affected_rows sql_service->mysql_affected_rows > +#define mysql_num_rows sql_service->mysql_num_rows > +#define mysql_store_result sql_service->mysql_store_result > +#define mysql_free_result sql_service->mysql_free_result > +#define mysql_fetch_row sql_service->mysql_fetch_row > +#define mysql_close sql_service->mysql_close > + > +#else > + > +MYSQL *mysql_real_connect_local(MYSQL *mysql, > + const char *host, const char *user, const char *db, > + unsigned long clientflag); normally the service file is the authoritative source of the service documentation, everything there is to know about the service should be there. In this case it's mostly the normal client C API, so no need to document it. But mysql_real_connect_local is new, so please, add a comment here, explaining what it is. For example, I'd think it connects to the current server, internally. But then, why does it need host and user? The comment should cover it. > + > +/* The rest of the function declarations mest be taken from the mysql.h */ > + > +#endif /*MYSQL_DYNAMIC_PLUGIN*/ > + > + > +#ifdef __cplusplus > +} > +#endif > + > +#endif /*MYSQL_SERVICE_SQL */ > + > + > diff --git a/plugin/test_sql_service/CMakeLists.txt b/plugin/test_sql_service/CMakeLists.txt > index aa9ecfe685e..7c61a1c3c7a 100644 > --- a/plugin/test_sql_service/CMakeLists.txt > +++ b/plugin/test_sql_service/CMakeLists.txt > @@ -15,4 +15,5 @@ > > SET(SOURCES test_sql_service.c) > > -MYSQL_ADD_PLUGIN(test_sql_service ${SOURCES} MODULE_ONLY RECOMPILE_FOR_EMBEDDED) > +ADD_DEFINITIONS(-DMYSQL_SERVER) why? > +MYSQL_ADD_PLUGIN(test_sql_service ${SOURCES} MODULE_ONLY) > diff --git a/plugin/test_sql_service/test_sql_service.c b/plugin/test_sql_service/test_sql_service.c > index 062f10fce58..e1155a98c40 100644 > --- a/plugin/test_sql_service/test_sql_service.c > +++ b/plugin/test_sql_service/test_sql_service.c > @@ -14,71 +14,113 @@ > Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335 USA */ > > > -#define PLUGIN_VERSION 0x100 > -#define PLUGIN_STR_VERSION "1.0.0" > - > -#define _my_thread_var loc_thread_var > +#define PLUGIN_VERSION 0x20000 > +#define PLUGIN_STR_VERSION "2.0" > > #include <my_config.h> > -#include <assert.h> > #include <my_global.h> > #include <my_base.h> > -#include <typelib.h> > -//#include <mysql_com.h> /* for enum enum_server_command */ > -#include <mysql/plugin.h> > #include <mysql/plugin_audit.h> > -//#include <string.h> > - > +#include <mysql.h> shouldn't be needed > > -LEX_STRING * thd_query_string (MYSQL_THD thd); > -unsigned long long thd_query_id(const MYSQL_THD thd); > -size_t thd_query_safe(MYSQL_THD thd, char *buf, size_t buflen); > -const char *thd_user_name(MYSQL_THD thd); > -const char *thd_client_host(MYSQL_THD thd); > -const char *thd_client_ip(MYSQL_THD thd); > -LEX_CSTRING *thd_current_db(MYSQL_THD thd); > -int thd_current_status(MYSQL_THD thd); > -enum enum_server_command thd_current_command(MYSQL_THD thd); > - > -int maria_compare_hostname(const char *wild_host, long wild_ip, long ip_mask, > - const char *host, const char *ip); > -void maria_update_hostname(const char **wild_host, long *wild_ip, long *ip_mask, > - const char *host); > > /* Status variables for SHOW STATUS */ > static long test_passed= 0; > +static char *sql_text_local, *sql_text_global; > +static char qwe_res[1024]= ""; > + > static struct st_mysql_show_var test_sql_status[]= > { > {"test_sql_service_passed", (char *)&test_passed, SHOW_LONG}, > + {"test_sql_query_result", qwe_res, SHOW_CHAR}, > {0,0,0} > }; > > static my_bool do_test= TRUE; > -static void run_test(MYSQL_THD thd, struct st_mysql_sys_var *var, > - void *var_ptr, const void *save); > -static MYSQL_SYSVAR_BOOL(run_test, do_test, PLUGIN_VAR_OPCMDARG, > - "Perform the test now.", NULL, run_test, FALSE); > +static int run_test(MYSQL_THD thd, struct st_mysql_sys_var *var, void *save, > + struct st_mysql_value *value); > +static int run_sql_local(MYSQL_THD thd, struct st_mysql_sys_var *var, void *save, > + struct st_mysql_value *value); > +static int run_sql_global(MYSQL_THD thd, struct st_mysql_sys_var *var, void *save, > + struct st_mysql_value *value); > + > +static void noop_update(MYSQL_THD thd, struct st_mysql_sys_var *var, > + void *var_ptr, const void *save); > + > +static MYSQL_SYSVAR_BOOL(run_test, do_test, > + PLUGIN_VAR_OPCMDARG, > + "Perform the test now.", > + run_test, NULL, FALSE); > + > +static MYSQL_SYSVAR_STR(execute_sql_local, sql_text_local, > + PLUGIN_VAR_OPCMDARG, > + "Create the new local connection, execute SQL statement with it.", > + run_sql_local, noop_update, FALSE); > + > +static MYSQL_SYSVAR_STR(execute_sql_global, sql_text_global, > + PLUGIN_VAR_OPCMDARG, > + "Execute SQL statement using the global connection.", > + run_sql_global, noop_update, FALSE); Why did you test local and global connections? What difference in behavior can one expect? > + > static struct st_mysql_sys_var* test_sql_vars[]= > { > MYSQL_SYSVAR(run_test), > + MYSQL_SYSVAR(execute_sql_local), > + MYSQL_SYSVAR(execute_sql_global), > NULL > }; > > diff --git a/sql/mysqld.cc b/sql/mysqld.cc > index ef6f40778fe..8a941a68e53 100644 > --- a/sql/mysqld.cc > +++ b/sql/mysqld.cc > @@ -5064,6 +5075,9 @@ static int init_server_components() > > tc_log= 0; // ha_initialize_handlerton() needs that > > + if (ddl_log_initialize()) > + unireg_abort(1); why? > + > if (plugin_init(&remaining_argc, remaining_argv, > (opt_noacl ? PLUGIN_INIT_SKIP_PLUGIN_TABLE : 0) | > (opt_abort ? PLUGIN_INIT_SKIP_INITIALIZATION : 0))) > diff --git a/sql/sql_class.h b/sql/sql_class.h > index e569fcd32d6..964626be3d4 100644 > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc > index 09ad632dd98..4725855c130 100644 > --- a/sql/sql_prepare.cc > +++ b/sql/sql_prepare.cc > @@ -4857,6 +4870,7 @@ Prepared_statement::execute_server_runnable(Server_runnable *server_runnable) > if (!(lex= new (mem_root) st_lex_local)) > return TRUE; > > + thd->set_time(); why? May be it should be executed using the timestamp of the top-level statement? > thd->set_n_backup_statement(this, &stmt_backup); > thd->set_n_backup_active_arena(this, &stmt_backup); > thd->stmt_arena= this; > @@ -6153,23 +6163,193 @@ static my_bool loc_read_query_result(MYSQL *mysql) > + > extern "C" MYSQL *mysql_real_connect_local(MYSQL *mysql, > - const char *host, const char *user, const char *passwd, const char *db) > + const char *host, const char *user, const char *db, you aren't using `host` or `db` for anything. And I don't understand what you're using `user` for. > + unsigned long clientflag) > { > - //char name_buff[USERNAME_LENGTH]; > - > + THD *thd_orig= current_thd; > + THD *new_thd; > + Protocol_local *p; > DBUG_ENTER("mysql_real_connect_local"); > > /* Test whether we're already connected */ > diff --git a/sql/thread_pool_info.cc b/sql/thread_pool_info.cc > index 90ac6871784..e3ffd160a11 100644 > --- a/sql/thread_pool_info.cc > +++ b/sql/thread_pool_info.cc > @@ -14,9 +14,9 @@ along with this program; if not, write to the Free Software > Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02111 - 1301 USA*/ > > #include <mysql_version.h> > -#include <mysql/plugin.h> > > #include <my_global.h> > +#include <mysql/plugin.h> shouldn't be needed, mysql/plugin.h should not need my_global.h > #include <sql_class.h> > #include <sql_i_s.h> > #include <mysql/plugin.h> > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 6

Re: [Maria-developers] 48261114b9d: MDEV-26572 Improve simple multibyte collation performance on the ASCII range
by Sergei Golubchik 13 Sep '21

13 Sep '21

Hi, Alexander! please add more benchmark results to the MDEV, all the variants that you've tried and speedups (or slowdowns) for each. otherwise, basically, no comments, see below. the only real concern is to know exactly when it helps and when it doesn't. On Sep 11, Alexander Barkov wrote: > revision-id: 48261114b9d (mariadb-10.6.1-77-g48261114b9d) > parent(s): 76149650764 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2021-09-09 15:20:57 +0400 > message: > > MDEV-26572 Improve simple multibyte collation performance on the ASCII range > > diff --git a/strings/ctype-ascii.h b/strings/ctype-ascii.h > new file mode 100644 > index 00000000000..66c9cfb3c6d > --- /dev/null > +++ b/strings/ctype-ascii.h > @@ -0,0 +1,189 @@ > +#ifndef CTYPE_ASCII_H_INCLUDED > +#define CTYPE_ASCII_H_INCLUDED The convention is CTYPE_ASCII_INCLUDED (every file has .h, so it's omitted from the include guard) > + > +#include "myisampack.h" > + > +/* > + Magic expression. It uses the fact that for any byte value X in > + the range 0..31 (0x00..0x1F) the expression (X+31)*5 returns > + the 7th bit (0x80) set only for the following six (out of 32) values: > + 0x00, 0x1B, 0x1C, 0x1D, 0x1E, 0x1F. > + These values correspond to offsets of non-letter characters > + in the ASCII table: > + > + The following macro sets the bit 0x20 for the following characters: > + ---------------- -------------------------------- > + Magic bit 10000000000000000000000000011111 > + ASCII 0x00..0x1F ................................ Control > + ASCII 0x20..0x3F ................................ Punctuation, digits > + ASCII 0x40..0x5F @ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_ > + ASCII 0x60..0x7F `abcdefghijklmnopqrstuvwxyz{|}~. > + ---------------- -------------------------------- > + We shift the magic bit 0x80 right twice to make it 0x20. > + So on the ranges [40..5F] and [60..7F] the expression > + has the bit 0x20 set for all non-letter characters. > + Note, other bits contain garbage. > + > + Requirements: > + All bytes must be in the range [00..7F], > + to avoid overflow and carry to the next byte. > +*/ > +#define MY_ASCII_20_IS_SET_IF_NOT_LETTER_MAGIC(i) \ > + (((((i)+0x1F1F1F1F1F1F1F1FULL) & 0x1F1F1F1F1F1F1F1F) * 5) >> 2) > + > + > +/* > + The following macro returns the bit 0x20 set to: > + - 1 for input bytes in the ranges [60..7F] or [E0..FF] > + - 0 otherwise > + Bytes in the ranges [40..7F] and [C0..FF] have the bit 0x40 set. > + Bytes in the ranges [60..7F] smd [E0..FF] have the bit 0x20 set. s/smd/and/ > + Hex BinHi BinLo > + ---- -1-- ---- > + 0x[4C]X .10. .... > + 0x[5D]X .10. .... > + 0x[6E]X .11. .... > + 0x[7F]X .11. .... > +*/ > +#define MY_ASCII_20_IS_SET_IF_RANGE_60_7F_OR_E0_FF(i) (((i) >> 1) & ((i))) > + > + > +/* > + The following macro evaluates to exactly 0x20 for all > + lower case ASCII letters [a-z], and to 0x00 otherwise: > + > + Value Range Character range Subrange > + -------- -------- -------------------------------- ------- > + 00000000 0x00..0x3F Control, punctuation, digits > + 00100000 0x40..0x5F @ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_ letters A-Z > + 00000000 0x40..0x5F @ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_ non-letters > + 00100000 0x60..0x7F `abcdefghijklmnopqrstuvwxyz{|}~. letters a-z > + 00000000 0x60..0x7F `abcdefghijklmnopqrstuvwxyz{|}~. non-letters > + > + Requirements: > + All bytes must be in the range [00..7F]. > + See the comments in MY_ASCII_20_IS_SET_IF_NOT_LETTER_MAGIC(). > +*/ > + > +#define MY_ASCII_20_IF_IS_LOWER_LETTER(i) \ > + (MY_ASCII_20_IS_SET_IF_RANGE_60_7F_OR_E0_FF(i) & \ > + ~MY_ASCII_20_IS_SET_IF_NOT_LETTER_MAGIC(i) & \ > + 0x2020202020202020) > + > +/* > + Convert lower case ASCII letters to upper case by unsetting > + the bit 0x20 with help of the magic expression. > + > + Requirements: > + All bytes must be in the range [00..7F]. > + See the comments in MY_ASCII_20_IS_SET_IF_NOT_LETTER_MAGIC() > +*/ > +#define MY_ASCII_TOUPPER_MAGIC(i) \ > + (i ^ MY_ASCII_20_IF_IS_LOWER_LETTER(i)) > + > + > +/* > + Convert a string (consisting of 8 bytes stored in uint64) > + to upper case algorithmically. > + > + Requirements: > + All bytes must be in the range [00..0x7F]. > + See the comments in MY_ASCII_20_IS_SET_IF_NOT_LETTER_MAGIC(). > + The result on 8bit data is unpredictable!!! > + The caller should make sure not to pass 8bit data. > +*/ > +static inline ulonglong my_ascii_to_upper_magic_uint64(ulonglong i) > +{ > + return MY_ASCII_TOUPPER_MAGIC(i); > +} > + > + > +/* > + Check if: > + - both strings "a" and "b" have at least 4 bytes, and > + - both strings have only 7bit data. > +*/ > +static inline int > +my_strcoll_ascii_4bytes_found(const uchar *a, const uchar *ae, > + const uchar *b, const uchar *be) > +{ > + return a + 4 <= ae && b + 4 <= be && > + (uint4korr(b) & 0x80808080) == 0 && > + (uint4korr(a) & 0x80808080) == 0; > +} > + > + > +/* > + Compare the leading four 7bit ASCII bytes in two strings case insensitively > + by converting letters [a-z] to upper case [A-Z]. > + > + Requirements: > + - The input strings must have at least four bytes, and > + - The leading four bytes in both strings must be 7bit ASCII. > + The caller must make sure to provide only strings that meet > + these requirements. The result on 8-bit data is unpredictable > + as 8-bit bytes may cause overflow in my_ascii_to_upper_magic_uint64(). > + See comments above. > +*/ > +static inline int > +my_strcoll_ascii_toupper_4bytes(const uchar *a, const uchar *b) > +{ > + ulonglong abn= (((ulonglong) mi_uint4korr(a)) << 32) | mi_uint4korr(b); > + abn= my_ascii_to_upper_magic_uint64(abn); > + if ((uint) (abn >> 32) == (uint32) abn) s/uint/uint32/ ? > + return 0; > + return ((uint32) (abn >> 32)) < ((uint32) abn) ? -1 : + 1; > +} > + > + > +/* > + Compare the leading eight 7bit ASCII bytes in two strings case insensitively > + by converting letters [a-z] to upper case [A-Z]. > + > + Requirements: > + - The input strings must have at least eight bytes, and > + - The leading eight bytes in both strings must be 7bit ASCII. > + See comments in my_strcoll_ascii_toupper_4bytes(). > +*/ > +static inline int > +my_strcoll_ascii_toupper_8bytes(const uchar *a, const uchar *b) > +{ > + /* > + TODO: > + Try to get advantage of SIMD instructions by massive comparison > + (16 bytes at a time) of characters against (x>='a' && x<='z') using: > + - either explicit intrinsics I thought your SIMD version was slower, wasn't it? > + - or a loop that can get vectorized automatically by some compilers. > + */ > + ulonglong an= mi_uint8korr(a); > + ulonglong bn= mi_uint8korr(b); > + an= my_ascii_to_upper_magic_uint64(an); > + bn= my_ascii_to_upper_magic_uint64(bn); > + return an == bn ? 0 : an < bn ? -1 : +1; > +} > + > + > +/* > + Compare the leading four 7bit ASCII bytes in two strings in binary style. > +*/ > +static inline int > +my_strcoll_mb7_bin_4bytes(const uchar *a, const uchar *b) > +{ > + uint32 an= mi_uint4korr(a); > + uint32 bn= mi_uint4korr(b); > + return an == bn ? 0 : an < bn ? -1 : +1; > +} > + > + > +/* > + Compare the leading four 7bit ASCII bytes in two strings in binary style. > +*/ > +static inline int > +my_strcoll_mb7_bin_8bytes(const uchar *a, const uchar *b) > +{ > + ulonglong an= mi_uint8korr(a); > + ulonglong bn= mi_uint8korr(b); > + return an == bn ? 0 : an < bn ? -1 : +1; > +} > + > +#endif /* CTYPE_ASCII_H_INCLUDED */ > diff --git a/strings/strcoll.ic b/strings/strcoll.ic > index 6aca0d0c460..72e0131e1c5 100644 > --- a/strings/strcoll.ic > +++ b/strings/strcoll.ic > @@ -39,6 +41,42 @@ > #endif > > > +/* > + For binary collations: > + - on 32bit platforms perform only 4 byte optimization > + - on 64bit platforms perform both 4 byte and 8 byte optimization > +*/ > +#if defined(STRCOLL_MB7_BIN) > +#define MY_STRCOLL_MB7_4BYTES(a,b) my_strcoll_mb7_bin_4bytes((a),(b)) > +#if defined(__x86_64__) as Wlad has already said, what about not gcc-on-intel 64bit platforms? > +#define STRCOLL_MB7_8BYTES > +#define MY_STRCOLL_MB7_8BYTES(a,b) my_strcoll_mb7_bin_8bytes((a),(b)) > +#endif > +#endif > + > + > +/* > + For case insensitive collations with trivial mapping from [a-z] to [A-Z] > + perform optimization only on 64 bit platforms. > + There is no sense to perform my_ascii_to_upper_magic_uint64() based > + optimization on 32bit platforms. The idea of this optimization > + is that it handles 8bytes at a time, using a 64bit CPU register. > + Enabling this optimization on 32bit platform may only slow things down. > +*/ > +#if defined(STRCOLL_MB7_TOUPPER) > +#if defined(__x86_64__) > +#define MY_STRCOLL_MB7_4BYTES(a,b) my_strcoll_ascii_toupper_4bytes((a),(b)) > +#define MY_STRCOLL_MB7_8BYTES(a,b) my_strcoll_ascii_toupper_8bytes((a),(b)) > +#endif /* Architecture test */ > +#endif /* STRCOLL_MB7_TOUPPER */ > + > + > +/* > + A helper macro to shift two pointers forward, to the given amount. > +*/ > +#define MY_STRING_SHIFT_PTR_PTR(a,b,len) do { a+= len; b+= len; } while(0) > + > + > /* > Weight of an illegal byte, must follow these rules: > 1. Must be greater than weight of any normal character in the collation. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] a00410a2ee4: MDEV-26238: Remove inconsistent behaviour of --default-* options in my_print_defaults
by Sergei Golubchik 11 Sep '21

11 Sep '21

Hi, Rucha! On Sep 11, Rucha Deodhar wrote: > revision-id: a00410a2ee4 (mariadb-10.6.1-77-ga00410a2ee4) > parent(s): 76149650764 > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2021-09-11 16:30:38 +0530 > message: > > MDEV-26238: Remove inconsistent behaviour of --default-* options > in my_print_defaults > > Analysis: --defaults* option is recognized anywhere in the commandline > instead of only at the beginning because handle_options() recognizes > options in any order. > Fix: use get_defaults_options() which recognizes --defaults* options only at > the beginning. After this is done, we only want to recognize other options > given in any order which can be done using handle_options(). So (only) skip > --defaults* options, combine it with program name to pass it to handle_options() > (because we skip program name in handle_options() too). Also, removed -e, -g and > -c because only my_print_defaults supports them. > > diff --git a/extra/my_print_defaults.c b/extra/my_print_defaults.c > index b7f52382721..0253614b410 100644 > --- a/extra/my_print_defaults.c > ++ b/extra/my_print_defaults.c > @@ -141,50 +124,49 @@ static int get_options(int *argc,char ***argv) > return 0; > } > > int main(int argc, char **argv) > { > int count, error, args_used; > char **load_default_groups= 0, *tmp_arguments[6]; > char **argument, **arguments, **org_argv, **new_argv, **pos; > int nargs, i= 0, index=1; > MY_INIT(argv[0]); > > org_argv= argv; > args_used= get_defaults_options(argv); > > /* Copy defaults-xxx arguments & program name */ > count= args_used; > arguments= tmp_arguments; > memcpy((char*) arguments, (char*) org_argv, count*sizeof(*org_argv)); > arguments[count]= 0; you're still overdoing it. you can do just argv+= args_used-1; argc+= args_used-1; get_options(&argc,&argv); no need to allocate or copy anything. Isn't it simpler? > /* > We already process --defaults* options at the beginning in > get_defaults_options(). So skip only --defaults* options and > pass program name and remaining options to handle_options(). > */ > > new_argv= (char**)malloc(argc*sizeof(*org_argv)); > memcpy((char*)new_argv, (char*)org_argv, 1*(sizeof(*new_argv))); > org_argv++; > > /* We want to skip --defaults* option only */ > if (!is_prefix(*org_argv, "--no-defaults")) > org_argv+= args_used-1; > > for (pos= org_argv; *pos; pos++) > new_argv[index++]= *(pos); > > new_argv[index]= 0; > argc= index; > > memcpy((char*)argv, (char*)new_argv, index*(sizeof(*new_argv))); > > /* Check out the args. */ > if (get_options(&argc,&argv)) > cleanup_and_exit(1); > > nargs= argc + 1; > if (opt_mysqld) > nargs+= array_elements(mysqld_groups); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] eb121bbe93e: Vanilla cleanups and refactorings
by Sergei Golubchik 11 Sep '21

11 Sep '21

Hi, Aleksey! On Sep 10, Aleksey Midenkov wrote: > revision-id: eb121bbe93e (mariadb-10.6.1-78-geb121bbe93e) > parent(s): d90f54e2206 > author: Aleksey Midenkov > committer: Aleksey Midenkov > timestamp: 2021-09-10 17:42:10 +0300 > message: > > diff --git a/dbug/tests.c b/dbug/tests.c > index 3e77bf82236..100388891eb 100644 > --- a/dbug/tests.c > +++ b/dbug/tests.c > @@ -13,6 +13,17 @@ char *push1=0; > #include <my_pthread.h> > #include <string.h> > > + > +#ifndef DBUG_OFF > +#define DBUG_EVALUATE(keyword,a1,a2) \ > + (_db_keyword_(0,(keyword), 0) ? (a1) : (a2)) > +#define DBUG_EVALUATE_IF(keyword,a1,a2) \ > + (_db_keyword_(0,(keyword), 1) ? (a1) : (a2)) > +#else > +#define DBUG_EVALUATE(keyword,a1,a2) (a2) > +#define DBUG_EVALUATE_IF(keyword,a1,a2) (a2) > +#endif > + could you fix the test properly, please? > const char *func3() > { > DBUG_ENTER("func3"); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3

Re: [Maria-developers] 0e09bc41cba: MDEV-9245 password "reuse prevention" validation plugin
by Sergei Golubchik 11 Sep '21

11 Sep '21

Hi, Oleksandr! Looks very good. See comments/questions below. The main thing - please add some tests for SQL errors in the plugin. On Sep 11, Oleksandr Byelkin wrote: > revision-id: 0e09bc41cba (mariadb-10.6.1-78-g0e09bc41cba) > parent(s): 67cd5da8c34 > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2021-09-10 13:47:07 +0200 > message: > > MDEV-9245 password "reuse prevention" validation plugin > > diff --git a/mysql-test/suite/plugins/t/password_reuse_check.test b/mysql-test/suite/plugins/t/password_reuse_check.test > --- /dev/null > +++ b/mysql-test/suite/plugins/t/password_reuse_check.test > @@ -0,0 +1,58 @@ > +--source include/not_embedded.inc > + > +if (!$PASSWORD_REUSE_CHECK_SO) { > + skip No PASSWORD_REUSE_CHECK plugin; > +} > + > +install soname "password_reuse_check"; > + > +set @save_password_reuse_check_interval= @@password_reuse_check_interval; this is kind of pointless, if you uninstall the plugin anyway. it's not harmless, just useless. > + > +set global password_reuse_check_interval= 0; ... > diff --git a/plugin/password_reuse_check/CMakeLists.txt b/plugin/password_reuse_check/CMakeLists.txt > --- /dev/null > +++ b/plugin/password_reuse_check/CMakeLists.txt > @@ -0,0 +1,2 @@ > + > +MYSQL_ADD_PLUGIN(password_reuse_check password_reuse_check.c) did you try to compile it statically? > diff --git a/plugin/password_reuse_check/password_reuse_check.c b/plugin/password_reuse_check/password_reuse_check.c > --- /dev/null > +++ b/plugin/password_reuse_check/password_reuse_check.c ... > +static void bin_to_hex512(char *to, const unsigned char *str) > +{ > + const unsigned char *str_end= str + (512/8); > + for (; str != str_end; ++str) > + { > + *to++= digits[((unsigned char) *str) >> 4]; > + *to++= digits[((unsigned char) *str) & 0x0F]; > + } > +} > + > + > +/** > + Send SQL error as ER_UNKNOWN_ERROR for information > + > + @param mysql Connection handler > +*/ > + > +static void report_sql_error(MYSQL *mysql) > +{ > + my_printf_error(ER_UNKNOWN_ERROR, "password_reuse_check:[%d] %s", ME_WARNING, > + mysql_errno(mysql), mysql_error(mysql)); > +} No tests for that? You can, for example, drop the table, and create a non-insertable view with the same name. CREATE VIEW mysql.password_reuse_check_history AS SELECT 1; That should make the plugin to fail, right? > + > + > +/** > + Create the history of passwords table for this plugin. > + > + @param mysql Connection handler > + > + @retval 1 - Error > + @retval 0 - OK > +*/ Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 818407474d6: MDEV-22166 CONVERT PARTITION: move out partition into a table
by Sergei Golubchik 10 Sep '21

10 Sep '21

Hi, Aleksey! I presume that changes to the logging code is what Monty asked. So I'll only comment on a couple of minor issues: On Sep 10, Aleksey Midenkov wrote: > revision-id: 818407474d6 (mariadb-10.6.1-80-g818407474d6) > parent(s): bb4b95975f7 > author: Aleksey Midenkov > committer: Aleksey Midenkov > timestamp: 2021-09-10 18:47:31 +0300 > message: > > MDEV-22166 CONVERT PARTITION: move out partition into a table > > diff --git a/sql/sql_partition.cc b/sql/sql_partition.cc > index db371d23ba7..cbdb40472d8 100644 > --- a/sql/sql_partition.cc > +++ b/sql/sql_partition.cc > @@ -5371,8 +5367,12 @@ that are reorganised. > tab_part_info->is_auto_partitioned= FALSE; > } > } > - else if (alter_info->partition_flags & ALTER_PARTITION_DROP) > + else if ((alter_info->partition_flags & ALTER_PARTITION_DROP) | > + (alter_info->partition_flags & ALTER_PARTITION_CONVERT_OUT)) > { > + const char * const cmd= > + (alter_info->partition_flags & ALTER_PARTITION_CONVERT_OUT) ? > + "EXTRACT" : "DROP"; "CONVERT" > /* > Drop a partition from a range partition and list partitioning is > always safe and can be made more or less immediate. It is necessary > diff --git a/sql/sql_table.h b/sql/sql_table.h > index aacb6c99f15..a580c7d348d 100644 > --- a/sql/sql_table.h > +++ b/sql/sql_table.h > @@ -20,6 +20,17 @@ > #include <my_sys.h> // pthread_mutex_t > #include "m_string.h" // LEX_CUSTRING > > +inline bool suicide() > +{ > + DBUG_SUICIDE(); > + return false; > +} as I wrote earlier, this is wrong. DBUG_SUICIDE() must always be an expression. > + > +#define ERROR_INJECT_CRASH(code) \ > + (DBUG_IF(code) && suicide()) > +#define ERROR_INJECT_ERROR(code) \ > + (DBUG_IF(code) && (my_error(ER_UNKNOWN_ERROR, MYF(0)), 1)) > + > class Alter_info; > class Alter_table_ctx; > class Column_definition; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 2

Re: [Maria-developers] ff0ece70855: MDEV-26531: mtr shows some hunks failed
by Sergei Golubchik 10 Sep '21

10 Sep '21

Hi, Anel! On Sep 10, Anel Husakovic wrote: > revision-id: ff0ece70855 (mariadb-10.6.1-73-gff0ece70855) > parent(s): 1bc82aaf0a7 > author: Anel Husakovic > committer: Anel Husakovic > timestamp: 2021-09-03 21:15:58 +0200 > message: > > MDEV-26531: mtr shows some hunks failed > > Reviewed by: wlad(a)mariadb.com > > --- > mysql-test/mariadb-test-run.pl | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/mysql-test/mariadb-test-run.pl b/mysql-test/mariadb-test-run.pl > index 6dcd25a9c86..7e0282b89e4 100755 > --- a/mysql-test/mariadb-test-run.pl > +++ b/mysql-test/mariadb-test-run.pl > @@ -3278,11 +3278,15 @@ sub do_before_run_mysqltest($) > # don't rebuild a file if it's up to date > unless (-e $dest and -M $dest < -M $resfile > and -M $dest < -M $base_result) { > - run_system(@cmd); > + if(run_system(@cmd) != 0){ > + die("system(@cmd) : $?"); I'd rather see it done more perl-ish, like run_system(@cmd) or die "system(@cmd): $?" > + } > } > } else { > $cmd[-3] = $dest = $opt_tmpdir . '/' . basename($dest); > - run_system(@cmd); > + if(run_system(@cmd) != 0){ > + die("system(@cmd) : $?"); > + } > } > $tinfo->{result_file} = $dest; > } > @@ -4887,7 +4891,7 @@ sub report_failure_and_restart ($) { > > sub run_system(@) { > mtr_verbose("Running '$_[0]'"); > - my $ret= system(@_) >> 8; > + my $ret= system(@_); this seems wrong. `perldoc -f system` says The return value is the exit status of the program as returned by the "wait" call. To get the actual exit value, shift right by eight (see below). > return $ret; > } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 4645a1e1950: MDEV-26238: Remove inconsistent behaviour of --default-* options in my_print_defaults
by Sergei Golubchik 10 Sep '21

10 Sep '21

Hi, Rucha! On Sep 08, Rucha Deodhar wrote: > Hi, Sergei ! > Thanks for the review. > > > > diff --git a/extra/my_print_defaults.c b/extra/my_print_defaults.c > > > index b7f52382721..e2ec0b00923 100644 > > > --- a/extra/my_print_defaults.c > > > +++ b/extra/my_print_defaults.c > > > @@ -48,20 +48,6 @@ static struct my_option my_long_options[] = > > > > you forgot the "config-file", which is here, just about "debug" > > I am sorry, I don't understand. Oops. Sorry. I looked at my_print_defaults.c in the checked out branch and I saw: {"config-file", 'c', "Deprecated, please use --defaults-file instead. " "Name of config file to read; if no extension is given, default " "extension (e.g., .ini or .cnf) will be added", (char**) &config_file, (char**) &config_file, 0, GET_STR, REQUIRED_ARG, 0, 0, 0, 0, 0, 0}, #ifdef DBUG_OFF {"debug", '#', "This is a non-debug version. Catch this and exit", 0,0, 0, GET_DISABLED, OPT_ARG, 0, 0, 0, 0, 0, 0}, #else so I wrote that "config-file" should also be removed. But now I see that it's present in 10.2, but not in 10.4, it was already removed. So, ignore that my comment, you didn't forget config-file, it was removed in 10.4, I looked at the wrong branch. > > > {"debug", '#', "Output debug log", (char**) &default_dbug_option, > > > (char**) &default_dbug_option, 0, GET_STR, OPT_ARG, 0, 0, 0, 0, 0, 0}, > > > #endif > > > - {"defaults-file", 'c', > > > - "Read this file only, do not read global or per-user config " > > > - "files; should be the first option", > > > - (char**) &config_file, (char*) &config_file, 0, GET_STR, REQUIRED_ARG, > > > @@ -141,49 +124,44 @@ static int get_options(int *argc,char ***argv) > > > return 0; > > > } > > > > > > -static char *make_args(const char *s1, const char *s2) > > > -{ > > > - char *s= malloc(strlen(s1) + strlen(s2) + 1); > > > - strmov(strmov(s, s1), s2); > > > - return s; > > > -} > > > - > > > int main(int argc, char **argv) > > > { > > > - int count= 0, error, no_defaults= 0; > > > + int count, error, args_used; > > > char **load_default_groups= 0, *tmp_arguments[6]; > > > - char **argument, **arguments, **org_argv; > > > - int nargs, i= 0; > > > + char **argument, **arguments, **org_argv, **new_argv, **pos; > > > + int nargs, i= 0, index=0; > > > MY_INIT(argv[0]); > > > > > > org_argv= argv; > > > - if (*argv && !strcmp(*argv, "--no-defaults")) > > > - { > > > - argv++; > > > - ++count; > > > - no_defaults= 1; > > > - } > > > - /* Copy program name and --no-defaults if present*/ > > > + args_used= get_defaults_options(argv); > > > + > > > + /* Copy defaults-xxx arguments & program name */ > > > + count= args_used; > > > arguments= tmp_arguments; > > > - memcpy((char*) arguments, (char*) org_argv, (++count)*sizeof(*org_argv)); > > > + memcpy((char*) arguments, (char*) org_argv, count*sizeof(*org_argv)); > > > arguments[count]= 0; > > > > > > - /* Check out the args */ > > > - if (get_options(&argc,&argv)) > > > - cleanup_and_exit(1); > > > - > > > - if (!no_defaults) > > > + /* > > > + We already process --defaults* options at the beginning in > > > + get_defaults_options(). So, now we we make a new commandline which > > > + has all options except --defaults* options and pass it to handle_options() > > > + instead of original commandline. > > > + */ > > > + new_argv= (char**)malloc(argc*sizeof(*org_argv)); > > > + for (pos= org_argv; *pos; pos++) > > > { > > > + if (!is_prefix(*pos, "--defaults-extra-file=") && > > > + !is_prefix(*pos, "--defaults-file=") && > > > + !is_prefix(*pos, "--defaults-group-suffix=")) > > > + new_argv[index++]= *(pos); > > > > Why are you doing it? > > After get_defaults_options, you know how many arguments were default, > > just skip args_used, and invoke handle_options on the rest. > > I did it to filter all the --defaults* options, anywhere on the command > line. > > get_defaults_options() counts the program name in addition to --defaults* > options, so args_used will at least be 1. So skipping argv using agrs_used > will at least skip the program name even before it is passed to > handle_options(). But argv is also skipped in handle_options() to skip the > program name. > So one extra argument will get skipped too. correct, you need to skip args_used-1, good point. > Say --defaults* options are somewhere at the end, they will not get counted > in get_defaults_options(). So --defaults* will be passed to > handle_options() without getting skipped. And handle_options() will give an > error even if the options are valid because > it will not find --defaults* options in my_long_options[]. This is what we want, right? That's why you're doing it. Try `mariadb` client or `mariadb-dump` or `mariadbd` server, anything, and put --defaults* option not at the beginning. It'll be "unknown option". The idea is to make my_print_defaults *not special*, to behave like any other executable. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0