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