maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Sergey!

On Mar 25, Sergey Vojtovich wrote:
> revision-id: 48af6851da2 (mariadb-10.3.12-95-g48af6851da2)
> parent(s): a83d6ee812b
> author: Sergey Vojtovich <svoj@xxxxxxxxxxx>
> committer: Sergey Vojtovich <svoj@xxxxxxxxxxx>
> 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@xxxxxxxxxxx