- developers - lists.mariadb.org

Re: [Maria-developers] 7807d9b6939: MDEV-22974: mysql_native_password make "invalid" valid
by Sergei Golubchik 30 Oct '20

30 Oct '20

Hi, Daniel! On Oct 22, Daniel Black wrote: > revision-id: 7807d9b6939 (mariadb-10.5.4-62-g7807d9b6939) > parent(s): 054f10365c4 > author: Daniel Black <daniel(a)mariadb.org> > committer: Daniel Black <daniel(a)mariadb.org> > timestamp: 2020-07-16 17:06:36 +1000 > message: > > MDEV-22974: mysql_native_password make "invalid" valid > > Per b9f3f06857ac, mysql_system_tables_data.sql creates > a mysql_native_password with a salted hash of "invalid" so that `set password` > will detect a native password can be pplied:. "applied" > > SHOW CREATE USER; dilignently uses this value in its output > generating the SQL: > > MariaDB [(none)]> show create user; > > +---------------------------------------------------------------------------------------------------+ > | CREATE USER for dan@localhost | > +---------------------------------------------------------------------------------------------------+ > | CREATE USER `dan`@`localhost` IDENTIFIED VIA mysql_native_password USING 'invalid' OR unix_socket | > +---------------------------------------------------------------------------------------------------+ > > Attempting to execute this before this patch resutls in: > > MariaDB [(none)]> CREATE USER `dan2`@`localhost` IDENTIFIED VIA mysql_native_password USING 'invalid' OR unix_socket; > ERROR 1372 (HY000): Password hash should be a 41-digit hexadecimal number > > As such deep the the implementation of mysql_native_password we make > "invalid" valid (pun intended) such that the above create user will > succeed. > > native_password_get_salt is only called in the context of > set_user_salt, so all setting of native passwords to hashed content of > 'invalid', quite literally create an invalid password. > > So other forms of "invalid" are valid SQL in creating invalid passwords: > > MariaDB [(none)]> set password = 'invalid'; > Query OK, 0 rows affected (0.001 sec) > > MariaDB [(none)]> alter user dan@localhost IDENTIFIED BY PASSWORD 'invalid'; > Query OK, 0 rows affected (0.000 sec) > > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index d94016b7815..3cd7a67ae1a 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -14539,6 +14539,12 @@ static int native_password_get_salt(const char *hash, size_t hash_length, > > if (hash_length != SCRAMBLED_PASSWORD_CHAR_LENGTH) > { > + if (hash_length == 7 && strcmp(hash, "invalid") == 0) > + { > + memcpy(out, "invalid", 7); > + *out_length= 7; > + return 0; > + } okay. After you said ASAN, I think I can see why this could be problematic. You can, of course, pad it to SCRAMBLED_PASSWORD_CHAR_LENGTH, but then you'll create a *valid* scramble that would correspond to some actual password. One option would be to allow "invalid" literal in set_user_auth(), before even any plugin checks. But I'm unsure of the implications. > my_error(ER_PASSWD_LENGTH, MYF(0), SCRAMBLED_PASSWORD_CHAR_LENGTH); > return 1; > } > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3

Re: [Maria-developers] f112c4a07475: MDEV-20787: Script dgcov.pl does not work
by Sergei Golubchik 30 Oct '20

30 Oct '20

Hi, Anel! On Oct 30, Anel Husakovic wrote: > revision-id: f112c4a07475 (mariadb-10.2.31-541-gf112c4a07475) > parent(s): 784473b98662 > author: Anel Husakovic <anel(a)mariadb.org> > committer: Anel Husakovic <anel(a)mariadb.org> > timestamp: 2020-10-26 14:25:41 +0100 > message: > > MDEV-20787: Script dgcov.pl does not work > > - Patch is changing CMakeList with `--coverage` flag as an alias for > `-fprofile-arcs -ftest-coverage -lgcov` in addition. > > - About the usage of `dgcov`: > When the server is compiled with `-DENABLE_GCOV=ON`, from object files are generated > `.gcno` and `.gcda` files. > `./mtr --gcov is_check_constraint` is invoking the following script calls: > - `./dgcov.pl --purge`, `./mtr is_check_constraint` and > - `./dgcov.pl --generate>/var/last_changes.dgcov`. > The `purge` flag is clearing `.gcda` files (and others extensions), > while running the test (which follows after the purge) new `.gcda` files are generated. > With dgcov `generate` flag, `gcov -i` (`intermediate format`) is called > on generated `<source-files-name>.gcda` files (`dbug.c.gcda` e.g.). > The patch is tested on `gcov 6.3` and `gcov 7.4` versions > and can be seen that resulting `.gcov` file for `6.3` creates > `<full path>.gcov` (`dbug.c.gcda.gcov` e.g.) file, > where `gcov 7.4+` is still creating `source-file-names.gcov`(`dbug.c.gcov`) files > as `gcov` in general is doing. > Results are stored in `./mysql-test/var/last_changes.dgcov`. > Note: for the newer versions of `gcov` intermediate format is > deprecated `-i` and should be used `--json-format`. does it mean that eventually we'll have to rewrite dgcov to support json format? > > - This patch doesn't take gcov versions in account since is extracting > filename of a `.gcov` file, which is generated with `gcov -i > <source-file>.gcda` during `dgcov.pl --generate`. > > - There is a check if file exists. > Reason for that is because some generated `gcov` files are outliers from above > rule (that a file generated in intermediate format(gcov) has the same source name). > Example 1: `aestables.cpp.gcda` is not generating `aestables.cpp.gcov`, > but only the files used in #include `modes.hpp.gcov runtime.hpp.gcov` > Example 2: `my_sha256.cc.gcda` is generating `my_sha.ic.gcov` > > - Gcov with out-of-source is not working, this patch will solve that > (probably?) > Make sure to test `MTR_BINDIR` which gets set during out-of-source > build and generate an error when running `./mysql-test/mtr --gcov alias`. > > diff --git a/mysql-test/dgcov.pl b/mysql-test/dgcov.pl > index fbc5540e6973..f2bbdad4d20e 100755 > --- a/mysql-test/dgcov.pl > +++ b/mysql-test/dgcov.pl > @@ -161,8 +161,14 @@ sub gcov_one_file { > system($cmd)==0 or die "system($cmd): $? $!"; > } I don't see any changes since the last review. Is it the correct commit? > - # now, read the generated file > - open FH, '<', "$_.gcov" or die "open(<$_.gcov): $!"; > + (my $filename = $_)=~ s/\.[^.]+$//; # remove extension I think this contradicts your explanation in the commit comment. You wrote that the file name can be either dbug.c.gcda.gcov or dbug.c.gcov. Here $_ is dbug.c.gcda, you unconditionally remove .gcda and then below you open $filename.gcov, that is dbug.c.gcov Which means this won't work with an older gcov. Is this your intention? I think it's fine not to support old gcov. But this should be a conscious decision, e.g. you can write in the commit comment "Now dgcov.pl only supports gcov 7.4+" - if that's indeed what you want. Also, if you do it this way, then there's no need to use s///, it's simpler to do, like sub gcov_one_file { return unless /\.gcda$/; + my $filename= $`; unless ($opt_skip_gcov) { or, may be even sub gcov_one_file { return unless /\.gcda$/; + my $gcov_file_path= "$File::Find::dir$`.gcov"; unless ($opt_skip_gcov) { > + my $gcov_file_path= $File::Find::dir."/$filename.gcov"; still an extra slash > + if (! -f $gcov_file_path) > + { > + return; > + } still not return unless -f $gcov_file_path; > + open FH, '<', "$gcov_file_path" or die "open(<$gcov_file_path): $!"; still extra quotes > + > my $fname; > while (<FH>) { > chomp; > diff --git a/mysql-test/mysql-test-run.pl b/mysql-test/mysql-test-run.pl > index 900ef736a455..2189529d82a7 100755 > --- a/mysql-test/mysql-test-run.pl > +++ b/mysql-test/mysql-test-run.pl > @@ -1791,7 +1791,7 @@ sub command_line_setup { > # -------------------------------------------------------------------------- > # Gcov flag > # -------------------------------------------------------------------------- > - if ( ($opt_gcov or $opt_gprof) and ! $source_dist ) > + if ( ($opt_gcov or $opt_gprof) and (! $source_dist or -d $ENV{MTR_BINDIR})) > { > mtr_error("Coverage test needs the source - please use source dist"); > } > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] MDEV-24015: SQL Error (1038): Out of sort memory when enough memory for the sort buffer is provided
by Sergey Petrunia 27 Oct '20

27 Oct '20

Hi Varun, I have only cosmetic comments, please find them below. Ok to push after they are addressed. > The issue here was with the re-execution of a subquey, after few executions > the subquery execution returned error that not enough space was provided inside the sort > buffer. > This is happening because with the introduction of packed sort keys, during the phase > where sort length is calculated the parameters of the sort keys are not reset and > we keep up adding the sort length to the previous calculated sort length for the > earlier executions of the subquery. This happens because the sort keys structure is > allocated only once and so if their is re-execution of a subquery involving sorting > then we should be resetting the parameters of the sort keys. The comment is hard to read. How about being more specific and using shorter sentences: The error was: filesort was done in a subquery which was executed multiple times. During each execution, sortlength() computed total sort key length in Sort_keys::sort_length, without resetting it first. Eventually Sort_keys::sort_length got larger than @@sort_buffer_size, which caused filesort() to be aborted with error. Fixed by making sortlength() to compute lengths only during the first invocation. Subsequent invocations return pre-computed values. > @@ -2518,7 +2526,7 @@ void Sort_param::try_to_pack_sortkeys() > return; > > const uint sz= Sort_keys::size_of_length_field; > - uint sort_len= sort_keys->get_sort_length(); > + uint sort_len= sort_keys->get_sort_length_with_original_values(); > > /* > Heuristic introduced, skip packing sort keys if saving less than 128 bytes > diff --git a/sql/sql_sort.h b/sql/sql_sort.h > index 40f0c5ede5f..6951d4d190a 100644 > --- a/sql/sql_sort.h > +++ b/sql/sql_sort.h > @@ -258,7 +258,9 @@ class Sort_keys :public Sql_alloc, > Sort_keys_array(arr, count), > m_using_packed_sortkeys(false), > size_of_packable_fields(0), > - sort_length(0) > + sort_length_with_original_values(0), > + sort_length_with_memcmp_values(0), > + first_execution(true) This violates the coding style. Please ident the member initialization (see e.g. item.h for examples) > { > DBUG_ASSERT(!is_null()); > }a ... > @@ -307,9 +319,12 @@ class Sort_keys :public Sql_alloc, > > void increment_original_sort_length(uint len) > { > - sort_length+= len; > + sort_length_with_original_values+= len; > } > > + bool is_first_execution() { return first_execution; } > + void set_first_execution(bool val) { first_execution= val; } A comment just about the naming: I would not drag "execution" into here. "Sort_keys" are not really executed. How about changing the functions to be bool is_parameters_computed(); bool set_parameters_computed(); and the variable to be "bool parameters_computed" ? > + > static const uint size_of_length_field= 4; > > private: BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] 5150dfd6ab3: MDEV-17891 Assertion failures in select_insert::abort_result_set and mysql_load upon attempt to replace into a full table
by Sergei Golubchik 25 Oct '20

25 Oct '20

Hi, Nikita! On Apr 27, Nikita Malyavin wrote: > revision-id: 5150dfd6ab3 (mariadb-10.3.12-230-g5150dfd6ab3) > parent(s): cf78b8c699d > author: Nikita Malyavin <nikitamalyavin(a)gmail.com> > committer: Nikita Malyavin <nikitamalyavin(a)gmail.com> > timestamp: 2019-06-12 22:11:45 +1000 > message: > > MDEV-17891 Assertion failures in select_insert::abort_result_set and mysql_load upon attempt to replace into a full table > > * set modified_non_trans_table in one missed place This commit needs way more text than that. This is just too misterious now. What asserts fails? What has the test case to do with the fix? Why modified_non_trans_table even matters without replication? Why it's specific to versioning and partitioning? Where trans_safe becomes false? > diff --git a/mysql-test/suite/versioning/r/partition.result b/mysql-test/suite/versioning/r/partition.result > index 3fcb59bdb40..99a297417d5 100644 > --- a/mysql-test/suite/versioning/r/partition.result > +++ b/mysql-test/suite/versioning/r/partition.result > @@ -541,6 +541,29 @@ t1 CREATE TABLE `t1` ( > PARTITION BY SYSTEM_TIME INTERVAL 7 SECOND > (PARTITION `ver_p1` HISTORY ENGINE = DEFAULT_ENGINE, > PARTITION `ver_pn` CURRENT ENGINE = DEFAULT_ENGINE) > +set timestamp= default; > +# MDEV-17891 Assertion failures in select_insert::abort_result_set and > +# mysql_load upon attempt to replace into a full table > +set @@max_heap_table_size= 1024*1024; > +create or replace table t1 ( > +pk integer auto_increment, > +primary key (pk), > +f varchar(45000) > +) with system versioning engine=memory > +partition by system_time interval 1 year (partition p1 history, > +partition pn current); > +# fill the table until full > +insert into t1 () values (),(),(),(),(),(),(),(),(),(),(),(),(),(),(),(); > +insert into t1 (f) select f from t1; > +ERROR HY000: The table 't1' is full > +# leave space for exactly one record in current partition > +delete from t1 where pk = 1; > +# copy all data into history partition > +replace into t1 select * from t1; > +replace into t1 select * from t1; > +ERROR HY000: The table 't1' is full > +drop table t1; > +set @@max_heap_table_size= 1048576; > # Test cleanup > drop database test; > create database test; > diff --git a/mysql-test/suite/versioning/t/partition.test b/mysql-test/suite/versioning/t/partition.test > index d5c83b4d3bb..23768836efc 100644 > --- a/mysql-test/suite/versioning/t/partition.test > +++ b/mysql-test/suite/versioning/t/partition.test > @@ -474,6 +474,7 @@ set timestamp=1523466002.799571; > insert into t1 values (11),(12); > set timestamp=1523466004.169435; > delete from t1 where pk in (11, 12); > +set timestamp= default; > > --echo # > --echo # MDEV-18136 Server crashes in Item_func_dyncol_create::prepare_arguments > @@ -489,6 +490,34 @@ partition by system_time interval column_get(column_create(7,7), 7 as int) secon > --replace_result $default_engine DEFAULT_ENGINE > show create table t1; > > +--echo # MDEV-17891 Assertion failures in select_insert::abort_result_set and > +--echo # mysql_load upon attempt to replace into a full table > + > +--let $max_heap_table_size_orig= `select @@max_heap_table_size;` > +set @@max_heap_table_size= 1024*1024; > +create or replace table t1 ( > + pk integer auto_increment, > + primary key (pk), > + f varchar(45000) > +) with system versioning engine=memory > + partition by system_time interval 1 year (partition p1 history, > + partition pn current); > + > +--echo # fill the table until full > +insert into t1 () values (),(),(),(),(),(),(),(),(),(),(),(),(),(),(),(); > +--error ER_RECORD_FILE_FULL > +insert into t1 (f) select f from t1; > +--echo # leave space for exactly one record in current partition > +delete from t1 where pk = 1; > +--echo # copy all data into history partition > +replace into t1 select * from t1; > +--error ER_RECORD_FILE_FULL > +replace into t1 select * from t1; > + > +# cleanup > +drop table t1; > +eval set @@max_heap_table_size= $max_heap_table_size_orig; > + > --echo # Test cleanup > drop database test; > create database test; > diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc > index 1f3a70721fc..44502ea6704 100644 > --- a/sql/sql_insert.cc > +++ b/sql/sql_insert.cc > @@ -1953,6 +1953,8 @@ int write_record(THD *thd, TABLE *table,COPY_INFO *info) > if (likely(!error)) > { > info->deleted++; > + if (!table->file->has_transactions()) > + thd->transaction.stmt.modified_non_trans_table= TRUE; > if (table->versioned(VERS_TIMESTAMP)) > { > store_record(table, record[2]); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] 05968211699: MDEV-16026: Global system_versioning_asof must not be used if client sessions can have non-default time zone
by Sergei Golubchik 25 Oct '20

25 Oct '20

Hi, Nikita! Okay. This reverts my "ok to push" on MDEV-16481 :) If you want to store the value in unix timestamp, you don't need MYSQL_TIME in save_result. You should calculate the value as a unix timestamp in ::check and store that in save_result. Now you still have timezone conversion in ::update and that can fail. Everything that can fail should be done in ::check, that's the contract. May be also I reapplied your patches to 10.3 incorrectly when I was reviewing. Perhaps you could rebase them yourself - to be sure I'm looking at the correct patch? On Oct 22, Nikita Malyavin wrote: > revision-id: 05968211699 (mariadb-10.4.11-291-g05968211699) > parent(s): efb1023b6f4 > author: Nikita Malyavin <nikitamalyavin(a)gmail.com> > committer: Sergei Golubchik <serg(a)mariadb.com> > timestamp: 2020-10-22 17:07:03 +0200 > message: > > MDEV-16026: Global system_versioning_asof must not be used if client sessions can have non-default time zone > > * store `system_versioning_asof` in unix time; > * both session and global vars are processed in session timezone; > * setting `default` does not copy global varibale abymore. Instead, it sets system_time to SYSTEM_TIME_UNSPECIFIED, which means that no 'AS OF' time is applied and `now()` can be assumed > As a regression, we cannot assign values below 1970 anymore > > --- > mysql-test/suite/versioning/r/sysvars.result | 67 +++++++++++++++++++++++ > mysql-test/suite/versioning/t/sysvars.test | 80 ++++++++++++++++++++++++---- > sql/mysqld.h | 3 +- > sql/sql_select.cc | 6 ++- > sql/sys_vars.ic | 27 ++++++---- > 5 files changed, 162 insertions(+), 21 deletions(-) > > diff --git a/sql/mysqld.h b/sql/mysqld.h > index bd45ff7b798..884e5a06066 100644 > --- a/sql/mysqld.h > +++ b/sql/mysqld.h > @@ -194,7 +194,8 @@ enum vers_system_time_t > struct vers_asof_timestamp_t > { > ulong type; > - MYSQL_TIME ltime; > + my_time_t unix_time; > + ulong second_part; > }; > > enum vers_alter_history_enum > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index d0bb0c816ec..4d62a9829eb 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -722,8 +722,12 @@ bool vers_select_conds_t::init_from_sysvar(THD *thd) > if (type != SYSTEM_TIME_UNSPECIFIED && type != SYSTEM_TIME_ALL) > { > DBUG_ASSERT(type == SYSTEM_TIME_AS_OF); > + MYSQL_TIME ltime; > + thd->variables.time_zone->gmt_sec_to_TIME(&ltime, in.unix_time); > + ltime.second_part = in.second_part; > + > start.item= new (thd->mem_root) > - Item_datetime_literal(thd, &in.ltime, TIME_SECOND_PART_DIGITS); > + Item_datetime_literal(thd, &ltime, TIME_SECOND_PART_DIGITS); > if (!start.item) > return true; > } > diff --git a/sql/sys_vars.ic b/sql/sys_vars.ic > index f33f469b160..417fa7842c2 100644 > --- a/sql/sys_vars.ic > +++ b/sql/sys_vars.ic > @@ -2646,9 +2646,11 @@ class Sys_var_vers_asof: public Sys_var_enum > } > > private: > - bool update(set_var *var, vers_asof_timestamp_t &out) > + bool update(set_var *var, vers_asof_timestamp_t &out, system_variables& pool) > { > bool res= false; > + uint error; > + MYSQL_TIME ltime; > out.type= static_cast<enum_var_type>(var->save_result.ulonglong_value); > if (out.type == SYSTEM_TIME_AS_OF) > { > @@ -2658,11 +2660,14 @@ class Sys_var_vers_asof: public Sys_var_enum > Datetime::Options opt(TIME_CONV_NONE | > TIME_NO_ZERO_IN_DATE | > TIME_NO_ZERO_DATE, thd); > - res= var->value->get_date(thd, &out.ltime, opt); > + res= var->value->get_date(thd, &ltime, opt); > + out.unix_time= pool.time_zone->TIME_to_gmt_sec(&ltime, &error); > + out.second_part= ltime.second_part; > + res|= (error != 0); > } > else // set DEFAULT from global var > { > - out= global_var(vers_asof_timestamp_t); > + out.type= SYSTEM_TIME_UNSPECIFIED; > res= false; > } > } > @@ -2672,11 +2677,11 @@ class Sys_var_vers_asof: public Sys_var_enum > public: > virtual bool global_update(THD *thd, set_var *var) > { > - return update(var, global_var(vers_asof_timestamp_t)); > + return update(var, global_var(vers_asof_timestamp_t), thd->variables); > } > virtual bool session_update(THD *thd, set_var *var) > { > - return update(var, session_var(thd, vers_asof_timestamp_t)); > + return update(var, session_var(thd, vers_asof_timestamp_t), thd->variables); > } > > private: > @@ -2690,10 +2695,14 @@ class Sys_var_vers_asof: public Sys_var_enum > case SYSTEM_TIME_AS_OF: > { > uchar *buf= (uchar*) thd->alloc(MAX_DATE_STRING_REP_LENGTH); > - if (buf &&!my_datetime_to_str(&val.ltime, (char*) buf, 6)) > + MYSQL_TIME ltime; > + > + thd->variables.time_zone->gmt_sec_to_TIME(&ltime, val.unix_time); > + ltime.second_part= val.second_part; > + > + if (buf && !my_datetime_to_str(&ltime, (char*) buf, 6)) > { > - // TODO: figure out variable name > - my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), "system_versioning_asof_timestamp", "NULL (wrong datetime)"); > + my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), name.str, "NULL (wrong datetime)"); > return (uchar*) thd->strdup("Error: wrong datetime"); > } > return buf; > @@ -2701,7 +2710,7 @@ class Sys_var_vers_asof: public Sys_var_enum > default: > break; > } > - my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), "system_versioning_asof_timestamp", "NULL (wrong range type)"); > + my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), name.str, "NULL (wrong range type)"); > return (uchar*) thd->strdup("Error: wrong range type"); > } > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] caec6b4b3d7: MDEV-17573 Assertion in federatedx on multi-update
by Sergei Golubchik 23 Oct '20

23 Oct '20

Hi, Aleksey! Looks reasonable to me. MYSQL_RES is stored in the handler. Of course, MYSQL_ROWS *current, which is a pointer into MYSQL_RES, has to be in the handler, it cannot be shared. Ok to push, thanks! On Oct 23, Aleksey Midenkov wrote: > revision-id: caec6b4b3d7 (mariadb-10.3.21-250-gcaec6b4b3d7) > parent(s): a00c668ddf2 > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Aleksey Midenkov <midenok(a)gmail.com> > timestamp: 2020-10-21 13:38:37 +0300 > message: > > MDEV-17573 Assertion in federatedx on multi-update > > Cause: shared federatedx_io cannot store table-specific data. > > Fix: move current row reference `federatedx_io_mysql::current` to > ha_federatedx. > > FederatedX connection (represented by federatedx_io) is stored into > federatedx_txn::txn_list of per-server connections (see > federatedx_txn::acquire()). federatedx_txn object is stored into THD > (see ha_federatedx::external_lock()). When multiple handlers acquire > FederatedX connection they get single federatedx_io instance. Multiple > handlers do their operation via federatedx_io_mysql::mark_position() > and federatedx_io_mysql::fetch_row() in arbitrarty manner. They access > the same federatedx_io_mysql instance and same MYSQL_ROWS *current > pointer, so one handler disrupts the work of the other. > > Related to "MDEV-14551 Can't find record in table on multi-table update > with ORDER BY". Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 2e0bac4688a: MDEV-22313: Neither SHOW CREATE USER nor SHOW GRANTS prints a user's default role
by Sergei Golubchik 23 Oct '20

23 Oct '20

Hi, Anel! On Oct 23, Anel Husakovic wrote: > revision-id: 2e0bac4688a (mariadb-10.2.31-517-g2e0bac4688a) > parent(s): 985ede92034 > author: Anel Husakovic <anel(a)mariadb.org> > committer: Anel Husakovic <anel(a)mariadb.org> > timestamp: 2020-10-23 12:38:11 +0200 > message: > > MDEV-22313: Neither SHOW CREATE USER nor SHOW GRANTS prints a user's default role > > diff --git a/mysql-test/r/grant5.result b/mysql-test/r/grant5.result > index 338bbd36e14..a69070e8399 100644 > --- a/mysql-test/r/grant5.result > +++ b/mysql-test/r/grant5.result ... > +SHOW GRANTS; > +Grants for root@localhost > +GRANT test_role TO 'root'@'localhost' WITH ADMIN OPTION > +GRANT ALL PRIVILEGES ON *.* TO 'root'@'localhost' WITH GRANT OPTION > +GRANT PROXY ON ''@'%' TO 'root'@'localhost' WITH GRANT OPTION > +GRANT USAGE ON *.* TO 'test_role' > +SET DEFAULT ROLE test_role FOR 'root'@'localhost' ... Yes, that's what I mean. I don't have a strong opinion on that, it seems that I very slightly prefer to have SET DEFAULT ROLE after GRANT PROXY, that is to have all statements for root, and then statements for roles. But having SET DEFAULT ROLE at the end makes sense too - all GRANTs first, then SET DEFAULT ROLE. Just make it the way you want. But either way it's good to have a test for it, a test that shows we've considered this issue and intentionally implemented that particular order. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 6d881a271a9: MDEV-23178: Qualified asterisk not supported in INSERT .. RETURNING
by Sergei Golubchik 23 Oct '20

23 Oct '20

Hi, Rucha! On Oct 23, Rucha Deodhar wrote: > revision-id: 6d881a271a9 (mariadb-10.5.4-108-g6d881a271a9) > parent(s): bbd70fcc43c > author: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > committer: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > timestamp: 2020-10-23 12:57:02 +0530 > message: > > MDEV-23178: Qualified asterisk not supported in INSERT .. RETURNING > > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > index 45ce4be3eb5..4ce7b034029 100644 > --- a/sql/sql_base.cc > +++ b/sql/sql_base.cc > @@ -7959,7 +7959,7 @@ insert_fields(THD *thd, Name_resolution_context *context, const char *db_name, > bool any_privileges, uint *hidden_bit_fields) > { > Field_iterator_table_ref field_iterator; > - bool found; > + bool found, is_insert_or_replace_returning= false; > char name_buff[SAFE_NAME_LEN+1]; > DBUG_ENTER("insert_fields"); > DBUG_PRINT("arena", ("stmt arena: %p",thd->stmt_arena)); > @@ -7978,13 +7978,31 @@ insert_fields(THD *thd, Name_resolution_context *context, const char *db_name, > > found= FALSE; > > + if ((thd->lex->sql_command == SQLCOM_INSERT_SELECT || > + thd->lex->sql_command == SQLCOM_REPLACE_SELECT || > + thd->lex->sql_command == SQLCOM_INSERT || > + thd->lex->sql_command == SQLCOM_REPLACE) && > + thd->lex->has_returning()) > + is_insert_or_replace_returning= true; > + > /* > If table names are qualified, then loop over all tables used in the query, > else treat natural joins as leaves and do not iterate over their underlying > tables. > + Also, When we have INSERT/REPLACE...RETURNING and have qualified asterisk, > + table_name is not NULL and context->table_list is either NULL or has > + incorrect reference because context->table_list has tables from the > + FROM clause. > + context->table_list has incorrect reference (has table from the FROM clause > + instead of table we are inserting into) for > + INSERT/REPLACE...SELECT...RETURNING because we have a FROM clause from the > + SELECT statement. For INSERT/REPLACE...RETURNING it is NULL because > + there is no FROM clause. > */ > - for (TABLE_LIST *tables= (table_name ? context->table_list : > - context->first_name_resolution_table); > + for (TABLE_LIST *tables= (table_name ? (is_insert_or_replace_returning ? > + context->first_name_resolution_table : > + context->table_list) : > + (context->first_name_resolution_table)); ok, I've compiled it to run few queries, now I see that this is wrong. you need to understand the old code and the old comment before modifying them. one way to do it is to change the condition from table_name ? context->table_list : context->first_name_resolution_table to simply context->first_name_resolution_table and see what breaks. You'll see that main.type_varchar will break with At line 319: query 'SELECT t1.* FROM t1 LEFT JOIN t2 USING (c1)' failed: 1051: Unknown table 'test.t1' indeed, t1 is only present in context->table_list, but not in context->first_name_resolution_table. So after your fix the following statement will no longer work: INSERT INTO t2 SELECT t1.* FROM t1 JOIN t1 AS t USING (id1) WHERE id1=1 RETURNING *; it's because you set is_insert_or_replace_returning both for the SELECT and for the RETURNING clauses. > tables; > tables= (table_name ? tables->next_local : > tables->next_name_resolution_table) also this looks suspicious. Normally one starts from table_list and follows the next_local pointer or one starts from first_name_resolution_table and follows the next_name_resolution_table pointer. Your change creates a strange mix of both, it looks like it would be wrong. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 581857bd999: MDEV-22313: Neither SHOW CREATE USER nor SHOW GRANTS prints a user's default role
by Sergei Golubchik 23 Oct '20

23 Oct '20

Hi, Anel! Looks good, thanks! A couple of small comments below. On Oct 23, Anel Husakovic wrote: > revision-id: 581857bd999 (mariadb-10.1.43-314-g581857bd999) let's fix it in 10.2 and up, we're doing the very last 10.1 release, so better be extra-super-conservative about it. > parent(s): 43ec9370b32 > author: Anel Husakovic <anel(a)mariadb.org> > committer: Anel Husakovic <anel(a)mariadb.org> > timestamp: 2020-10-23 00:20:53 +0200 > message: > > MDEV-22313: Neither SHOW CREATE USER nor SHOW GRANTS prints a user's default role > > diff --git a/mysql-test/t/grant5.test b/mysql-test/t/grant5.test > index 74a69952124..bf97a930097 100644 > --- a/mysql-test/t/grant5.test > +++ b/mysql-test/t/grant5.test > @@ -52,6 +52,23 @@ disconnect u1; > drop user u1@localhost; > drop database mysqltest1; > > +# > +# MDEV-22313: Neither SHOW CREATE USER nor SHOW GRANTS prints a user's default role > +# > +CREATE ROLE test_role; > +CREATE USER test_user; > +GRANT test_role TO test_user; > +SET DEFAULT ROLE test_role FOR test_user; > +SHOW GRANTS FOR test_user; > +SET DEFAULT ROLE NONE for test_user; > +SHOW GRANTS FOR test_user; > +SET DEFAULT ROLE test_role; oops, sorry, my mistake. Please, add `SET ROLE test_role` before SHOW GRANTS. The goal is to see that SET DEFAULT ROLE is shown *after* all grants to root@localhost, but before any grants to test_role. > +SHOW GRANTS; > +SET DEFAULT ROLE NONE; > +SHOW GRANTS; > +DROP USER test_user; > +DROP ROLE test_role; > + > # > # End of 10.1 tests > # > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index cf0b1d87bd7..6c852976061 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -8001,15 +8007,44 @@ static ROLE_GRANT_PAIR *find_role_grant_pair(const LEX_STRING *u, > my_hash_search(&acl_roles_mappings, (uchar*)pair_key.ptr(), key_length); > } > > +static bool show_default_role(THD *thd, const char *hostname, > + ACL_USER *acl_entry, char *buff, size_t buffsize) > +{ > + Protocol *protocol= thd->protocol; > + LEX_STRING def_rolename= acl_entry->default_rolename; > + > + if (def_rolename.length) > + { > + String def_str(buff, buffsize, system_charset_info); > + def_str.length(0); > + def_str.append(STRING_WITH_LEN("SET DEFAULT ROLE ")); > + def_str.append(&def_rolename); > + def_str.append(" FOR '"); > + def_str.append(&acl_entry->user); > + DBUG_ASSERT(!(acl_entry->flags & IS_ROLE)); > + def_str.append(STRING_WITH_LEN("'@'")); > + def_str.append(acl_entry->host.hostname, acl_entry->hostname_length, > + system_charset_info); > + def_str.append('\''); > + protocol->prepare_for_resend(); > + protocol->store(def_str.ptr(),def_str.length(),def_str.charset()); > + if (protocol->write()) > + { > + return TRUE; > + } > + } > + return FALSE; looks good. The hostname argument seems to be unused now, doesn't it? > +} > + > -static bool show_role_grants(THD *thd, const char *username, > - const char *hostname, ACL_USER_BASE *acl_entry, > +static bool show_role_grants(THD *thd, const char *hostname, > + ACL_USER_BASE *acl_entry, > char *buff, size_t buffsize) > { > uint counter; > Protocol *protocol= thd->protocol; > LEX_STRING host= {const_cast<char*>(hostname), strlen(hostname)}; > > - String grant(buff,sizeof(buff),system_charset_info); > + String grant(buff, buffsize, system_charset_info); ouch, indeed. thanks! > for (counter= 0; counter < acl_entry->role_grants.elements; counter++) > { > grant.length(0); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 01612f594b3: MDEV-23769: MTR can abort before it prints the test result summary
by Sergei Golubchik 22 Oct '20

22 Oct '20

Hi, Anel! On Oct 22, Anel Husakovic wrote: > Hi Sergei, > thanks for the review. > > Having warning/error is a necessary condition. > Sufficient condition is to call at least 2 tests. > Incorrect behaviour can be seen if: > - Both tests are correct but there is error generated (incorrect option > passed): > So this `./mysql-test/mtr --mysqld=--lock-wait-timeout=-xx 1st` will > generate and write statistic at the end, > but this `./mysql-test/mtr --mysqld=--lock-wait-timeout=-xx 1st 1st > --force` will not generate the statistic. > Note: warning is generated (--mysqld=--lock-wait-timeout=-1) statistic > will be generated Okay, thanks. This would be useful to show in the commit comment, that one can use ./mtr --mysqld=--lock-wait-timeout=-xx 1st 1st --force --parallel 1 to repeat the error. But then I don't understand why this happens. Why one failure is not enough? Why --parallel 2 destroys the effect? Apparently, it's check_warnings_post_shutdown() that causes that, not exit(1). One can change to exit(0) or remove exit(1) completely - the bug will still happen. Why check_warnings_post_shutdown() writing to the master server causes this bug? It looks rather fishy that in your fix it's the worker process that prints the statistics, not the master. Btw, note that check_warnings_post_shutdown() doesn't have any return statement, it very implicitly return "number of bytes sent" - it would be easier to read if it'd return something explicitly. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0