March 2019 - developers - lists.mariadb.org

Re: [Maria-developers] c1bf5766aed: MDEV-16490: It's possible to make a system versioned table without any versioning field
by Sergei Golubchik 27 Mar '19

27 Mar '19

Hi, Nikita! On Mar 27, Nikita Malyavin wrote: > revision-id: c1bf5766aed (versioning-1.0.5-41-gc1bf5766aed) > parent(s): 141a5b24843 > author: Nikita Malyavin <nikitamalyavin(a)gmail.com> > committer: Nikita Malyavin <nikitamalyavin(a)gmail.com> > timestamp: 2018-07-20 23:37:26 +1000 > message: > > MDEV-16490: It's possible to make a system versioned table without any versioning field > > * do not allow versioned table to be without versioned (non-system) fields > * prohibit changing field versioning, when removing table versioning > > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index dbf400b53d7..ee4cf457f45 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -8473,11 +8473,21 @@ mysql_prepare_alter_table(THD *thd, TABLE *table, > } > } > > - if (table->versioned() && !(alter_info->flags & ALTER_DROP_SYSTEM_VERSIONING) && > - new_create_list.elements == VERSIONING_FIELDS) > + if (table->versioned()) > { > - my_error(ER_VERS_TABLE_MUST_HAVE_COLUMNS, MYF(0), table->s->table_name.str); > - goto err; > + uint versioned_fields= 0; > + field_it.rewind(); > + while (Create_field *f= field_it++) > + { > + if (!(f->flags & VERS_UPDATE_UNVERSIONED_FLAG)) > + versioned_fields++; > + } > + if (!(alter_info->flags & ALTER_DROP_SYSTEM_VERSIONING) && > + versioned_fields == VERSIONING_FIELDS) > + { > + my_error(ER_VERS_TABLE_MUST_HAVE_COLUMNS, MYF(0), table->s->table_name.str); > + goto err; > + } wouldn't that fit naturally into Vers_parse_info::fix_alter_info() ? Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] [Commits] e0fa1993692: MDEV-18899: Server crashes in Field::set_warning_truncated_wrong_value
by Sergey Petrunia 27 Mar '19

27 Mar '19

Hi Varun, First, Some input on the test: On Wed, Mar 27, 2019 at 01:55:16PM +0530, Varun wrote: > revision-id: e0fa1993692a3552feddae56cbee3078d4da2121 (mariadb-10.2.22-91-ge0fa1993692) > parent(s): 50a8fc52988d13a5164a1a542b9d7a85e3ecc1c1 > author: Varun Gupta > committer: Varun Gupta > timestamp: 2019-03-27 13:48:44 +0530 > message: > > MDEV-18899: Server crashes in Field::set_warning_truncated_wrong_value > > To fix the crash there we need to make sure that the > server while storing the statistical values in statistical tables should do it > in a multi-byte safe way. > Also there is no need to throw warnings if there is truncation while storing > values from statistical fields. > ... > --- a/mysql-test/r/stat_tables_innodb.result > +++ b/mysql-test/r/stat_tables_innodb.result > @@ -651,6 +651,63 @@ SELECT MAX(pk) FROM t1; > MAX(pk) > NULL > DROP TABLE t1; > +# > +# MDEV-18899: Server crashes in Field::set_warning_truncated_wrong_value > +# > +set names utf8; > +set @save_optimizer_use_condition_selectivity=@@optimizer_use_condition_selectivity; > +set optimizer_use_condition_selectivity=4; > +set use_stat_tables=preferably; > +set @save_histogram_size= @@histogram_size; > +set histogram_size=255; > +create table t1 ( a varchar(255) character set utf8); > +insert into t1 values (REPEAT('ӥ',255)), (REPEAT('ç',255)); > +analyze table t1; > +Table Op Msg_type Msg_text > +test.t1 analyze status Engine-independent statistics collected > +test.t1 analyze status OK > +select HEX(RIGHT(min_value, 1)), length(min_value) from mysql.column_stats; > +HEX(RIGHT(min_value, 1)) length(min_value) > +A7 254 > +select HEX(RIGHT(max_value, 1)), length(max_value) from mysql.column_stats; > +HEX(RIGHT(max_value, 1)) length(max_value) > +A5 254 > +analyze select * from t1 where a >= 'ӥ'; > +id select_type table type possible_keys key key_len ref rows r_rows filtered r_filtered Extra > +1 SIMPLE t1 ALL NULL NULL NULL NULL 2 2.00 50.00 50.00 Using where > +set @save_sql_mode= @@sql_mode; > +set sql_mode='ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION'; > +update mysql.column_stats set min_value= REPEAT('ӥ',255); This looks scary, what if there were some other data in these tables? Can you change add a WHERE db_name=database() and table_name='t1' ? > +Warnings: > +Warning 1265 Data truncated for column 'min_value' at row 1 > +select HEX(RIGHT(min_value, 1)), length(min_value) from mysql.column_stats; > +HEX(RIGHT(min_value, 1)) length(min_value) > +D3 255 > +analyze select * from t1 where a >= 'ӥ'; > +id select_type table type possible_keys key key_len ref rows r_rows filtered r_filtered Extra > +1 SIMPLE t1 ALL NULL NULL NULL NULL 2 2.00 50.00 50.00 Using where > +set names latin1; > +drop table t1; > +CREATE TABLE t1 (col1 date); > +INSERT INTO t1 VALUES('2004-01-01'),('2004-02-29'); > +INSERT INTO t1 VALUES('0000-10-31'); > +analyze table t1; > +Table Op Msg_type Msg_text > +test.t1 analyze status Engine-independent statistics collected > +test.t1 analyze status OK > +update mysql.column_stats set min_value='2004-0-31123'; The same as above. > +select min_value from mysql.column_stats; > +min_value > +2004-0-31123 This case also makes one wonder whether not producing a warning was a good idea after all. A varchar string that is 255-byte long, and after 255 bytes it gets truncated in the middle of a character is a rare case. An incorrect datetime value... not as rare? > +select * from t1; Is the above supposed to read the EITS data? It doesn't as the query doesn't use the WHERE clause. > +col1 > +2004-01-01 > +2004-02-29 > +0000-10-31 > +drop table t1; > +set @@sql_mode= @save_sql_mode; > set use_stat_tables=@save_use_stat_tables; > +set @@histogram_size= @save_histogram_size; > +set @@optimizer_use_condition_selectivity=@save_optimizer_use_condition_selectivity; > set optimizer_switch=@save_optimizer_switch_for_stat_tables_test; > SET SESSION STORAGE_ENGINE=DEFAULT; > diff --git a/mysql-test/t/stat_tables.test b/mysql-test/t/stat_tables.test > index b89ab2bbd2d..114c5c97a7e 100644 > --- a/mysql-test/t/stat_tables.test > +++ b/mysql-test/t/stat_tables.test > @@ -401,4 +401,44 @@ SELECT MAX(pk) FROM t1; > > DROP TABLE t1; > > +--echo # > +--echo # MDEV-18899: Server crashes in Field::set_warning_truncated_wrong_value > +--echo # > + > +set names utf8; > +set @save_optimizer_use_condition_selectivity=@@optimizer_use_condition_selectivity; > +set optimizer_use_condition_selectivity=4; > +set use_stat_tables=preferably; > +set @save_histogram_size= @@histogram_size; > +set histogram_size=255; > + > +create table t1 ( a varchar(255) character set utf8); > +insert into t1 values (REPEAT('ӥ',255)), (REPEAT('ç',255)); > + > +analyze table t1; > +select HEX(RIGHT(min_value, 1)), length(min_value) from mysql.column_stats; > +select HEX(RIGHT(max_value, 1)), length(max_value) from mysql.column_stats; > +analyze select * from t1 where a >= 'ӥ'; > + > +set @save_sql_mode= @@sql_mode; > +set sql_mode='ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION'; > +update mysql.column_stats set min_value= REPEAT('ӥ',255); > +select HEX(RIGHT(min_value, 1)), length(min_value) from mysql.column_stats; > +analyze select * from t1 where a >= 'ӥ'; > + > +set names latin1; > +drop table t1; > + > +CREATE TABLE t1 (col1 date); > +INSERT INTO t1 VALUES('2004-01-01'),('2004-02-29'); > +INSERT INTO t1 VALUES('0000-10-31'); > +analyze table t1; > +update mysql.column_stats set min_value='2004-0-31123'; > +select min_value from mysql.column_stats; > +select * from t1; > +drop table t1; > + > +set @@sql_mode= @save_sql_mode; > set use_stat_tables=@save_use_stat_tables; > +set @@histogram_size= @save_histogram_size; > +set @@optimizer_use_condition_selectivity=@save_optimizer_use_condition_selectivity; > diff --git a/sql/field.cc b/sql/field.cc > index 080cf34c76d..c26195a3650 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -2219,7 +2219,6 @@ bool Field_str::can_be_substituted_to_equal_item(const Context &ctx, > return false; > } > > - > void Field_num::make_field(Send_field *field) > { > Field::make_field(field); > @@ -7023,12 +7022,16 @@ Field_longstr::check_string_copy_error(const String_copier *copier, > { > const char *pos; > char tmp[32]; > + THD *thd= get_thd(); > > if (!(pos= copier->most_important_error_pos())) > return FALSE; > > - convert_to_printable(tmp, sizeof(tmp), pos, (end - pos), cs, 6); > - set_warning_truncated_wrong_value("string", tmp); > + if (thd->count_cuted_fields) > + { > + convert_to_printable(tmp, sizeof(tmp), pos, (end - pos), cs, 6); > + set_warning_truncated_wrong_value("string", tmp); > + } > return TRUE; > } > > diff --git a/sql/sql_statistics.cc b/sql/sql_statistics.cc > index b5811c683e8..0960a9a1ec5 100644 > --- a/sql/sql_statistics.cc > +++ b/sql/sql_statistics.cc > @@ -1044,6 +1044,7 @@ class Column_stat: public Stat_table > { > char buff[MAX_FIELD_WIDTH]; > String val(buff, sizeof(buff), &my_charset_bin); > + uint32 length= 0; > > for (uint i= COLUMN_STAT_MIN_VALUE; i <= COLUMN_STAT_HISTOGRAM; i++) > { > @@ -1060,7 +1061,9 @@ class Column_stat: public Stat_table > else > { > table_field->collected_stats->min_value->val_str(&val); > - stat_field->store(val.ptr(), val.length(), &my_charset_bin); > + length= Well_formed_prefix(val.charset(), val.ptr(), > + MY_MIN(val.length(), stat_field->field_length)).length(); > + stat_field->store(val.ptr(), length, &my_charset_bin); > } > break; > case COLUMN_STAT_MAX_VALUE: > @@ -1069,7 +1072,9 @@ class Column_stat: public Stat_table > else > { > table_field->collected_stats->max_value->val_str(&val); > - stat_field->store(val.ptr(), val.length(), &my_charset_bin); > + length= Well_formed_prefix(val.charset(), val.ptr(), > + MY_MIN(val.length(), stat_field->field_length)).length(); > + stat_field->store(val.ptr(), length, &my_charset_bin); > } > break; > case COLUMN_STAT_NULLS_RATIO: > @@ -2934,7 +2939,6 @@ int update_statistics_for_table(THD *thd, TABLE *table) > DBUG_RETURN(rc); > } > > - > /** > @brief > Read statistics for a table from the persistent statistical tables > @@ -2980,6 +2984,7 @@ int read_statistics_for_table(THD *thd, TABLE *table, TABLE_LIST *stat_tables) > Table_statistics *read_stats= table_share->stats_cb.table_stats; > > DBUG_ENTER("read_statistics_for_table"); > + DBUG_ASSERT(thd->count_cuted_fields == CHECK_FIELD_IGNORE); > > /* Read statistics from the statistical table table_stats */ > stat_table= stat_tables[TABLE_STAT].table; > @@ -3059,7 +3064,7 @@ int read_statistics_for_table(THD *thd, TABLE *table, TABLE_LIST *stat_tables) > } > } > } > - > + > table->stats_is_read= TRUE; > > DBUG_RETURN(0); BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] 494c0448c87: MDEV-15951 system versioning by trx id doesn't work with partitioning
by Sergei Golubchik 26 Mar '19

26 Mar '19

Hi, Nikita! That's good, thanks! It's always nice to see a fix that removes more code than it adds :) But I'll reformulate the error message a bit, "Transactional system versioned tables" also means InnoDB system versioned by timestamp. I'll clarify (somehow) that it means versioned by trx_id, and push. On Mar 26, Nikita Malyavin wrote: > revision-id: 494c0448c87 (mariadb-10.3.12-65-g494c0448c87) > parent(s): 4946eb7b543 > author: Nikita Malyavin <nikitamalyavin(a)gmail.com> > committer: Nikita Malyavin <nikitamalyavin(a)gmail.com> > timestamp: 2019-02-28 19:29:12 +1000 > message: > > MDEV-15951 system versioning by trx id doesn't work with partitioning > > Fix partitioning for trx_id-versioned tables. > `partition by hash`, `range` and others now work. > `partition by system_time` is forbidden. > Currently we cannot use row_start and row_end in `partition by`, because > insertion of versioned field is done by engine's handler, as well as > row_start/row_end's value set up, which is a transaction id -- so it's > also forbidden. > > The drawback is that it's now impossible to use `partition by key()` > without parameters for such tables, because it references row_start and > row_end implicitly. > > * add handler::vers_can_native() > * drop Table_scope_and_contents_source_st::vers_native() > * drop partition_element::find_engine_flag as unused > * forbid versioning partitioning for trx_id as not supported > * adopt vers tests for trx_id partitioning > * forbid any row_end referencing in `partition by` clauses, > including implicit `by key()` > > diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt > index 9112795b754..dff12ef0a97 100644 > --- a/sql/share/errmsg-utf8.txt > +++ b/sql/share/errmsg-utf8.txt > @@ -7907,8 +7907,8 @@ ER_UNUSED_24 > ER_VERS_TEMPORARY > eng "TEMPORARY tables do not support system versioning" > > -ER_VERS_NOT_SUPPORTED > - eng "%s is not supported for %s system-versioned tables" > +ER_VERS_TRX_PART_HISTORIC_ROW_NOT_SUPPORTED > + eng "Transactional system versioned tables do not support partitioning by ROW START or ROW END" > ER_INDEX_FILE_FULL > eng "The index file for table '%-.192s' is full" Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] [Commits] 842ef80b029: MDEV-18464: Port kill_one_trx fixes from 10.4 to 10.1
by Marko Mäkelä 26 Mar '19

26 Mar '19

Hi Jan, On Fri, Mar 22, 2019 at 1:59 PM jan <jan.lindstrom(a)mariadb.com> wrote: > > revision-id: 842ef80b029c975b7f7f7ec25fe55817ec626f02 (mariadb-10.1.38-68-g842ef80b029) Was this pushed to buildbot as well? I did not find the commit ID. First, some general comments: * The functions trx_start_if_not_started_low() and trx_start_for_ddl_low() differ between XtraDB and InnoDB with regard to #ifdef WITH_WSREP. Why? Are the debug assignments truly necessary? I see that the difference was introduced 5 years ago in MDEV-6247. * The files storage/{innobase,xtradb}/trx/trx0roll.cc differ from each other after the change. It looks like the innobase version is better. Please remove the unnecessary lines from xtradb: --- storage/xtradb/trx/trx0roll.cc 2019-03-26 10:20:31.767971537 +0200 +++ storage/innobase/trx/trx0roll.cc 2019-03-26 10:20:31.759971465 +0200 @@ -33,8 +33,6 @@ #include "trx0roll.ic" #endif -#include <mysql/service_wsrep.h> - #include "fsp0fsp.h" #include "mach0data.h" #include "trx0rseg.h" @@ -51,9 +49,6 @@ #include "pars0pars.h" #include "srv0mon.h" #include "trx0sys.h" -#ifdef WITH_WSREP -#include "ha_prototypes.h" -#endif /* WITH_WSREP */ /** This many pages must be undone before a truncate is tried within rollback */ @@ -1075,12 +1070,6 @@ if (trx->update_undo) { trx_undo_truncate_end(trx, trx->update_undo, limit); } - -#ifdef WITH_WSREP_OUT - if (wsrep_on(trx->mysql_thd)) { - trx->lock.was_chosen_as_deadlock_victim = FALSE; - } -#endif /* WITH_WSREP */ } /***********************************************************************//** On to the detailed review: > diff --git a/storage/innobase/include/trx0trx.h b/storage/innobase/include/trx0trx.h > index fe16b8272b8..b83734b154a 100644 > --- a/storage/innobase/include/trx0trx.h > +++ b/storage/innobase/include/trx0trx.h > @@ -1,7 +1,7 @@ > /***************************************************************************** > > Copyright (c) 1996, 2016, Oracle and/or its affiliates. All Rights Reserved. > -Copyright (c) 2015, 2018, MariaDB Corporation. > +Copyright (c) 2015, 2019, MariaDB Corporation. > > 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 > @@ -613,6 +613,9 @@ struct trx_lock_t { > transaction as a victim in deadlock > resolution, it sets this to TRUE. > Protected by trx->mutex. */ > + bool was_chosen_as_replication_victim; > + /* replication has marked this > + trx to abort */ > time_t wait_started; /*!< lock wait started at this time, > protected only by lock_sys->mutex */ > > @@ -624,6 +627,12 @@ struct trx_lock_t { > only be modified by the thread that is > serving the running transaction. */ > > +#ifdef WITH_WSREP > + bool was_chosen_as_wsrep_victim; > + /*!< high priority wsrep thread has > + marked this trx to abort */ > +#endif /* WITH_WSREP */ > + > mem_heap_t* lock_heap; /*!< memory heap for trx_locks; > protected by lock_sys->mutex */ > > @@ -695,14 +704,6 @@ lock_rec_convert_impl_to_expl()) will access transactions associated > to other connections. The locks of transactions are protected by > lock_sys->mutex and sometimes by trx->mutex. */ > > -enum trx_abort_t { > - TRX_SERVER_ABORT = 0, > -#ifdef WITH_WSREP > - TRX_WSREP_ABORT, > -#endif > - TRX_REPLICATION_ABORT > -}; > - > struct trx_t{ > ulint magic_n; > > @@ -880,8 +881,6 @@ struct trx_t{ > /*------------------------------*/ > THD* mysql_thd; /*!< MySQL thread handle corresponding > to this trx, or NULL */ > - trx_abort_t abort_type; /*!< Transaction abort type*/ > - > const char* mysql_log_file_name; > /*!< if MySQL binlog is used, this field > contains a pointer to the latest file Why are you replacing trx_t::abort_type with two fields in trx_lock_t that are located far apart from each other? To avoid changing the performance too much due to caching, I would suggest to introduce the two fields in trx_t, replacing the former location of abort_type. Do we really need such long names? Would trx_t::replication_victim and trx_t::wsrep_victim suffice? Please document how concurrent access to the fields is being protected. Apparently for the WSREP field it is both trx->mutex and lock_sys->mutex. > diff --git a/storage/innobase/lock/lock0lock.cc b/storage/innobase/lock/lock0lock.cc > index 3970a559a4c..984ac6cacbc 100644 > --- a/storage/innobase/lock/lock0lock.cc > +++ b/storage/innobase/lock/lock0lock.cc > @@ -4785,9 +4783,8 @@ lock_report_waiters_to_mysql( > innobase_kill_query. We mark this by setting > current_lock_mutex_owner, so we can avoid trying > to recursively take lock_sys->mutex. */ > - w_trx->abort_type = TRX_REPLICATION_ABORT; > + w_trx->lock.was_chosen_as_replication_victim = true; > thd_report_wait_for(mysql_thd, w_trx->mysql_thd); > - w_trx->abort_type = TRX_SERVER_ABORT; > } > ++i; > } Here, we seem to be only holding lock_sys->mutex, not w_trx->mutex. > @@ -7993,6 +7981,37 @@ lock_trx_handle_wait( > return DB_LOCK_WAIT; > } > > +/*********************************************************************//** > +Check whether the transaction has already been rolled back because it > +was selected as a deadlock victim, or if it has to wait then cancel > +the wait lock. > +@return DB_DEADLOCK, DB_LOCK_WAIT or DB_SUCCESS */ > +UNIV_INTERN > +dberr_t > +lock_trx_handle_wait( > +/*=================*/ > + trx_t* trx) /*!< in/out: trx lock state */ > +{ > + bool lock_mutex_taken = false; Do we really need this variable? > +#ifdef WITH_WSREP > + /* We already own mutexes */ > + if (trx->lock.was_chosen_as_wsrep_victim) { > + return lock_trx_handle_wait_low(trx); > + } > +#endif /* WITH_WSREP */ > + if (!trx->lock.was_chosen_as_replication_victim) { > + lock_mutex_enter(); > + lock_mutex_taken = true; > + } > + trx_mutex_enter(trx); > + dberr_t err = lock_trx_handle_wait_low(trx); > + if (lock_mutex_taken) { > + lock_mutex_exit(); > + } > + trx_mutex_exit(trx); > + return err; > +} I would expand the second part of the code and remove the condition before releasing the 2 mutexes: if (!trx->lock.was_chosen_as_replication_victim) { lock_mutex_enter(); trx_mutex_enter(trx); dberr_t err = lock_trx_handle_wait_low(trx); lock_mutex_exit(); trx_mutex_exit(trx); return err; } else { trx_mutex_enter(trx); dberr_t err = lock_trx_handle_wait_low(trx); trx_mutex_exit(trx); return err; } Also, I think that it would be better to declare lock_trx_handle_wait_low() only static but not inline, to reduce unnecessary code expansion. Best regards, Marko

1 0

Re: [Maria-developers] 0fa832dc426: Revert THD::THD(skip_global_sys_var_lock) argument
by Sergei Golubchik 25 Mar '19

25 Mar '19

Hi, Sergey! ok to push On Mar 25, Sergey Vojtovich wrote: > revision-id: 0fa832dc426 (mariadb-10.3.12-97-g0fa832dc426) > parent(s): 6f14489ad31 > author: Sergey Vojtovich <svoj(a)mariadb.org> > committer: Sergey Vojtovich <svoj(a)mariadb.org> > timestamp: 2019-03-21 17:17:26 +0400 > message: > > Revert THD::THD(skip_global_sys_var_lock) argument > > Originally introduced by e972125f1 to avoid harmless wait for > LOCK_global_system_variables in a newly created thread, which creation was > initiated by system variable update. > > At the same time it opens dangerous hole, when system variable update > thread already released LOCK_global_system_variables and ack_receiver > thread haven't yet completed new THD construction. In this case THD > constructor goes completely unprotected. > > Since ack_receiver.stop() waits for the thread to go down, we have to > temporarily release LOCK_global_system_variables so that it doesn't > deadlock with ack_receiver.run(). Unfortunately it breaks atomicity > of rpl_semi_sync_master_enabled updates and makes them not serialized. > > LOCK_rpl_semi_sync_master_enabled was introduced to workaround the above. > TODO: move ack_receiver start/stop into repl_semisync_master > enable_master/disable_master under LOCK_binlog protection? > > Part of MDEV-14984 - regression in connect performance > Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] 6f14489ad31: Adieu find_sys_var_ex()
by Sergei Golubchik 25 Mar '19

25 Mar '19

Hi, Sergey! ok to push On Mar 25, Sergey Vojtovich wrote: > revision-id: 6f14489ad31 (mariadb-10.3.12-96-g6f14489ad31) > parent(s): 48af6851da2 > author: Sergey Vojtovich <svoj(a)mariadb.org> > committer: Sergey Vojtovich <svoj(a)mariadb.org> > timestamp: 2019-03-21 01:32:04 +0400 > message: > > Adieu find_sys_var_ex() > > Only take LOCK_plugin for plugin system variables. > > Reverted optimisation that was originally done for session tracker: it > makes much less sense now. Specifically only if connections would want to > track plugin session variables changes and these changes would actually > happen frequently. If this ever becomes an issue, there're much better > ways to optimise this workload. > > Part of MDEV-14984 - regression in connect performance > Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] 48af6851da2: Make connect speed great again
by Sergei Golubchik 25 Mar '19

25 Mar '19

Hi, Sergey! On Mar 25, Sergey Vojtovich wrote: > revision-id: 48af6851da2 (mariadb-10.3.12-95-g48af6851da2) > parent(s): a83d6ee812b > author: Sergey Vojtovich <svoj(a)mariadb.org> > committer: Sergey Vojtovich <svoj(a)mariadb.org> > timestamp: 2019-03-20 01:58:42 +0400 > message: > > Make connect speed great again > > Rather than parsing session_track_system_variables when thread starts, do > it when first trackable event occurs. > > Benchmarked on a 2socket/20core/40threads Broadwell system using sysbench > connect brencmark @40 threads (with select 1 disabled): > 101379.77 -> 143016.68 CPS, whereas 10.2 is currently at 137766.31 CPS. > > Part of MDEV-14984 - regression in connect performance > > diff --git a/sql/session_tracker.cc b/sql/session_tracker.cc > index 5d98e58046d..c493c790596 100644 > --- a/sql/session_tracker.cc > +++ b/sql/session_tracker.cc > @@ -378,16 +378,9 @@ bool Session_sysvars_tracker::configure() > > bool Session_sysvars_tracker::enable(THD *thd) > { > - LEX_STRING tmp= { session_track_system_variables, > - safe_strlen(session_track_system_variables) }; > orig_list.reinit(); > - if (orig_list.parse_var_list(thd, tmp, true, thd->charset()) == true) > - { > - orig_list.reinit(); > - m_enabled= false; > - return true; > - } > - m_enabled= true; > + m_parsed= false; > + m_enabled= session_track_system_variables && *session_track_system_variables; wouldn't it be simpler not to allow both NULL and "" as valid values for session_track_system_variables ? Either always store the empty string as NULL or as "", why would you want both? > return false; > } > > @@ -530,6 +524,19 @@ void Session_sysvars_tracker::mark_as_changed(THD *thd, > { > sysvar_node_st *node; > sys_var *svar= (sys_var *)var; > + > + if (!m_parsed) > + { > + LEX_STRING tmp= { session_track_system_variables, > + safe_strlen(session_track_system_variables) }; > + if (orig_list.parse_var_list(thd, tmp, true, thd->charset())) > + { > + orig_list.reinit(); > + return; > + } > + m_parsed= true; This is of somewhat dubious value. The default value for the @@session_track_system_variables is DEFAULT("autocommit,character_set_client,character_set_connection," "character_set_results,time_zone"), and these are variables that are typically set by connectors automatically on every new connection. Only a pure C/C will benefit from it, that's what sysbench shows. If you'd tested with java, you, probably, wouldn't get any speedup. > + } > + > /* > Check if the specified system variable is being tracked, if so > mark it as changed and also set the class's m_changed flag. > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index 7194bbd03ef..53203d15f0b 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -7320,8 +7320,13 @@ void THD::set_last_commit_gtid(rpl_gtid &gtid) > #ifndef EMBEDDED_LIBRARY > if (changed_gtid && session_tracker.session_sysvars_tracker.is_enabled()) > { > + THD *orig_thd= current_thd; > + if (orig_thd != this) When can it be true? > + set_current_thd(this); > session_tracker.session_sysvars_tracker. > mark_as_changed(this, (LEX_CSTRING*)Sys_last_gtid_ptr); > + if (orig_thd != this) > + set_current_thd(orig_thd); > } > #endif > } > Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] a83d6ee812b: Safe session_track_system_variables snapshot
by Sergei Golubchik 25 Mar '19

25 Mar '19

Hi, Sergey! Just one question. Why do you need Session_sysvars_tracker::session_track_system_variables if you have THD::system_variables::session_track_system_variables ? On Mar 25, Sergey Vojtovich wrote: > revision-id: a83d6ee812b (mariadb-10.3.12-94-ga83d6ee812b) > parent(s): 3c6f17a130b > author: Sergey Vojtovich <svoj(a)mariadb.org> > committer: Sergey Vojtovich <svoj(a)mariadb.org> > timestamp: 2019-03-19 23:40:08 +0400 > message: > > Safe session_track_system_variables snapshot > > When enabling system variables tracker, make a copy of > global_system_variables.session_track_system_variables > under LOCK_global_system_variables. This protects from concurrent variable > updates and potential freed memory access, as well as from variable > reconstruction (which was previously protected by LOCK_plugin). > > We can also use this copy as a session variable pointer, so that we don't > have to allocate memory and reconstruct it every time it is referenced. > > For this very reason we don't need buffer_length stuff anymore. > > As well as don't we need to take LOCK_plugin early in ::enable(). > Unified ::parse_var_list() to acquire LOCK_plugin unconditionally. > For no apparent reason it wasn't previously acquired for global > variable update. > > No reason to access session_sysvars_tracker via get_tracker(), so access > it directly instead. > > Part of MDEV-14984 - regression in connect performance Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] 3c6f17a130b: Allocate Session_sysvars_tracker statically
by Sergei Golubchik 25 Mar '19

25 Mar '19

Hi, Sergey! Please, see few questions below On Mar 25, Sergey Vojtovich wrote: > revision-id: 3c6f17a130b (mariadb-10.3.12-93-g3c6f17a130b) > parent(s): db9854b7d48 > author: Sergey Vojtovich <svoj(a)mariadb.org> > committer: Sergey Vojtovich <svoj(a)mariadb.org> > timestamp: 2019-03-19 23:40:07 +0400 > message: > > Allocate Session_sysvars_tracker statically > > One less new/delete per connection. > > Removed m_mem_flag since most allocs are thread specific. The only > exception are allocs performed during initialization. > > Removed State_tracker and Session_tracker constructors as they don't make > sense anymore. > > Rather than having session_tracker.deinit(), fixed memory accounting so > that it gets initialized before any other constructor and deinitialized > after all destructors done. Cons: makes THD a few pointers larger :( > > Part of MDEV-14984 - regression in connect performance > > diff --git a/sql/sql_class.h b/sql/sql_class.h > index 69e6f58f854..64d772e5d17 100644 > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -2145,13 +2145,34 @@ struct wait_for_commit > > extern "C" void my_message_sql(uint error, const char *str, myf MyFlags); > > + > +/** > + A wrapper around thread_count. > + > + It must be specified as a first base class of THD, so that increment is > + done before any other THD constructors and decrement - after any other THD > + destructors. > + > + Destructor unblocks close_conneciton() if there are no more THD's left. > +*/ > +class THD_count > +{ > + THD *orig_thd; > + THD_count(); > + ~THD_count(); > + friend class THD; > +}; 1. Why a separate class, what's wrong with having it in THD? No downcasts, no risk that someone would try to use it separately. 2. orig_thd looks very suspicious. You preserve the old THD* pointer for the whole duration of this THD lifetime and restore it at the end. How can you know that the "orig_thd" is still valid? Why do you need to do it anyway? > + > + > /** > @class THD > For each client connection we create a separate thread with THD serving as > a thread/connection descriptor > */ > > -class THD :public Statement, > +class THD: public THD_count, /* this must be first */ an assert would be better than "/* must be first */". Like, assert(&orig_thd == this) or something. > + public Statement, > + > /* > This is to track items changed during execution of a prepared > statement/stored procedure. It's created by > @@ -2176,19 +2197,6 @@ class THD :public Statement, > inline bool is_conventional() const > { DBUG_ASSERT(0); return Statement::is_conventional(); } > > - void dec_thread_count(void) > - { > - DBUG_ASSERT(thread_count > 0); > - thread_safe_decrement32(&thread_count); > - signal_thd_deleted(); > - } > - > - > - void inc_thread_count(void) > - { > - thread_safe_increment32(&thread_count); > - } > - > public: > MDL_context mdl_context; > > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index 9c9fa228e2a..afe4123a52c 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -597,8 +597,46 @@ extern "C" void thd_kill_timeout(THD* thd) > thd->awake(KILL_TIMEOUT); > } > > -THD::THD(my_thread_id id, bool is_wsrep_applier, bool skip_global_sys_var_lock) > - :Statement(&main_lex, &main_mem_root, STMT_CONVENTIONAL_EXECUTION, > + > +THD_count::THD_count() > +{ > + THD *thd= static_cast<THD*>(this); > + thread_safe_increment32(&thread_count); > + /* > + We set THR_THD to temporally point to this THD to register all the > + variables that allocates memory for this THD > + */ > + orig_thd= current_thd; > + set_current_thd(thd); > + thd->status_var.local_memory_used= sizeof(THD); > + thd->status_var.max_local_memory_used= thd->status_var.local_memory_used; > + thd->status_var.global_memory_used= 0; > + thd->variables.max_mem_used= global_system_variables.max_mem_used; > +} > + > + > +THD_count::~THD_count() > +{ > + THD *thd= static_cast<THD*>(this); > + if (thd->status_var.local_memory_used != 0) > + { > + DBUG_PRINT("error", ("memory_used: %lld", > + thd->status_var.local_memory_used)); > + SAFEMALLOC_REPORT_MEMORY(thd->thread_id); > + DBUG_ASSERT(thd->status_var.local_memory_used == 0 || > + !debug_assert_on_not_freed_memory); > + } > + update_global_memory_status(thd->status_var.global_memory_used); > + set_current_thd(orig_thd); > + DBUG_ASSERT(thread_count > 0); > + thread_safe_decrement32(&thread_count); > + signal_thd_deleted(); > +} > + > + > +THD::THD(my_thread_id id, bool is_wsrep_applier, bool skip_global_sys_var_lock): > + THD_count(), > + Statement(&main_lex, &main_mem_root, STMT_CONVENTIONAL_EXECUTION, > /* statement id */ 0), > rli_fake(0), rgi_fake(0), rgi_slave(NULL), > protocol_text(this), protocol_binary(this), > @@ -654,15 +692,6 @@ THD::THD(my_thread_id id, bool is_wsrep_applier, bool skip_global_sys_var_lock) > ulong tmp; > bzero(&variables, sizeof(variables)); > > - /* > - We set THR_THD to temporally point to this THD to register all the > - variables that allocates memory for this THD > - */ > - THD *old_THR_THD= current_thd; > - set_current_thd(this); > - status_var.local_memory_used= sizeof(THD); > - status_var.max_local_memory_used= status_var.local_memory_used; > - status_var.global_memory_used= 0; > variables.pseudo_thread_id= thread_id; > variables.max_mem_used= global_system_variables.max_mem_used; > main_da.init(); > @@ -849,8 +878,7 @@ THD::THD(my_thread_id id, bool is_wsrep_applier, bool skip_global_sys_var_lock) > save_prep_leaf_list= FALSE; > org_charset= 0; > /* Restore THR_THD */ > - set_current_thd(old_THR_THD); > - inc_thread_count(); > + set_current_thd(orig_thd); > } > > > @@ -1589,7 +1617,6 @@ void THD::reset_for_reuse() > > THD::~THD() > { > - THD *orig_thd= current_thd; > THD_CHECK_SENTRY(this); > DBUG_ENTER("~THD()"); > /* Check that we have already called thd->unlink() */ > @@ -1601,7 +1628,11 @@ THD::~THD() > In error cases, thd may not be current thd. We have to fix this so > that memory allocation counting is done correctly > */ > - set_current_thd(this); > + orig_thd= current_thd; > + if (orig_thd == this) > + orig_thd= 0; > + else > + set_current_thd(this); > if (!status_in_global) > add_status_to_global(); > > @@ -1655,22 +1686,6 @@ THD::~THD() > lf_hash_put_pins(xid_hash_pins); > /* Ensure everything is freed */ > status_var.local_memory_used-= sizeof(THD); > - > - /* trick to make happy memory accounting system */ > -#ifndef EMBEDDED_LIBRARY > - session_tracker.deinit(); > -#endif //EMBEDDED_LIBRARY > - > - if (status_var.local_memory_used != 0) > - { > - DBUG_PRINT("error", ("memory_used: %lld", status_var.local_memory_used)); > - SAFEMALLOC_REPORT_MEMORY(thread_id); > - DBUG_ASSERT(status_var.local_memory_used == 0 || > - !debug_assert_on_not_freed_memory); > - } > - update_global_memory_status(status_var.global_memory_used); > - set_current_thd(orig_thd == this ? 0 : orig_thd); > - dec_thread_count(); > DBUG_VOID_RETURN; > } Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] 72e4e57a52a: Cleanup session tracker API
by Sergei Golubchik 25 Mar '19

25 Mar '19

Hi, Sergey! ok to push On Mar 25, Sergey Vojtovich wrote: > revision-id: 72e4e57a52a (mariadb-10.3.12-88-g72e4e57a52a) > parent(s): 68abb07298b > author: Sergey Vojtovich <svoj(a)mariadb.org> > committer: Sergey Vojtovich <svoj(a)mariadb.org> > timestamp: 2019-03-19 17:11:16 +0400 > message: > > Cleanup session tracker API > > - Session_sysvars_tracker::server_init_check() -> sysvartrack_validate_value() > - Session_sysvars_tracker::check_var_list() -> sysvartrack_validate_value() > > - Session_sysvars_tracker::server_init_process() -> sysvartrack_global_update() > - sysvartrack_reprint_value() -> sysvartrack_global_update() > > - sysvartrack_value_len() -> sysvartrack_session_value_ptr() > - sysvartrack_value_construct() -> sysvartrack_session_value_ptr() > > - sysvartrack_update() -> Session_sysvars_tracker::update() > - Session_tracker::server_boot_verify() -> session_tracker_init() > - sysvar_tracker() -> /dev/null > > Part of MDEV-14984 - regression in connect performance Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0