October 2020 - developers - lists.mariadb.org

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

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

22 Oct '20

Hi, Anel! On Oct 22, Anel Husakovic wrote: > revision-id: 99c14476251 (mariadb-10.1.43-314-g99c14476251) > parent(s): 43ec9370b32 > author: Anel Husakovic <anel(a)mariadb.org> > committer: Anel Husakovic <anel(a)mariadb.org> > timestamp: 2020-10-22 16:49:28 +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..8756648c3e1 100644 > --- a/mysql-test/t/grant5.test > +++ b/mysql-test/t/grant5.test > @@ -53,5 +53,19 @@ drop user u1@localhost; > drop database mysqltest1; > > # > -# End of 10.1 tests > +# 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; > +SHOW GRANTS; The point is to run `SHOW GRANTS` for a user with a default role. E.g. SET DEFAULT ROLE test_role; SHOW GRANTS; SET DEFAULT ROLE NONE; > +DROP USER test_user; > +DROP ROLE test_role; > + > # > +# End of 10.1 tests > +# > \ No newline at end of file new line at the end of file? > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index cf0b1d87bd7..f42f7bdcfaf 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -8001,6 +8007,36 @@ 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) > +{ > + Protocol *protocol= thd->protocol; > + LEX_STRING host= {const_cast<char*>(hostname), strlen(hostname)}; > + > + LEX_STRING def_rolename= acl_entry->default_rolename; > + if (def_rolename.length) > + { > + String def_str(buff,sizeof(buff),system_charset_info); eh? sizeof(buff) is sizeof(char*) that is usually 8 on 64-bit, 4 on 32-bit architecture. You want the size of the buffer here, not the size of the pointer to the buffer. > + 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.str, acl_entry->user.length, > + system_charset_info); again, just as above, you can use def_str.append(&acl_entry->user); > + DBUG_ASSERT(!(acl_entry->flags & IS_ROLE)); > + def_str.append(STRING_WITH_LEN("'@'")); > + def_str.append(&host); why not to use 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; > +} Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

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

22 Oct '20

Hi, Anel! On Oct 22, Anel Husakovic wrote: > Hi Sergei, > thank you for your review. > How is easier for you (or to ask a general question: how does the review > process go?) regarding the commits? > Is it better to add a new commit as a review commit and if the patch gets > accepted to squash after, or to rebase commits and send only a single > commit again for the review? One single commit is better, thanks. At least for small patches. > Addressed in the new patch: > https://github.com/MariaDB/server/commit/99c144762514d4dea3b6ec395de96aaf10… Thanks! I'll reply to the commit directly. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 83614913d9a: MDEV-16243 [Warning] "Versioned table ...: partition ... is full, add more HISTORY partitions" floods the error log
by Sergei Golubchik 22 Oct '20

22 Oct '20

Hi, Aleksey! On Oct 22, Aleksey Midenkov wrote: > revision-id: 83614913d9a (mariadb-10.4.11-291-g83614913d9a) > parent(s): efb1023b6f4 > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Sergei Golubchik <serg(a)mariadb.com> > timestamp: 2020-10-22 16:44:13 +0200 > message: > > MDEV-16243 [Warning] "Versioned table ...: partition ... is full, add more HISTORY partitions" floods the error log > > diff --git a/sql/partition_info.cc b/sql/partition_info.cc > index 8989a918c0c..f287cfddcd2 100644 > --- a/sql/partition_info.cc > +++ b/sql/partition_info.cc > @@ -879,9 +879,12 @@ void partition_info::vers_set_hist_part(THD *thd) > } > return; > warn: > - my_error(WARN_VERS_PART_FULL, MYF(ME_WARNING|ME_ERROR_LOG), > - table->s->db.str, table->s->table_name.str, > - vers_info->hist_part->partition_name); > + if (vers_info->hist_part->warn_vers_part_full) > + { > + my_error(WARN_VERS_PART_FULL, MYF(ME_WARNING|ME_ERROR_LOG), > + table->s->db.str, table->s->table_name.str, > + vers_info->hist_part->partition_name); > + } would it make sense to put warn_vers_part_full into vers_info, instead of storing it per-partition? > } > > > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0