[Commits] 38564f9: Sc #27132 proposed fix.
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; } @@ -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 @@ -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; } diff --git a/sql/sql_audit.h b/sql/sql_audit.h index 550b2a5..9a74675 100644 --- a/sql/sql_audit.h +++ b/sql/sql_audit.h @@ -60,6 +60,7 @@ static inline void mysql_audit_notify(THD *thd, uint event_class, #define mysql_audit_connection_enabled() 0 #define mysql_audit_table_enabled() 0 #endif +extern my_bool mysql_audit_release_required(THD *thd); extern void mysql_audit_release(THD *thd); #define MAX_USER_HOST_SIZE 512 diff --git a/sql/sql_class.cc b/sql/sql_class.cc index 8fabcd5..6bcff6d 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -776,6 +776,9 @@ THD::THD(bool is_wsrep_applier) waiting_on_group_commit(FALSE), has_waiter(FALSE), spcont(NULL), m_parser_state(NULL), +#ifndef EMBEDDED_LIBRARY + audit_plugin_version(-1), +#endif #if defined(ENABLED_DEBUG_SYNC) debug_sync_control(0), #endif /* defined(ENABLED_DEBUG_SYNC) */ @@ -1562,7 +1565,6 @@ THD::~THD() mdl_context.destroy(); ha_close_connection(this); - mysql_audit_release(this); plugin_thdvar_cleanup(this); main_security_ctx.destroy(); diff --git a/sql/sql_class.h b/sql/sql_class.h index 1cb516c..6392394 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -2978,6 +2978,7 @@ class THD :public Statement, added to the list of audit plugins which are currently in use. */ unsigned long audit_class_mask[MYSQL_AUDIT_CLASS_MASK_SIZE]; + int audit_plugin_version; #endif #if defined(ENABLED_DEBUG_SYNC) diff --git a/sql/sql_connect.cc b/sql/sql_connect.cc index 4dbb53f..a6a01b1 100644 --- a/sql/sql_connect.cc +++ b/sql/sql_connect.cc @@ -1326,7 +1326,8 @@ void do_handle_one_connection(THD *thd_arg) while (thd_is_connection_alive(thd)) { - mysql_audit_release(thd); + if (mysql_audit_release_required(thd)) + mysql_audit_release(thd); if (do_command(thd)) break; } 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; static bool initialized= 0; ulong dlopen_count; @@ -2181,6 +2182,7 @@ bool mysql_install_plugin(THD *thd, const LEX_STRING *name, reap_plugins(); } err: + global_plugin_version++; mysql_mutex_unlock(&LOCK_plugin); if (argv) free_defaults(argv); @@ -2327,6 +2329,7 @@ bool mysql_uninstall_plugin(THD *thd, const LEX_STRING *name, } reap_plugins(); + global_plugin_version++; mysql_mutex_unlock(&LOCK_plugin); DBUG_RETURN(error); diff --git a/sql/sql_plugin.h b/sql/sql_plugin.h index 7f74114..3bde06a 100644 --- a/sql/sql_plugin.h +++ b/sql/sql_plugin.h @@ -37,6 +37,7 @@ enum enum_plugin_load_option { PLUGIN_OFF, PLUGIN_ON, PLUGIN_FORCE, PLUGIN_FORCE_PLUS_PERMANENT }; extern const char *global_plugin_typelib_names[]; +extern volatile int global_plugin_version; extern ulong dlopen_count; #include <my_sys.h> diff --git a/sql/threadpool_common.cc b/sql/threadpool_common.cc index b4066bd..b8be708 100644 --- a/sql/threadpool_common.cc +++ b/sql/threadpool_common.cc @@ -266,7 +266,8 @@ int threadpool_process_request(THD *thd) { Vio *vio; thd->net.reading_or_writing= 0; - mysql_audit_release(thd); + if (mysql_audit_release_required(thd)) + mysql_audit_release(thd); if ((retval= do_command(thd)) != 0) goto end;
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
Hi, Sergey! On May 28, Sergey Vojtovich wrote:
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.
What obstacle?
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.
For this particular approach - versioning should also be used to avoid calling acquire_plugins all the time. Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi, Sergey!
On May 28, Sergey Vojtovich wrote:
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.
What obstacle? E.g. if we ever decide to get rid of LOCK_plugin, we'll have to study locking
Hi! On Mon, Jun 03, 2019 at 07:24:56PM +0200, Sergei Golubchik wrote: pattern of another global variable. It is fun, but rather time consuming.
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.
For this particular approach - versioning should also be used to avoid calling acquire_plugins all the time.
I assume "this particular" is the one that I suggested, that is pre-acquire all plugins on connect. Why would one need versioning in this case? Regards, Sergey
Hi, Sergey! On Jun 03, Sergey Vojtovich wrote:
On May 28, Sergey Vojtovich wrote:
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.
What obstacle? E.g. if we ever decide to get rid of LOCK_plugin, we'll have to study locking
On Mon, Jun 03, 2019 at 07:24:56PM +0200, Sergei Golubchik wrote: pattern of another global variable. It is fun, but rather time consuming.
I see. I would definitely prefer to get rid of LOCK_plugin, but I'd use something RCU-like, which is, in a way, a versioning too, so this global variable would not be an issue.
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.
For this particular approach - versioning should also be used to avoid calling acquire_plugins all the time. I assume "this particular" is the one that I suggested, that is pre-acquire all plugins on connect. Why would one need versioning in this case?
Sorry. I've just quoted you. You wrote above "My comments for this particular approach inline", so I meant versioning too. That is, if we'll be using versioning to avoid releasing plugins for every statement, we should also use versioning to avoid looking for new plugins for every statement. Regards, Sergei Chief Architect MariaDB and security@mariadb.org
participants (3)
-
holyfoot＠askmonty.org -
Sergei Golubchik -
Sergey Vojtovich