- developers - lists.mariadb.org

Re: [Maria-developers] 9e1c15355a9: resolve the possible deadlock issue through intern_plugin_lock
by Sergei Golubchik 08 Sep '21

08 Sep '21

Hi, Nikita! Looks good so far but I noticed too late that it's just one patch on top of others, not a complete thing. See few comments below. Shall I review everything? Is it feature complete except that it skips inactive threads? On Sep 07, Nikita Malyavin wrote: > revision-id: 9e1c15355a9 (mariadb-10.6.3-44-g9e1c15355a9) > parent(s): d7c36600144 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2021-09-07 18:37:51 +0300 > message: > > resolve the possible deadlock issue through intern_plugin_lock > > diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc > index be043bf229a..ecd885ed296 100644 > --- a/sql/sql_plugin.cc > +++ b/sql/sql_plugin.cc > @@ -1053,6 +1053,15 @@ plugin_ref plugin_lock(THD *thd, plugin_ref ptr) > } > this needs a function comment. the function is quite specialized and very different from normal plugin lock functions. it requires a THD (normally it's optional), it does not return a locked plugin like the others (because, exactly, it's stored in the THD). because it's not what people usually expect from a plugin_lock* function, the comment should describe all these differences. > +void plugin_lock_by_var_if_different(THD *thd, sys_var *var, sys_var *var_prev) > +{ > + sys_var_pluginvar *pv= var->cast_pluginvar(); > + sys_var_pluginvar *pv_prev= var_prev ? var_prev->cast_pluginvar() : NULL; > + if (pv && (!pv_prev || pv->plugin != pv_prev->plugin)) > + intern_plugin_lock(thd->lex, plugin_int_to_ref(pv->plugin)); > +} > + > + > /* > Notes on lifetime: > > diff --git a/storage/perfschema/pfs_variable.cc b/storage/perfschema/pfs_variable.cc > index 984b68e699b..85656532c72 100644 > --- a/storage/perfschema/pfs_variable.cc > +++ b/storage/perfschema/pfs_variable.cc > @@ -100,6 +102,15 @@ bool PFS_system_variable_cache::init_show_var_array(enum_var_type scope, bool st > > mysql_prlock_unlock(&LOCK_system_variables_hash); > > + for (uint i= 0; i < m_show_var_array.elements(); i++) why separately, not in the loop above? > + { > + sys_var *var= (sys_var *)m_show_var_array.at(i).value; > + if (!var) continue; can var be NULL? > + sys_var *prev= i == 0 ? NULL : (sys_var *)m_show_var_array.at(i-1).value; hmm, normally this is done with sys_var *prev= NULL; before the loop and prev= var; in the loop, after plugin_lock_by_var_if_different(). > + > + plugin_lock_by_var_if_different(m_current_thd, var, prev); > + } > + > /* Increase cache size if necessary. */ > m_cache.reserve(m_show_var_array.elements()); > > @@ -316,7 +327,27 @@ class PFS_system_variable_cache_apc: public Apc_target::Apc_call > > void call_in_target_thread() override > { > - (m_pfs->*m_func)(m_param); > + call(m_pfs, m_func, m_param); > + } > +public: > + static void call(PFS_system_variable_cache *pfs, Request func, uint param) > + { > + THD *safe_thd= pfs->safe_thd(); > + > + if (pfs->query_scope() == OPT_SESSION) what's the point doing apc call if the query_scope() is not OPT_SESSION? It seems like here you need a DBUG_ASSERT, and the if() should be much earlier up the stack. > + { > + mysql_mutex_lock(&LOCK_global_system_variables); > + if (!safe_thd->variables.dynamic_variables_ptr || > + global_system_variables.dynamic_variables_head > > + safe_thd->variables.dynamic_variables_head) > + { > + mysql_prlock_rdlock(&LOCK_system_variables_hash); > + sync_dynamic_session_variables(safe_thd, false); > + mysql_prlock_unlock(&LOCK_system_variables_hash); > + } > + mysql_mutex_unlock(&LOCK_global_system_variables); > + } > + (pfs->*func)(param); > } > }; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 2

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

07 Sep '21

Hi, Rucha! On Sep 07, Rucha Deodhar wrote: > revision-id: 4645a1e1950 (mariadb-10.6.1-72-g4645a1e1950) > parent(s): 21ce69123c1 > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2021-09-02 22:10:37 +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(). But > handle_options() returns error and exits if an option is not found. > We don't want to exit and give error because --defaults* options are still > valid. So make a new commandline which doesn't have --defaults* > option and pass it to handle_options(). 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..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" > {"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, > - 0, 0, 0, 0, 0, 0}, > - {"defaults-extra-file", 'e', > - "Read this file after the global config file and before the config " > - "file in the users home directory; should be the first option", > - (void *)&my_defaults_extra_file, (void *)&my_defaults_extra_file, 0, > - GET_STR, REQUIRED_ARG, 0, 0, 0, 0, 0, 0}, > - {"defaults-group-suffix", 'g', > - "In addition to the given groups, read also groups with this suffix", > - (char**) &my_defaults_group_suffix, (char**) &my_defaults_group_suffix, > - 0, GET_STR, REQUIRED_ARG, 0, 0, 0, 0, 0, 0}, > {"mysqld", 0, "Read the same set of groups that the mysqld binary does.", > &opt_mysqld, &opt_mysqld, 0, GET_BOOL, NO_ARG, 0, 0, 0, 0, 0, 0}, > {"no-defaults", 'n', "Return an empty string (useful for scripts).", > @@ -109,9 +95,6 @@ get_one_option(const struct my_option *opt __attribute__((unused)), > const char *filename __attribute__((unused))) > { > switch (opt->id) { > - case 'c': > - opt_defaults_file_used= 1; > - break; > case 'n': > cleanup_and_exit(0); > case 'I': > @@ -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. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 271afbc88e4: Pre requiste MDEV-9245: add host to validate password interface
by Sergei Golubchik 07 Sep '21

07 Sep '21

Hi, Oleksandr! this is ok! On Sep 07, Oleksandr Byelkin wrote: > revision-id: 271afbc88e4 (mariadb-10.6.1-78-g271afbc88e4) > parent(s): 47207ea291a > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2021-09-07 11:52:28 +0200 > message: > > Pre requiste MDEV-9245: add host to validate password interface > > Hostname added to the interface. > > Cracklib plugin fixed according to the new interface. > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] b48dfdedd59: MDEV-26307 multi-source-replication support mysql syntax(for channel)
by Sergei Golubchik 02 Sep '21

02 Sep '21

Hi, Michael! On Sep 02, Michael Widenius wrote: > revision-id: b48dfdedd59 (mariadb-10.6.1-64-gb48dfdedd59) > parent(s): 5f0b6b017a8 > author: Michael Widenius > committer: Michael Widenius > timestamp: 2021-08-24 11:22:29 +0300 > message: > > MDEV-26307 multi-source-replication support mysql syntax(for channel) > > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index 0c1c68cd644..704812e5dd8 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -2307,6 +2309,34 @@ connection_name: > } > ; > > +optional_for_channel: > + /* empty */ > + { > + /*do nothing */ > + } > + | for_channel > + > + ; > + > +for_channel: > + FOR_SYM CHANNEL_SYM TEXT_STRING_sys > + { > + if (Lex->mi.connection_name.str != NULL) > + { > + my_yyabort_error((ER_WRONG_ARGUMENTS, MYF(0), "CONNECTION_NAME AND FOR CHANNEL CAN NOT BE SPECIFIED AT THE SAME TIME)")); No, please, no LONG ENGLISH MESSAGES IN UPPERCASE WITH STRAY PARENTHESYS) embedded as a part of the error message. In your tests it looks like ERROR HY000: Incorrect arguments to CONNECTION_NAME AND FOR CHANNEL CAN NOT BE SPECIFIED AT THE SAME TIME) or, for example, ERROR HY000: Felaktiga argument till CONNECTION_NAME AND FOR CHANNEL CAN NOT BE SPECIFIED AT THE SAME TIME) The correct approach is to have the parser ensure the syntax is correct. Meaning, it should be START_SYM SLAVE optional_connection_name slave_thread_opts | START_SYM SLAVE slave_thread_opts optional_for_channel the incorrect but still kind of acceptable approach is to fake this and check in the code, like you did, but use my_parse_error, not ER_WRONG_ARGUMENTS > + } > + else > + { > + Lex->mi.connection_name= $3; > +#ifdef HAVE_REPLICATION > + if (unlikely(check_master_connection_name(&$3))) > + my_yyabort_error((ER_WRONG_ARGUMENTS, MYF(0), "MASTER_CONNECTION_NAME")); > +#endif > + } > + > + } > + ; > + > /* create a table */ > > create: > @@ -8030,7 +8060,7 @@ opt_to: > ; > > slave: > - START_SYM SLAVE optional_connection_name slave_thread_opts > + START_SYM SLAVE optional_connection_name slave_thread_opts optional_for_channel > { > LEX *lex=Lex; > lex->sql_command = SQLCOM_SLAVE_START; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] dc2ace70f1b: Syntax with MIGRATE keyword
by Sergei Golubchik 01 Sep '21

01 Sep '21

Hi, Aleksey! On Sep 01, Aleksey Midenkov wrote: > revision-id: dc2ace70f1b (mariadb-10.6.1-68-gdc2ace70f1b) > parent(s): 19fbfab084f > author: Aleksey Midenkov > committer: Aleksey Midenkov > timestamp: 2021-08-30 22:37:08 +0300 > message: > > Syntax with MIGRATE keyword > > ALTER TABLE tbl_name > [alter_option [, alter_option] ...] | > [partition_options] > > partition_option: { > ... > | MIGRATE [OUT] PARTITION partition_name TO [TABLE] tbl_name > } > > Examples: > > ALTER TABLE t1 MIGRATE PARTITION p1 TO tp1; > ALTER TABLE t1 MIGRATE PARTITION p2 TO TABLE tp2; > ALTER TABLE t1 MIGRATE OUT PARTITION p3 TO TABLE tp3; > > diff --git a/mysql-test/suite/parts/r/alter_table.result b/mysql-test/suite/parts/r/alter_table.result > index bc1bf661d4c..7a2d45c51c1 100644 > --- a/mysql-test/suite/parts/r/alter_table.result > +++ b/mysql-test/suite/parts/r/alter_table.result > @@ -258,9 +258,46 @@ show grants for current_user; > Grants for alan@% > GRANT USAGE ON *.* TO `alan`@`%` > GRANT INSERT, CREATE, DROP, ALTER ON `test`.* TO `alan`@`%` > -alter table t1 extract partition p1 as table tp1; > +alter table t1 migrate partition p1 to table tp1; > disconnect alan; > connection default; > drop database EXISTENT; > drop user alan; > drop tables t1, tp1, tp2, tp4; > +# Cunning syntax what's so cunning here? > +create or replace table t1 (x int) > +partition by range(x) ( > diff --git a/sql/handler.h b/sql/handler.h > index c455625aafe..7ac9e929ecc 100644 > --- a/sql/handler.h > +++ b/sql/handler.h > @@ -824,7 +824,7 @@ typedef bool Log_func(THD*, TABLE*, bool, const uchar*, const uchar*); > #define ALTER_PARTITION_TRUNCATE (1ULL << 11) > // Set for REORGANIZE PARTITION > #define ALTER_PARTITION_TABLE_REORG (1ULL << 12) > -#define ALTER_PARTITION_EXTRACT (1ULL << 13) > +#define ALTER_PARTITION_MIGRATE_OUT (1ULL << 13) > > /* > This is master database for most of system tables. However there > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index 49dd244f2ff..e1be6ea9557 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -7492,6 +7492,16 @@ ident_or_empty: > | ident > ; > > +move_out_partition: > + MIGRATE_SYM PARTITION_SYM > + | MIGRATE_SYM OUT_SYM PARTITION_SYM Where does this OUT_SYM came out from? It doesn't add any values, it doesn't make the syntax more natural. Why did you add it? > + ; > + > +to_table: > + TO_SYM TABLE_SYM > + | TO_SYM Let's keep the TABLE_SYM. Saying MIGRATE PARTITION p1 TO p2 is very vague, the syntax should explicitly say that it migrates a PARTITION to a TABLE. The statement mentions many different types of objects, if should be clear about what object type every identifier refers to. > + ; > + > alter_commands: > /* empty */ > | DISCARD TABLESPACE > @@ -7619,15 +7629,15 @@ alter_commands: > MYSQL_YYABORT; > Lex->alter_info.partition_flags|= ALTER_PARTITION_EXCHANGE; > } > - | EXTRACT_SYM PARTITION_SYM alt_part_name_item > - AS TABLE_SYM table_ident have_partitioning > + | move_out_partition alt_part_name_item > + to_table table_ident have_partitioning please, let it be just MIGRATE_SYM PARTITION_SYM alt_part_name_item TO_SYM TABLE_SYM table_ident have_partitioning > { > - if (Lex->stmt_alter_table($6)) > + if (Lex->stmt_alter_table($4)) > MYSQL_YYABORT; > Lex->m_sql_cmd= new (thd->mem_root) Sql_cmd_alter_table(); > if (unlikely(Lex->m_sql_cmd == NULL)) > MYSQL_YYABORT; > - Lex->alter_info.partition_flags|= ALTER_PARTITION_EXTRACT; > + Lex->alter_info.partition_flags|= ALTER_PARTITION_MIGRATE_OUT; > } > ; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 5

Re: [Maria-developers] 19fbfab084f: Compilation fix
by Sergei Golubchik 01 Sep '21

01 Sep '21

Hi, Aleksey! On Sep 01, Aleksey Midenkov wrote: > revision-id: 19fbfab084f (mariadb-10.6.1-67-g19fbfab084f) > parent(s): 8009680fbc3 > author: Aleksey Midenkov > committer: Aleksey Midenkov > timestamp: 2021-08-30 22:02:40 +0300 > message: > > Compilation fix > > diff --git a/sql/sql_table.h b/sql/sql_table.h > index 83e88c82f0e..8cb9ebe0445 100644 > --- a/sql/sql_table.h > +++ b/sql/sql_table.h > @@ -20,8 +20,14 @@ > #include <my_sys.h> // pthread_mutex_t > #include "m_string.h" // LEX_CUSTRING > > +inline bool suicide() > +{ > + DBUG_SUICIDE(); > + return false; > +} > + > #define ERROR_INJECT_CRASH(code) \ > - (DBUG_IF(code) && (DBUG_SUICIDE(), 0)) > + (DBUG_IF(code) && suicide()) What did that change? A compiler didn't like (void,0) ? Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 2

Re: [Maria-developers] 87953156c76: Vanilla cleanup: DBUG_EVALUATE_IF() macro replaced by shorter form DBUG_IF()
by Sergei Golubchik 01 Sep '21

01 Sep '21

Hi, Aleksey! On Sep 01, Aleksey Midenkov wrote: > revision-id: 87953156c76 (mariadb-10.6.1-61-g87953156c76) > parent(s): b67fe0c009d > author: Aleksey Midenkov > committer: Aleksey Midenkov > timestamp: 2021-08-30 21:24:35 +0300 > message: > > Vanilla cleanup: DBUG_EVALUATE_IF() macro replaced by shorter form DBUG_IF() Thanks, I agree, this looks much better > diff --git a/dbug/user.r b/dbug/user.r > index 8d8a9ce6910..3740a60851c 100644 > --- a/dbug/user.r > +++ b/dbug/user.r > @@ -785,18 +785,20 @@ EX:\fC > +.B 0 > +Like DBUG_EXECUTE_IF this could be used to conditionally execute > "dangerous" actions. > .SP 1 > EX:\fC > .br > if (prepare_transaction () || > .br > - DBUG_EVALUATE ("crashme", (DBUG_ABORT(), 0), 0) || > + (DBUG_IF("crashme") && (DBUG_ABORT(), 0)) || oh, a bug in the manual! good catch. > .br > commit_transaction () )\fR > .SP 1 > diff --git a/include/my_dbug.h b/include/my_dbug.h > index e25bfcf28a7..2afcb010e7c 100644 > --- a/include/my_dbug.h > +++ b/include/my_dbug.h > @@ -102,8 +102,7 @@ extern int (*dbug_sanity)(void); > do {if (_db_keyword_(0, (keyword), 1)) { a1 }} while(0) > #define DBUG_EVALUATE(keyword,a1,a2) \ > (_db_keyword_(0,(keyword), 0) ? (a1) : (a2)) you've kept DBUG_EVALUATE, although it was only added for consistency together with DBUG_EVALUATE_IF. And DBUG_EVALUATE is not used anywhere, I've just grepped. Remove it too? > -#define DBUG_EVALUATE_IF(keyword,a1,a2) \ > - (_db_keyword_(0,(keyword), 1) ? (a1) : (a2)) > +#define DBUG_IF(keyword) _db_keyword_(0, (keyword), 1) > #define DBUG_PRINT(keyword,arglist) \ > do if (_db_pargs_(__LINE__,keyword)) _db_doprnt_ arglist; while(0) > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] f14cfd22115: Vanilla refactoring: binlog query in DDL_LOG_RENAME_ACTION
by Aleksey Midenkov 24 Aug '21

24 Aug '21

Hi Sergei! Really, this is not pure refactoring. I've been planning to remove it if it will not be useful for MDEV-22165 (that task should be based on this branch). Meanwhile, can you please review other commits? On Mon, Aug 23, 2021 at 7:35 PM Sergei Golubchik <serg(a)mariadb.com> wrote: > > Hi, Aleksey! > > On Aug 23, Aleksey Midenkov wrote: > > revision-id: f14cfd22115 (mariadb-10.6.1-63-gf14cfd22115) > > parent(s): 03225474d60 > > author: Aleksey Midenkov > > committer: Aleksey Midenkov > > timestamp: 2021-08-17 20:01:48 +0300 > > message: > > > > Vanilla refactoring: binlog query in DDL_LOG_RENAME_ACTION > > > > This turned out to be not needed for MDEV-22166. But it still might be > > useful for MDEV-22165 or later. > > There are three changes here. > Adding 'locked' to ddl_log_store_query, creating > st_ddl_recovery::binlog_query(), adding 'case RENAME_PHASE_BINLOG' > > st_ddl_recovery::binlog_query() is ok, if you want to keep it. > But the other two aren't a refactoring, it's creating new functionality > which isn't used in this patch, dead code basically. And your commit > comment seems to imply that this new functionality isn't needed > in the following feature patch either. > > While your other refactoring patches are indeed pure refactoring, this > one isn't, so, please, remove it from the patchset if it isn't used > there. You can keep st_ddl_recovery::binlog_query() if you'd like - it > is indeed pure refactoring. > > Regards, > Sergei > VP of MariaDB Server Engineering > and security(a)mariadb.org -- All the best, Aleksey Midenkov @midenok

2 1

Re: [Maria-developers] bc55e3c438f: MDEV-4742 - provide function to sort string in "natural" order
by Sergei Golubchik 20 Aug '21

20 Aug '21

Hi, Vladislav! On Aug 20, Vladislav Vaintroub wrote: > commit bc55e3c438f > Author: Vladislav Vaintroub <wlad(a)mariadb.com> > Date: Thu Jul 29 13:28:11 2021 +0200 > > MDEV-4742 - provide function to sort string in "natural" order > > The numbers should be compared as numbers, while the rest should be compared > as string. > > Introduce natural_sort_key() function that transforms original string > so that the lexicographic order of such keys is suitable for > natural sort. This comment doesn't mention the second optional argument. Neither does Jira. > diff --git a/mysql-test/main/natural_sort_key.test b/mysql-test/main/natural_sort_key.test > new file mode 100644 > index 00000000000..bee0d8e76bd > --- /dev/null > +++ b/mysql-test/main/natural_sort_key.test > @@ -0,0 +1,64 @@ > +--source include/have_innodb.inc > +--source include/have_sequence.inc > + > +SET NAMES utf8mb4; > +SELECT NATURAL_SORT_KEY(NULL); > + > +# Sort without tables > +SELECT '' c WHERE 0 UNION VALUES('a10'),('a9'),('a1000'), ('a0'),('b'),('b0') ORDER BY NATURAL_SORT_KEY(c); > + > +#Test that max packet overflow produces NULL plus warning > +SELECT NATURAL_SORT_KEY(repeat('a1',@@max_allowed_packet/2-1)); > + > +#Test with virtual(index only) key > +CREATE TABLE t1( > + c VARCHAR(30) CHARACTER SET latin1 COLLATE latin1_bin, > + k VARCHAR(45) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci AS (NATURAL_SORT_KEY(CONVERT(c USING utf8mb4))) INVISIBLE, > + KEY(k,c)) ENGINE=InnoDB; > +INSERT INTO t1 values > + ('A1'),('a1'),('A100'),('a100'),('A2'),('ä2'),('a2'),('A99'), > + ('äb'),('B1'),('B100'),('B9'),('C'),('100'); > +EXPLAIN SELECT c FROM t1 ORDER BY k,c; > +-- echo #Natural sort order. > +# We sort by 2 colums, for stable sort,as we do not currenly have a case and accent insensitive Unicode collation. > +SELECT c FROM t1 ORDER BY k,c; > +-- echo #Unnatural but unicode aware) sort order > +SELECT c FROM t1 ORDER BY CONVERT(c USING utf8mb4) COLLATE utf8mb4_unicode_ci,c; > +# CREATE TABLE AS SELECT, to see that length of the column is correct. > +CREATE TABLE t2 AS SELECT c, NATURAL_SORT_KEY(c) FROM t1 WHERE 0; you could've simply used --enable_metadata > +SHOW CREATE TABLE t2; > +DROP TABLE t1,t2; > + > +#Show encoding of numbers, including fractions, and leading whitespace. > +SELECT RPAD(val,21,' ') value , RPAD(NATURAL_SORT_KEY(val),26,' ') sortkey , LENGTH(NATURAL_SORT_KEY(val)) - LENGTH(val) encoding_overhead > +FROM > +( > +SELECT 0 val > +UNION VALUES ('0.0'),('0.1'), ('0,15'),('1.001'),('1.002'),('1.010'),('1.02'),('1.1'),('1.3'),('1'),('01'),('0001') > +UNION SELECT CONCAT('1',repeat('0',seq)) FROM seq_1_to_20 > +) AS numbers ORDER BY sortkey; > + > +--echo # Disable fractions handling by passing NULL as second parameter to NATURAL_SORT_KEY > +SELECT val value > +FROM > +( > +SELECT 0 val > +UNION VALUES ('0.1'),('1.001'),('1.002'),('1.010'),('1.02'),('1.1'),('1.3'),('1'), ('0,1'),('1,001'),('1,002'),('1,010'),('1,02'),('1,1'),('1,3'),('1')) > +AS numbers ORDER BY NATURAL_SORT_KEY(val, NULL); > + > +--echo # Use ',' as decimal separator for NATURAL_SORT_KEY > +SELECT val value, NATURAL_SORT_KEY(val,',') sortkey > +FROM > +( > +SELECT 0 val > +UNION VALUES ('0,1'),('1,001'),('1,002'),('1,010'),('1,02'),('1,1'),('1,3'),('1')) > +AS numbers ORDER BY sortkey; > + > +--echo # Use '.' as decimal separator for NATURAL_SORT_KEY > +SELECT val value,NATURAL_SORT_KEY(val,'.') sortkey > +FROM > +( > +SELECT 0 val > +UNION VALUES ('0.1'),('1.001'),('1.002'),('1.010'),('1.02'),('1.1'),('1.3'),('1')) > +AS numbers ORDER BY sortkey; > +SET NAMES DEFAULT; > diff --git a/sql/item_strfunc.cc b/sql/item_strfunc.cc > index 7e8ff667e75..a2efd414750 100644 > --- a/sql/item_strfunc.cc > +++ b/sql/item_strfunc.cc > @@ -5293,6 +5293,232 @@ String *Item_temptable_rowid::val_str(String *str) > return &str_value; > } > > +/** > + Here is how we generate a key suitable for lexicographic comparison > + from a numeric string (representing positive number). > + > + We prepend variable length prefix, that only encodes string length > + The longer the string, the lexicographically larger is the prefix. > + > + Rules for generating prefix is following. > + > + 1. Small length (1 to 9) > + Prefix is single char '0' + length -1 > + This gives the range '0' - '8' > + 2. Medium length(10 to 18) > + Prefix is 2 chars - concat('9', '0'+length-10)' > + This gives range '90'-'98' > + 3. Large length(19) > + Prefix is "990" > + 4. Huge length (20 to 2^32-1) > + Prefix stars with '99', then log10 of the length is added as char > + then the number itself is added > + The range is '99120' - '9994294967295' please also explain here how you handle leading zeros > +*/ > + > +#define NATSORT_BUFSIZE 16 > +static size_t natsort_num2str(size_t n, char *s) > +{ > + if (n < 10) > + { > + s[0]= '0' + (char)n -1; > + return 1; > + } > + > + if (n < 19) > + { > + s[0]= '9'; > + s[1]= '0'+ (char)n- 10; > + return 2; > + } > + > + if (n == 19) > + { > + s[0]= '9'; > + s[1]= '9'; > + s[2]= '0'; > + return 3; > + } > + > + /* Here, n > 19*/ > + s[0]= '9'; > + s[1]= '9'; > + size_t log10n= 0; > + for (size_t tmp= n/10; tmp; tmp /= 10) > + log10n++; > + DBUG_ASSERT(log10n && log10n < 10); first, I don't see where the caller ensures that log10n < 10 (in other words, that n < 9999999999) second, please, never write assert(a && b), always do two separate asserts, so that when it fires, it'd be clear what condition was false > + s[2]='0' + (char)log10n; > + return ll2str(n,s+3,10,0) - s; > +} > + > +/** > + Convert a string to natural sort key. > + @param[in] in - input string > + @param[out] s - output string > + > + We assume that memory is preallocated for the output > + string, so that appends do not fail. > + > + The lead zeros count, if positive, is stored after > + the numeric string. > + > + Fractional parts are stored and compared as strings. > + Even if they have numbers in them, they should just > + be compared as strings. > +*/ > +static void to_natsort_key(const String *in, String *s, my_wc_t decimal_sep) > +{ > + CHARSET_INFO *cs= in->charset(); > + uchar *pos= (uchar *) in->ptr(); > + uchar *end= pos + in->length(); > + size_t num_len= 0; > + size_t leading_zeros= 0; > + uchar *num_start= nullptr; > + bool in_fraction= false; > + size_t fraction_len=0; > + > + for (;;) > + { > + my_wc_t c= 0; > + int charlen= cs->mb_wc(&c, pos, end); > + bool is_digit= charlen >= 1 && (c >= '0' && c <= '9'); what do you mean `>= 1` ? how can it be `> 1` and still satisfy the secon part of the condition? > + > + if (!is_digit && (num_start || leading_zeros)) > + { > + /* > + Handle end of digits run. > + > + Write prefix, natsort_num2str(length) is without leading zeros > + Write numeric value, without leading zeros > + If there were leading zeros, write variable length suffix > + */ > + char buf[NATSORT_BUFSIZE]; > + if (!num_len) > + { > + // number was zeros only > + leading_zeros--; > + num_len= 1; > + } > + > + s->append(buf, natsort_num2str(num_len, buf)); > + if (!num_start) > + s->append('0'); > + else > + s->append((const char *) num_start, pos - num_start, cs); > + > + if (leading_zeros) > + s->append(buf, natsort_num2str(leading_zeros, buf)); hmm. How would it sort "1;", "001;", "1a" ? > + > + /* Reset state.*/ > + num_len= 0; > + num_start= nullptr; > + leading_zeros= 0; > + in_fraction= c == decimal_sep; I don't really like this idea. First, see https://natsort.readthedocs.io/en/master/index.html there are many ways one can potentially want to customize natsort. (of course, none of them have to be in this MDEV) One of those customizations, indeed, is treating floating point numbers as floating point numbers. But comma vs dot is locale thing, and a one string argument it's not how you should tell the function what decimal separator the current locale uses. So. Simple approach - remove the second argument, no floating point support. Complex approach - have a second argument that enables floating point support, but that can be used in the future to specify more customizations, not that is hard-wired to specify decimal separator duplicating and bypassing the locale. > + fraction_len= 0; > + } > + > + if (charlen < 1) > + break; > + > + if (is_digit && !in_fraction) > + { > + /* Do not count leading zeros in num_len, and num_pos */ > + if (c != '0' || num_len) > + { > + num_len++; > + if (!num_start) > + num_start= pos; > + } > + else > + leading_zeros++; > + } > + else > + { > + /* Append a non-digit, or fractional part of the number (handling the same as string) */ > + s->append((const char *) pos, charlen); > + if (in_fraction) > + { > + if (!is_digit && fraction_len) > + in_fraction= false; > + fraction_len++; > + } > + } > + pos+= charlen; > + } > +} > + > + > +String *Item_func_natural_sort_key::val_str(String *s) > +{ > + if (args[0]->is_null()) > + { > + null_value= true; > + return nullptr; > + } > + > + String *in= args[0]->val_str(); > + CHARSET_INFO *cs= in->charset(); > + ulong max_allowed_packet= current_thd->variables.max_allowed_packet; > + size_t reserve_length= (size_t) in->length() + in->length() / 2 + 2; > + if (s->alloc((uint32)reserve_length)) > + goto error_exit; > + s->length(0); > + s->set_charset(cs); > + > + to_natsort_key(in, s, decimal_separator()); > + > + DBUG_ASSERT(s->length() < reserve_length); > + > + if (s->length() > max_allowed_packet) better check this inside to_natsort_key(). max_allowed_packet is supposed to prevent the string from growing too large, instead of just letting it grow and erroring out afterwards. > + { > + push_warning_printf(current_thd, Sql_condition::WARN_LEVEL_WARN, > + ER_WARN_ALLOWED_PACKET_OVERFLOWED, > + ER(ER_WARN_ALLOWED_PACKET_OVERFLOWED), func_name(), > + max_allowed_packet); > + goto error_exit; > + } > + return s; > + > +error_exit: > + null_value=true; > + return nullptr; > +} > + > + > +my_wc_t Item_func_natural_sort_key::decimal_separator() > +{ > + if (m_decimal_separator != DECIMAL_SEP_UNDEFINED) > + return m_decimal_separator; > + > + DBUG_ASSERT(arg_count > 1); > + String *s= args[1]->val_str(); > + if (s && s->length()) > + { > + my_wc_t c; > + if (s->charset()->mb_wc(&c, (const uchar *) s->ptr(), > + (const uchar *) s->end())) > + return c; > + } > + return DECIMAL_SEP_NONE; > +} > + > +bool Item_func_natural_sort_key::fix_length_and_dec(void) > +{ > + if (agg_arg_charsets_for_string_result(collation, args, 1)) > + return true; > + DBUG_ASSERT(collation.collation != NULL); > + uint32 max_clen= args[0]->max_char_length(); > + fix_char_length(max_clen + max_clen/2 + ((max_clen <= 3)?1:0)); > + set_maybe_null(); better, only set maybe_null if the first argument maybe_null or the first argument maybe_too_long > + > + if (arg_count> 1 && args[1]->can_eval_in_optimize()) > + { > + /* Initialize decimal separator */ > + m_decimal_separator= decimal_separator(); > + } > + return false; > +} > + > #ifdef WITH_WSREP > > #include "wsrep_mysqld.h" Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] a19495728de: MDEV-16355 Add option for mysqldump to read data as of specific timestamp from system-versioned tables
by Sergei Golubchik 15 Aug '21

15 Aug '21

Hi, Aleksey! I'm reviewing it separately from other commits in the branch, because it looks to be functionally independent from, say, MDEV-16029 mysqldump: dump and restore historical data and doesn't have to wait for the latter to be pushed first. I think I was able to guess what the patch would look like, if you'd apply it to vanilla 10.7 without MDEV-16029. But just to be sure, please, let me look it over again after the cherry-pick. Otherwise no comments code-wise. A couple of comments related to naming, see below. On Aug 15, Aleksey Midenkov wrote: > revision-id: a19495728de (versioning-1.0.5-45-ga19495728de) > parent(s): be4605ae086 > author: Aleksey Midenkov > committer: Aleksey Midenkov > timestamp: 2018-07-19 14:47:35 +0300 > message: > > MDEV-16355 Add option for mysqldump to read data as of specific timestamp from system-versioned tables > > mysqldump changes: > > * --asof-timestamp option specifies historical point; "asof" isn't a word. I'd suggest --as-of-timestamp or simply --as-of > * with --dump-history it dumps all prior history, without it dumps only single point in time; > * query forging protection for --asof-timestamp parameter. > > diff --git a/client/mysqldump.c b/client/mysqldump.c > index eee12555271..f426e8c58ef 100644 > --- a/client/mysqldump.c > +++ b/client/mysqldump.c > @@ -1099,6 +1102,13 @@ static int get_options(int *argc, char ***argv) > my_progname_short); > return(EX_USAGE); > } > + if (opt_asof_timestamp && strchr(opt_asof_timestamp, '\'')) > + { > + fprintf(stderr, > + "%s: --asof-timestamp wrong argument!\n", > + my_progname_short); May be, use the wording that is closer to the error one would get from any other invalid character, like, if one would do --asof-timestamp='!@#$%' That is, instead of "wrong argument", something like fprintf(stderr, "%s: Incorrect DATETIME value: '%s'\n", opt_asof_timestamp, my_progname_short); Because MariaDB [test]> select timestamp'2010!@#$%'; ERROR 1525 (HY000): Incorrect DATETIME value: '2010!@#$%' > + return(EX_USAGE); > + } > if (strcmp(default_charset, charset_info->csname) && > !(charset_info= get_charset_by_csname(default_charset, > MY_CS_PRIMARY, MYF(MY_WME)))) Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0