- developers - lists.mariadb.org

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

Re: [Maria-developers] 4c0eb412ff5: A proper State_tracker::m_changed enacpsulation
by Sergei Golubchik 25 Mar '19

25 Mar '19

Hi, Sergey! ok to push On Mar 25, Sergey Vojtovich wrote: > revision-id: 4c0eb412ff5 (mariadb-10.3.12-85-g4c0eb412ff5) > parent(s): 69b33fca8cc > author: Sergey Vojtovich <svoj(a)mariadb.org> > committer: Sergey Vojtovich <svoj(a)mariadb.org> > timestamp: 2019-03-18 22:51:45 +0400 > message: > > A proper State_tracker::m_changed enacpsulation > > Note: preserved original behaviour, where remaining trackers are not reset > on error from store(). This effectively means subsequent statements will > start tracking from unclean state. > > Part of MDEV-14984 - regression in connect performance > Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] 68abb07298b: Cleanup session tracker redundnacy
by Sergei Golubchik 25 Mar '19

25 Mar '19

Hi, Sergey! ok to push On Mar 25, Sergey Vojtovich wrote: > revision-id: 68abb07298b (mariadb-10.3.12-87-g68abb07298b) > parent(s): facc973189a > author: Sergey Vojtovich <svoj(a)mariadb.org> > committer: Sergey Vojtovich <svoj(a)mariadb.org> > timestamp: 2019-03-18 22:51:58 +0400 > message: > > Cleanup session tracker redundnacy > > - m_enabled is initialised by the base class constructor > - removed unused schema_track_inited > - moved Transaction_state_tracker constructor to declaration > - common enable() > - removed unused Session_sysvars_tracker::check_str() > - classes are "private" by default > - don't even try to compile for embedded > > Part of MDEV-14984 - regression in connect performance > Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] facc973189a: Session_sysvars_tracker::vars_list cleanups
by Sergei Golubchik 25 Mar '19

25 Mar '19

Hi, Sergey! On Mar 25, Sergey Vojtovich wrote: > revision-id: facc973189a (mariadb-10.3.12-86-gfacc973189a) > parent(s): 4c0eb412ff5 > author: Sergey Vojtovich <svoj(a)mariadb.org> > committer: Sergey Vojtovich <svoj(a)mariadb.org> > timestamp: 2019-03-18 22:51:58 +0400 > message: > > Session_sysvars_tracker::vars_list cleanups > > - return proper type > - removed useless node argument > - removed useless mem_flag > - classes are "private" by default > - simplified iterators > > Part of MDEV-14984 - regression in connect performance ok to push Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0