- developers - lists.mariadb.org

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

Re: [Maria-developers] 62d62228aa6: Static current schema and state change trackers
by Sergei Golubchik 25 Mar '19

25 Mar '19

Hi, Sergey! Why two trackers are static and the other two aren't? Please, explain it in a comment. On Mar 25, Sergey Vojtovich wrote: > revision-id: 62d62228aa6 (mariadb-10.3.12-91-g62d62228aa6) > parent(s): 61b9bffbedc > author: Sergey Vojtovich <svoj(a)mariadb.org> > committer: Sergey Vojtovich <svoj(a)mariadb.org> > timestamp: 2019-03-19 23:40:07 +0400 > message: > > Static current schema and state change trackers > > Saves 2 new/delete per connection. > > Part of MDEV-14984 - regression in connect performance Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 1

Re: [Maria-developers] db9854b7d48: Allocate Transaction_state_tracker statically
by Sergei Golubchik 25 Mar '19

25 Mar '19

Hi, Sergey! ok to push On Mar 25, Sergey Vojtovich wrote: > revision-id: db9854b7d48 (mariadb-10.3.12-92-gdb9854b7d48) > parent(s): 62d62228aa6 > author: Sergey Vojtovich <svoj(a)mariadb.org> > committer: Sergey Vojtovich <svoj(a)mariadb.org> > timestamp: 2019-03-19 23:40:07 +0400 > message: > > Allocate Transaction_state_tracker statically > > One less new/delete per connection. > > Part of MDEV-14984 - regression in connect performance > Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] 61b9bffbedc: Allocate orig_list statically
by Sergei Golubchik 25 Mar '19

25 Mar '19

Hi, Sergey! ok to push On Mar 25, Sergey Vojtovich wrote: > revision-id: 61b9bffbedc (mariadb-10.3.12-90-g61b9bffbedc) > parent(s): 9dd4fcb3f7c > author: Sergey Vojtovich <svoj(a)mariadb.org> > committer: Sergey Vojtovich <svoj(a)mariadb.org> > timestamp: 2019-03-19 23:39:19 +0400 > message: > > Allocate orig_list statically > > tool_list is a temporary list needed only for SET SESSION > session_track_system_variables, so allocate it on stack instead. > > Sane reinit() calls. > > Saves 2 new/delete per connection. > > Part of MDEV-14984 - regression in connect performance > Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] 9dd4fcb3f7c: Get rid of not implemented SESSION_GTIDS_TRACKER
by Sergei Golubchik 25 Mar '19

25 Mar '19

Hi, Sergey! ok to push On Mar 25, Sergey Vojtovich wrote: > revision-id: 9dd4fcb3f7c (mariadb-10.3.12-89-g9dd4fcb3f7c) > parent(s): 72e4e57a52a > author: Sergey Vojtovich <svoj(a)mariadb.org> > committer: Sergey Vojtovich <svoj(a)mariadb.org> > timestamp: 2019-03-19 17:28:09 +0400 > message: > > Get rid of not implemented SESSION_GTIDS_TRACKER > > One less new/delete per connection. > > Part of MDEV-14984 - regression in connect performance > Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0