- developers - lists.mariadb.org

Re: [Maria-developers] ab6ef429a84: MDEV-16370 row-based binlog events (updates by primary key) can not be applied multiple times to system versioned tables
by Sergei Golubchik 29 Mar '19

29 Mar '19

Hi, Aleksey! On Jan 31, Aleksey Midenkov wrote: > revision-id: ab6ef429a84 (versioning-1.0.7-2-gab6ef429a84) > parent(s): 14c2f90ad08 > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Aleksey Midenkov <midenok(a)gmail.com> > timestamp: 2018-11-26 12:46:37 +0300 > message: > > MDEV-16370 row-based binlog events (updates by primary key) can not be applied multiple times to system versioned tables > > Case 2: > > * Idempotent BINLOG updates: duplicate key on history row is skipped; > * Fix row start and warn when applying BINLOG from the past. As far as I can see now you cannot just arbitrarily adjust row_start. Think of it in term or application time period tables, a primary key is (kind of) defined WITHOUT OVERLAPS. There is no explicit constraint, it happens automatically when the history is valid. Now, here's your test case: timestamp=100; insert (1,1) -> the table has (1,1,100,MAX); timestamp=300; update to (1,3) -> (1,1,100,300),(1,3,300,MAX); timestamp=200; update to (1,2) -> here the hisory row should've been (1,3,300,200) but you adjust it to get 1,1,100,300 1,3,199,200 1,2,200,MAX and if you select AS OF 199, you'll get two rows for the pk=1 So, let's * ignore HA_ERR_FOUND_DUPP_KEY (please check if it's needed for insert or delete too) * add TIMESTAMP=xxx to mysqldumps. can vers_insert_history_row see row_start>row_end in a normal use case? no replication or any other way of adjusting the timestamp. equality can surely happen, but greater than ? Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 1

[Maria-developers] THD::set_time()
by Aleksey Midenkov 29 Mar '19

29 Mar '19

What if system_time.sec_part == TIME_MAX_SECOND_PART here: else { start_time= t; start_time_sec_part= ++system_time.sec_part; } ? -- All the best, Aleksey Midenkov @midenok

1 0

[Maria-developers] (no subject)
by Samyak Jain 28 Mar '19

28 Mar '19

I want to contribute for full outer join gsoc project of mariaDB. Please provide the technical requirements for this and how I start contributing on this

1 0

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

28 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 27 Mar '19

27 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 26 Mar '19

26 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 26 Mar '19

26 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 26 Mar '19

26 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