[Commits] 38564f9: Sc #27132 proposed fix.

Sergey Vojtovich svoj at mariadb.org
Tue May 28 17:47:04 EEST 2019


Hi Alexey,

More aggressive caching of audit plugins sounds good in general.
But I doubt versioning worth it: in this patch idle connections don't allow
concurrent UNINSTALL PLUGIN anyway. And it'd be another obstacle for further
optimisations.

In current state of audit API, I'd just pre-acquire all plugins on connect
and release on disconnect.

I leave it up to you and Sergei to decide upon which approach to take. My
comments for this particular approach inline.

On Mon, May 27, 2019 at 09:50:20AM +0400, Alexey Botchkov wrote:
> revision-id: 38564f99677726bf35013b5b94dcbf11a3809db0 (mariadb-10.1.39-43-g38564f9)
> parent(s): aaf53ea0b68efde4a90cabbbcaf9ca41c1fbf62f
> committer: Alexey Botchkov
> timestamp: 2019-05-27 09:48:40 +0400
> message:
> 
> Sc #27132 proposed fix.
> 
> ---
>  sql/sql_audit.cc         | 16 ++++++++++++++++
>  sql/sql_audit.h          |  1 +
>  sql/sql_class.cc         |  4 +++-
>  sql/sql_class.h          |  1 +
>  sql/sql_connect.cc       |  3 ++-
>  sql/sql_plugin.cc        |  3 +++
>  sql/sql_plugin.h         |  1 +
>  sql/threadpool_common.cc |  3 ++-
>  8 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/sql/sql_audit.cc b/sql/sql_audit.cc
> index dd98e3c..cee0ac2 100644
> --- a/sql/sql_audit.cc
> +++ b/sql/sql_audit.cc
> @@ -212,6 +212,7 @@ void mysql_audit_acquire_plugins(THD *thd, ulong *event_class_mask)
>    {
>      plugin_foreach(thd, acquire_plugins, MYSQL_AUDIT_PLUGIN, event_class_mask);
>      add_audit_mask(thd->audit_class_mask, event_class_mask);
> +    thd->audit_plugin_version= global_plugin_version;
>    }
>    DBUG_VOID_RETURN;
>  }
Should it be done under `if (thd->audit_plugin_version == -1)`? So that we don't
miss UNINSTALL PLUGIN when there're audit plugins for multiple classes:

con1> mysql_audit_acquire_plugins(MYSQL_AUDIT_CONNECTION_CLASS)
con2> UNINSTALL PLUGIN
con1> mysql_audit_acquire_plugins(MYSQL_AUDIT_TABLE_CLASS)

I feel like it has to be done inside acquire_plugins() under LOCK_plugin
protection. So that acquisition inside initialize_audit_plugin() is affected
as well.

> @@ -242,6 +243,20 @@ void mysql_audit_notify(THD *thd, uint event_class, uint event_subtype, ...)
>  
>  
>  /**
> +  Check if there were changes in the state of plugins
> +  so we need to do the mysql_audit_release asap.
> +
> +  @param[in] thd
> +
> +*/
> +
> +my_bool mysql_audit_release_required(THD *thd)
> +{
> +  return thd && (thd->audit_plugin_version != global_plugin_version);
> +}
> +
> +
> +/**
>    Release any resources associated with the current thd.
>    
>    @param[in] thd
`thd` check looks redudant.
Parenthesis are redundant.
Definitely move it to sql_audit.h.
Probably make it "release_if_requires". Something like:

static inline void mysql_audit_release_if_required(THD *thd)
{
#ifndef EMBEDDED_LIBRARY
  if (thd->audit_plugin_version != global_plugin_version)
    mysql_audit_release(thd);
#endif
}

> @@ -276,6 +291,7 @@ void mysql_audit_release(THD *thd)
>    /* Reset the state of thread values */
>    reset_dynamic(&thd->audit_class_plugins);
>    bzero(thd->audit_class_mask, sizeof(thd->audit_class_mask));
> +  thd->audit_plugin_version= -1;
>  }
`0` as a special value hurt my eyes less. But Ok.

> diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc
> index 21093e3..48131b1 100644
> --- a/sql/sql_plugin.cc
> +++ b/sql/sql_plugin.cc
> @@ -228,6 +228,7 @@ static DYNAMIC_ARRAY plugin_array;
>  static HASH plugin_hash[MYSQL_MAX_PLUGIN_TYPE_NUM];
>  static MEM_ROOT plugin_mem_root;
>  static bool reap_needed= false;
> +volatile int global_plugin_version= 1;
No `volatile` please. Either use `my_atomic` or always access it under
LOCK_plugin.

> @@ -2181,6 +2182,7 @@ bool mysql_install_plugin(THD *thd, const LEX_STRING *name,
>      reap_plugins();
>    }
>  err:
> +  global_plugin_version++;
Should we do this on error as well?

> @@ -2327,6 +2329,7 @@ bool mysql_uninstall_plugin(THD *thd, const LEX_STRING *name,
>    }
>    reap_plugins();
>  
> +  global_plugin_version++;
Should we do this if plugin wasn't found as well? For the sake of this task
I'd probably do it around WARN_PLUGIN_BUSY.

Regards,
Sergey


More information about the commits mailing list