- developers - lists.mariadb.org

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

Re: [Maria-developers] 6768e3a2830: MDEV-22166 MIGRATE PARTITION: move out partition into a table
by Sergei Golubchik 10 Sep '21

10 Sep '21

Hi, Aleksey! It's great that you made it atomic. Frankly, I didn't think of it, MDEV defines the feature as being exactly equivalent to the 3-command sequence, so non-atomic. But, of course, making it atomic makes perfect sense. See other comments below. On Aug 23, Aleksey Midenkov wrote: > commit 6768e3a2830 > Author: Aleksey Midenkov <midenok(a)gmail.com> > Date: Thu Jul 29 22:54:12 2021 +0300 > > diff --git a/sql/lex.h b/sql/lex.h > index cbf9d9d51b2..55eefc39a7e 100644 > --- a/sql/lex.h > +++ b/sql/lex.h > @@ -240,6 +240,7 @@ SYMBOL symbols[] = { > { "EXPLAIN", SYM(DESCRIBE)}, > { "EXTENDED", SYM(EXTENDED_SYM)}, > { "EXTENT_SIZE", SYM(EXTENT_SIZE_SYM)}, > + { "EXTRACT", SYM(EXTRACT_SYM)}, So, you did EXTRACT after all, not MIGRATE? ADD/EXTRACT aren't opposites in SQL. It would be rather consistent to have ALTER ... ADD PARTITION (...) FROM table_name and ALTER ... DROP PARTITION ... TO table_name except that DROP...TO looks kind of strange, and ADD...FROM does not convey the meaning that the table disappears. The second issue also applies to IMPORT/EXPORT. Another option, also kind of strange, but also consistent and makes it clear that the table/partition disappears: ALTER ... MOVE TABLE table_name TO PARTITION (...) ALTER ... MOVE PARTITION ... TO TABLE table_name or the same with CHANGE instead of MOVE to reuse an existing keyword. I'm not saying anything of the above is perfect, and better variants are very much welcome. But pretty much anything is better than ADD/EXTRACT. > { "FALSE", SYM(FALSE_SYM)}, > { "FAST", SYM(FAST_SYM)}, > { "FAULTS", SYM(FAULTS_SYM)}, > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index dc2eb4425f0..a41ea7d3e4a 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -7605,6 +7605,25 @@ alter_commands: > if (Lex->stmt_alter_table_exchange_partition($6)) > MYSQL_YYABORT; > } > + | EXTRACT_SYM PARTITION_SYM alt_part_name_item > + AS TABLE_SYM table_ident have_partitioning > + { > + LEX *lex= Lex; > + lex->first_select_lex()->db= $6->db; > + if (lex->first_select_lex()->db.str == NULL && > + lex->copy_db_to(&lex->first_select_lex()->db)) > + MYSQL_YYABORT; > + if (unlikely(check_table_name($6->table.str,$6->table.length, > + FALSE)) || > + ($6->db.str && unlikely(check_db_name((LEX_STRING*) &$6->db)))) > + my_yyabort_error((ER_WRONG_TABLE_NAME, MYF(0), $6->table.str)); good point, LEX::stmt_alter_table_exchange_partition() doesn't seem to do it > + lex->name= $6->table; > + > + 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; why wouldn't you combine the common code from here and EXCHANGE PARTITION? less code duplication and as a bonus EXCHANGE will get proper table/db name checks. > + } > ; > > remove_partitioning: > diff --git a/sql/sql_alter.h b/sql/sql_alter.h > index a0f89c28d2a..6aa65d232ca 100644 > --- a/sql/sql_alter.h > +++ b/sql/sql_alter.h > @@ -345,6 +348,8 @@ class Alter_table_ctx > /** Indicates that we are altering temporary table */ > bool tmp_table; > > + DDL_LOG_STATE *ddl_log_state; What do you need this for? > + > private: > char new_filename[FN_REFLEN + 1]; > char new_alias_buff[NAME_LEN + 1]; > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 779d125af70..033d77a827d 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -10016,7 +10146,7 @@ bool mysql_alter_table(THD *thd, const LEX_CSTRING *new_db, > } > > // In-place execution of ALTER TABLE for partitioning. > - DBUG_RETURN(fast_alter_partition_table(thd, table, alter_info, > + DBUG_RETURN(fast_alter_partition_table(thd, table, alter_info, &alter_ctx, > create_info, table_list, > &alter_ctx.db, > &alter_ctx.table_name)); you no longer need to pass alter_ctx.db and alter_ctx.table_name, if you pass the whole alter_ctx down. > diff --git a/include/my_dbug.h b/include/my_dbug.h > index e25bfcf28a7..cf969ce5750 100644 > --- a/include/my_dbug.h > +++ b/include/my_dbug.h > @@ -104,6 +104,8 @@ extern int (*dbug_sanity)(void); > (_db_keyword_(0,(keyword), 0) ? (a1) : (a2)) > #define DBUG_EVALUATE_IF(keyword,a1,a2) \ > (_db_keyword_(0,(keyword), 1) ? (a1) : (a2)) > +#define DBUG_TRUE_IF(keyword) DBUG_EVALUATE_IF(keyword, 1, 0) > +#define DBUG_FALSE_IF(keyword) DBUG_EVALUATE_IF(keyword, 0, 1) hmm. DBUG_FALSE_IF is not used anywhere, DBUG_TRUE_IF is used once as if (DBUG_TRUE_IF("error_extract_partition_00") || unlikely(error= file->ha_rename_table(from_name, to_name))) and this is typically written as if (DBUG_EVALUATE_IF("error_extract_partition_00", 1, error= file->ha_rename_table(from_name, to_name))) so I see no need for either DBUG_TRUE_IF or DBUG_FALSE_IF, please, remove both > #define DBUG_PRINT(keyword,arglist) \ > do if (_db_pargs_(__LINE__,keyword)) _db_doprnt_ arglist; while(0) > > diff --git a/sql/sql_partition.cc b/sql/sql_partition.cc > index 85d880cbbb4..4172a61812e 100644 > --- a/sql/sql_partition.cc > +++ b/sql/sql_partition.cc > @@ -5432,7 +5432,7 @@ uint prep_alter_part_table(THD *thd, TABLE *table, Alter_info *alter_info, > } while (++part_count < tab_part_info->num_parts); > if (num_parts_found != num_parts_dropped) > { > - my_error(ER_DROP_PARTITION_NON_EXISTENT, MYF(0), "DROP"); > + my_error(ER_DROP_PARTITION_NON_EXISTENT, MYF(0), cmd); Not part of your patch, I know, but the error message here is "Error in list of partitions to %-.64s" shouldn't it say "Partition does not exist" or something? > goto err; > } > if (table->file->is_fk_defined_on_table_or_index(MAX_KEY)) > @@ -7268,6 +7420,48 @@ uint fast_alter_partition_table(THD *thd, TABLE *table, > if (alter_partition_lock_handling(lpt)) > goto err; > } > + else if (alter_info->partition_flags & ALTER_PARTITION_EXTRACT) > + { > + if (mysql_write_frm(lpt, WFRM_WRITE_EXTRACTED) || > + write_log_drop_shadow_frm(lpt) || > + ERROR_INJECT_CRASH("crash_extract_partition_1") || > + ERROR_INJECT_ERROR("fail_extract_partition_1") || > + mysql_write_frm(lpt, WFRM_WRITE_SHADOW) || > + ERROR_INJECT_CRASH("crash_extract_partition_2") || > + ERROR_INJECT_ERROR("fail_extract_partition_2") || > + wait_while_table_is_used(thd, table, HA_EXTRA_NOT_USED) || > + ERROR_INJECT_CRASH("crash_extract_partition_3") || > + ERROR_INJECT_ERROR("fail_extract_partition_3") || > + write_log_extract_partition(lpt) || > + ERROR_INJECT_CRASH("crash_extract_partition_4") || > + ERROR_INJECT_ERROR("fail_extract_partition_4") || > + alter_close_table(lpt) || > + ERROR_INJECT_CRASH("crash_extract_partition_5") || > + ERROR_INJECT_ERROR("fail_extract_partition_5") || > + alter_partition_extract(lpt) || > + ERROR_INJECT_CRASH("crash_extract_partition_7") || > + ERROR_INJECT_ERROR("fail_extract_partition_7") || > + (frm_install= TRUE, FALSE) || > + mysql_write_frm(lpt, WFRM_INSTALL_SHADOW|WFRM_BACKUP_ORIGINAL) || > + log_partition_alter_to_ddl_log(lpt) || > + (frm_install= FALSE, FALSE) || > + ERROR_INJECT_CRASH("crash_extract_partition_8") || > + ERROR_INJECT_ERROR("fail_extract_partition_8") || > + (ddl_log_complete(lpt->part_info), false) || as far as I understand this ^^^ should be done *after* binlogging. I suspect that if you crash here, blnlog won't get this ALTER TABLE. > + ((!thd->lex->no_write_to_binlog) && > + (write_bin_log(thd, FALSE, > + thd->query(), thd->query_length()), FALSE)) || if you crash here, what will roll forward on recovery and drop the backup? The log is already closed here, isn't it? I don't understand why you need a backup. Why not to install shadow after the binlog was written? > + mysql_write_frm(lpt, WFRM_DROP_BACKUP) || > + ERROR_INJECT_CRASH("crash_extract_partition_9") || > + ERROR_INJECT_ERROR("fail_extract_partition_9")) > + { > + (void) ddl_log_revert(thd, lpt->part_info); > + handle_alter_part_error(lpt, true, true, frm_install); > + goto err; > + } > + if (alter_partition_lock_handling(lpt)) > + goto err; > + } > else if ((alter_info->partition_flags & ALTER_PARTITION_ADD) && > (part_info->part_type == RANGE_PARTITION || > part_info->part_type == LIST_PARTITION)) > diff --git a/mysql-test/include/lcase_names.inc b/mysql-test/include/lcase_names.inc > new file mode 100644 > index 00000000000..e69de29bb2d better put here some comment, like in other dummy *.inc files see e.g. platform.inc or protocol.inc > diff --git a/mysql-test/suite/atomic/alter_partition,aria.rdiff b/mysql-test/suite/atomic/alter_partition,aria.rdiff > new file mode 100644 > index 00000000000..20c0510c5ed > --- /dev/null > +++ b/mysql-test/suite/atomic/alter_partition,aria.rdiff > @@ -0,0 +1,219 @@ > +--- alter_partition.result > ++++ alter_partition,aria.reject > +@@ -1,4 +1,4 @@ > +-set @@default_storage_engine=MyISAM; > ++set @@default_storage_engine=Aria; > + # Crash recovery > + create or replace procedure prepare_table() > + begin > +@@ -13,12 +13,12 @@ > + end $ > + # QUERY: ALTER TABLE t1 EXTRACT PARTITION p1 AS TABLE tp1 > + # CRASH POINT: ddl_log_create_before_create_frm > +-t1#P#p0.MYD > +-t1#P#p0.MYI > +-t1#P#p1.MYD may be better `replace_result MAI MYI MAD MYD` ? if that's the only difference. To avoid big rdiff files. > +-t1#P#p1.MYI > +-t1#P#pn.MYD > +-t1#P#pn.MYI > ++t1#P#p0.MAD > ++t1#P#p0.MAI Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 20

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

09 Sep '21

Hi, Oleksandr! you forgot to check in the result file On Sep 07, Oleksandr Byelkin wrote: > revision-id: ca4ef7185da (mariadb-10.6.1-79-gca4ef7185da) > parent(s): 271afbc88e4 > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2021-09-07 14:35:05 +0200 > message: > > MDEV-9245 password "reuse prevention" validation plugin > > diff --git a/plugin/reuse_password_check/CMakeLists.txt b/plugin/reuse_password_check/CMakeLists.txt > --- /dev/null > +++ b/plugin/reuse_password_check/CMakeLists.txt > @@ -0,0 +1,4 @@ > + > +ADD_DEFINITIONS(-DMYSQL_SERVER) as I wrote to HF, this should not be needed. > + > +MYSQL_ADD_PLUGIN(reuse_password_check reuse_password_check.c MODULE_ONLY) Unless you have a reason why this must be "MODULE_ONLY", please remove MODULE_ONLY and try to compile and test with -DPLUGIN_REUSE_PASSWORD_CHECK=STATIC also, I'd suggest "password_reuse_check" not "reuse_password_check". > diff --git a/plugin/reuse_password_check/reuse_password_check.c b/plugin/reuse_password_check/reuse_password_check.c > --- /dev/null > +++ b/plugin/reuse_password_check/reuse_password_check.c > @@ -0,0 +1,236 @@ > +/* Copyright (c) 2021, Oleksandr Byelkin and MariaDB > + > + 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 St, Fifth Floor, Boston, MA 02110-1335 USA */ > + > +#include <my_config.h> > +#include <my_global.h> > +#include <my_base.h> > +#include <mysql/plugin_password_validation.h> > +#include <mysql.h> only plugin_password_validation.h should be needed here. > + > + > +#include <mysqld_error.h> this might be needed all right > +#include <mysql/service_sha2.h> but this isn't needed either > + > +#define HISTORY_DB_NAME "reuse_password_check_history" > + > +#define SQL_BUFF_LEN 2048 > + > +// 0 - unlimited, otherwise number of days to check > +static unsigned interval= 0; > + > +// helping string for bin_to_hex512 > +static char digits[]= "0123456789ABCDEF"; > + > + > +/** > + Convert string of 512 bits (64 bytes) to hex representation > + > + @param to pointer to the result puffer > + (should be at least 64*2 bytes) > + @param str pointer to 512 bits (64 bytes string) > +*/ > + > +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[((uchar) *str) >> 4]; > + *to++= digits[((uchar) *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, "[%d] %s", ME_WARNING, > + mysql_errno(mysql), mysql_error(mysql)); may be include plugin name here? > +} > + > + > +/** > + Create the history of passwords table for this plugin. > + > + @param mysql Connection handler > + > + @retval 1 - Error > + @retval 0 - OK > +*/ > + > +static int create_table(MYSQL *mysql) > +{ > + if (mysql_real_query(mysql, > + // 512/8 = 64 > + STRING_WITH_LEN("CREATE TABLE mysql." HISTORY_DB_NAME > + " ( hash binary(64)," > + " time timestamp," make it "default current_timestamp", just to be explicit. even though it's implicitly default current_timestamp already. > + " primary key (hash), index tm (time) )" > + " ENGINE=Aria"))) > + { > + report_sql_error(mysql); > + return 1; > + } > + return 0; > +} > + > + > +/** > + Run this query and create table if needed > + > + @param mysql Connection handler > + @param query The query to run > + @param len length of the query text > + > + @retval 1 - Error > + @retval 0 - OK > +*/ > + > +static int run_query_with_table_creation(MYSQL *mysql, const char *query, > + size_t len) > +{ > + if (unlikely(mysql_real_query(mysql, query, len))) > + { > + unsigned int rc= mysql_errno(mysql); > + if (rc != ER_NO_SUCH_TABLE) > + { > + // suppress this error in case of try to add the same password twice > + if (rc != ER_DUP_ENTRY) > + report_sql_error(mysql); > + return 1; > + } > + if (create_table(mysql)) > + return 1; > + if (unlikely(mysql_real_query(mysql, query, len))) > + { > + report_sql_error(mysql); > + return 1; > + } > + } > + return 0; > +} > + > + > +/** > + Password validator > + > + @param username User name (part of whole login name) > + @param password Password to validate > + @param hostname Host name (part of whole login name) > + > + @retval 1 - Password is not OK or an error happened > + @retval 0 - Password is OK > +*/ > + > +static int validate(const MYSQL_CONST_LEX_STRING *username, > + const MYSQL_CONST_LEX_STRING *password, > + const MYSQL_CONST_LEX_STRING *hostname) > +{ > + MYSQL *mysql= NULL; > + size_t key_len= username->length + password->length + hostname->length; > + size_t buff_len= (key_len > SQL_BUFF_LEN ? key_len : SQL_BUFF_LEN); > + size_t len; > + char *buff= malloc(buff_len); > + unsigned char hash[512/8]; > + char escaped_hash[512/8*2 + 1]; > + if (!buff) > + return 1; > + > + mysql= mysql_init(NULL); > + if (!mysql) > + { > + free(buff); > + return 1; > + } > + > + memcpy(buff, hostname->str, hostname->length); > + memcpy(buff + hostname->length, username->str, username->length); > + memcpy(buff + hostname->length + username->length, password->str, > + password->length); > + buff[key_len]= 0; > + bzero(hash, sizeof(hash)); > + my_sha512(hash, buff, key_len); > + if (mysql_real_connect_local(mysql, NULL, NULL, NULL, 0) == NULL) > + goto sql_error; > + > + if (interval) > + { > + // trim the table > + len= snprintf(buff, buff_len, > + "DELETE FROM mysql." HISTORY_DB_NAME > + " WHERE time < DATE_SUB(NOW(), interval %d day)", > + interval); > + if (unlikely(run_query_with_table_creation(mysql, buff, len))) > + goto sql_error; > + } > + > + bin_to_hex512(escaped_hash, hash); > + escaped_hash[512/8*2]= '\0'; > + len= snprintf(buff, buff_len, > + "INSERT INTO mysql." HISTORY_DB_NAME "(hash) " > + "values (x'%s')", > + escaped_hash); > + if (unlikely(run_query_with_table_creation(mysql, buff, len))) > + goto sql_error; > + > + free(buff); > + mysql_close(mysql); > + return 0; // OK > + > +sql_error: > + free(buff); > + if (mysql) > + mysql_close(mysql); > + return 1; // Error > +} > + > +static MYSQL_SYSVAR_UINT(interval, interval, PLUGIN_VAR_RQCMDARG, > + "How old (in days) passwords to check (0 means unlimited)", NULL, NULL, better "Password history retention period in days (0 means unlimited)" > > + 0, 0, 365*100, 1); > + > + > +static struct st_mysql_sys_var* sysvars[]= { > + MYSQL_SYSVAR(interval), > + NULL > +}; > + > +static struct st_mariadb_password_validation info= > +{ > + MariaDB_PASSWORD_VALIDATION_INTERFACE_VERSION, > + validate > +}; > + > +maria_declare_plugin(simple_password_check) ^^^ must be the same name as in CMakeLists.txt, otherwise static linking won't work. > +{ > + MariaDB_PASSWORD_VALIDATION_PLUGIN, > + &info, > + "reuse_password_check", > + "Oleksandr Byelkin", > + "Reusage of password check", Better "Password reuse check" or, as it's a description, "Prevent password reuse" > + PLUGIN_LICENSE_GPL, > + NULL, > + NULL, > + 0x0100, > + NULL, > + sysvars, > + "1.0", > + MariaDB_PLUGIN_MATURITY_ALPHA > +} > +maria_declare_plugin_end; > > diff --git a/mysql-test/suite/plugins/t/reuse_password_check.test b/mysql-test/suite/plugins/t/reuse_password_check.test > --- /dev/null > +++ b/mysql-test/suite/plugins/t/reuse_password_check.test > @@ -0,0 +1,58 @@ > +--source include/not_embedded.inc > + > +if (!$REUSE_PASSWORD_CHECK_SO) { > + skip No REUSE_PASSWORD_CHECK plugin; > +} > + > +install soname "reuse_password_check"; > + > +set @save_reuse_password_check_interval= @@reuse_password_check_interval; > + > +set global reuse_password_check_interval= 0; > + > +--echo # Default value (sould be unlimited i.e. 0) > +SHOW GLOBAL VARIABLES like "reuse_password_check%"; > + > +--echo # insert user > +grant select on *.* to user_name@localhost identified by 'test_pwd'; what about sql mode that doesn't allow to create users with grant? I don't see where you've disabled that. > + > +#--error ER_NOT_VALID_PASSWORD > +#grant select on *.* to user_name@localhost identified by 'test_pwd'; > +#show warnings; why is that commented out? > + > +--error ER_CANNOT_USER > +alter user user_name@localhost identified by 'test_pwd'; > +show warnings; > + > +# Plugin does not work for it > +#--error ER_NOT_VALID_PASSWORD > +#SET PASSWORD FOR user_name@localhost = PASSWORD('test_pwd'); > + > +--echo # check exparation > + > +set global reuse_password_check_interval= 10; > + > +--error ER_CANNOT_USER > +alter user user_name@localhost identified by 'test_pwd'; > +show warnings; > +select hex(hash) from mysql.reuse_password_check_history; > + > +--echo # emulate old password > +update mysql.reuse_password_check_history set time= date_sub(now(), interval > +11 day); > + > +alter user user_name@localhost identified by 'test_pwd'; > +show warnings; > + > +drop user user_name@localhost; > + > +show create table mysql.reuse_password_check_history; > +select count(*) from mysql.reuse_password_check_history; > + > +drop table mysql.reuse_password_check_history; > + > +set @save_reuse_password_check_interval= @@reuse_password_check_interval; > + > +set global reuse_password_check_interval= @save_reuse_password_check_interval; > + > +uninstall plugin reuse_password_check; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

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