- developers - lists.mariadb.org

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

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