Re: [Maria-developers] 4728581: MDEV-7652 - More explanatory ERROR and WARNING messages when loading plugins
Hi, Sergey! Looks ok, just a couple of comments about the messages. If you're agree - please, fix and push. On Jul 28, Sergey Vojtovich wrote:
revision-id: 4728581813000522d05ab589df8e5f8cb6ad1ee6 parent(s): cf30074c3ff6815d85bbd864b28adfe7949d340c committer: Sergey Vojtovich branch nick: mariadb
Please, update the git post-commit hook (bzr-pull from mariadb-tools). I've just changed it to report a branch better than just "mariadb".
timestamp: 2015-07-28 10:18:55 +0400 message:
MDEV-7652 - More explanatory ERROR and WARNING messages when loading plugins with plugin-load-add that are already registered at mysql.plugin
- issue just one error message, without this extra warning - don't abuse ER_UDF_EXISTS, instead add a proper error message for plugins - report started initialization for each plugin source
--- sql/share/errmsg-utf8.txt | 3 +++ sql/sql_plugin.cc | 15 +++++++++++---- 2 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt index 6954170..374e31f 100644 --- a/sql/share/errmsg-utf8.txt +++ b/sql/share/errmsg-utf8.txt @@ -7111,3 +7111,6 @@ ER_SLAVE_SKIP_NOT_IN_GTID eng "When using GTID, @@sql_slave_skip_counter can not be used. Instead, setting @@gtid_slave_pos explicitly can be used to skip to after a given GTID position." ER_TABLE_DEFINITION_TOO_BIG eng "The definition for table %`s is too big" +ER_PLUGIN_EXISTS + eng "Plugin '%-.192s' already exists" + rus "Плагин '%-.192s' уже существует"
May be "already installed" ? The command is INSTALL PLUGIN. For UDF it was CREATE FUNCTION, that's why "already exists", I suppose.
diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc index dc40870..21efeb0 100644 --- a/sql/sql_plugin.cc +++ b/sql/sql_plugin.cc @@ -1546,6 +1546,9 @@ int plugin_init(int *argc, char **argv, int flags) /* First we register builtin plugins */ + if (global_system_variables.log_warnings >= 9) + sql_print_information("Initializing mandatory plugins"); + for (builtins= mysql_mandatory_plugins; *builtins || mandatory; builtins++) { if (!*builtins)
Here you should've put "Initializing optional plugins". Actually, a better message would be "Initializing other built-in plugins" or " Initializing remaining built-in plugins".
@@ -1627,11 +1630,17 @@ int plugin_init(int *argc, char **argv, int flags) { I_List_iterator
iter(opt_plugin_load_list); i_string *item; + if (global_system_variables.log_warnings >= 9) + sql_print_information("Initializinig plugins specified by command line");
"Initializinig plugins specified on the command line"
while (NULL != (item= iter++)) plugin_load_list(&tmp_root, item->ptr);
if (!(flags & PLUGIN_INIT_SKIP_PLUGIN_TABLE)) + { + if (global_system_variables.log_warnings >= 9) + sql_print_information("Initializing installed plugins"); plugin_load(&tmp_root); + } }
Regards, Sergei
Hi Sergei, thanks for your review. Answers inline. On Tue, Jul 28, 2015 at 07:36:29PM +0200, Sergei Golubchik wrote:
Hi, Sergey!
Looks ok, just a couple of comments about the messages. If you're agree - please, fix and push.
On Jul 28, Sergey Vojtovich wrote:
revision-id: 4728581813000522d05ab589df8e5f8cb6ad1ee6 parent(s): cf30074c3ff6815d85bbd864b28adfe7949d340c committer: Sergey Vojtovich branch nick: mariadb
Please, update the git post-commit hook (bzr-pull from mariadb-tools). I've just changed it to report a branch better than just "mariadb". Thanks, that was quite annoying.
timestamp: 2015-07-28 10:18:55 +0400 message:
MDEV-7652 - More explanatory ERROR and WARNING messages when loading plugins with plugin-load-add that are already registered at mysql.plugin
- issue just one error message, without this extra warning - don't abuse ER_UDF_EXISTS, instead add a proper error message for plugins - report started initialization for each plugin source
--- sql/share/errmsg-utf8.txt | 3 +++ sql/sql_plugin.cc | 15 +++++++++++---- 2 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt index 6954170..374e31f 100644 --- a/sql/share/errmsg-utf8.txt +++ b/sql/share/errmsg-utf8.txt @@ -7111,3 +7111,6 @@ ER_SLAVE_SKIP_NOT_IN_GTID eng "When using GTID, @@sql_slave_skip_counter can not be used. Instead, setting @@gtid_slave_pos explicitly can be used to skip to after a given GTID position." ER_TABLE_DEFINITION_TOO_BIG eng "The definition for table %`s is too big" +ER_PLUGIN_EXISTS + eng "Plugin '%-.192s' already exists" + rus "Плагин '%-.192s' уже существует"
May be "already installed" ? The command is INSTALL PLUGIN. For UDF it was CREATE FUNCTION, that's why "already exists", I suppose.
I just wonder if term "installed" is Ok for built-in plugins and those that were loaded by command line.
diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc index dc40870..21efeb0 100644 --- a/sql/sql_plugin.cc +++ b/sql/sql_plugin.cc @@ -1546,6 +1546,9 @@ int plugin_init(int *argc, char **argv, int flags) /* First we register builtin plugins */ + if (global_system_variables.log_warnings >= 9) + sql_print_information("Initializing mandatory plugins"); + for (builtins= mysql_mandatory_plugins; *builtins || mandatory; builtins++) { if (!*builtins)
Here you should've put "Initializing optional plugins". Actually, a better message would be "Initializing other built-in plugins" or " Initializing remaining built-in plugins".
Sorry, I missed this. But do we need to distinguish between mandatory and optional built-ins? May be change "Initializing mandatory plugins" with "Initializing built-in plugins"?
@@ -1627,11 +1630,17 @@ int plugin_init(int *argc, char **argv, int flags) { I_List_iterator
iter(opt_plugin_load_list); i_string *item; + if (global_system_variables.log_warnings >= 9) + sql_print_information("Initializinig plugins specified by command line"); "Initializinig plugins specified on the command line"
Ok. Thanks, Sergey
Hi, Sergey! On Jul 29, Sergey Vojtovich wrote:
diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt index 6954170..374e31f 100644 --- a/sql/share/errmsg-utf8.txt +++ b/sql/share/errmsg-utf8.txt @@ -7111,3 +7111,6 @@ ER_SLAVE_SKIP_NOT_IN_GTID eng "When using GTID, @@sql_slave_skip_counter can not be used. Instead, setting @@gtid_slave_pos explicitly can be used to skip to after a given GTID position." ER_TABLE_DEFINITION_TOO_BIG eng "The definition for table %`s is too big" +ER_PLUGIN_EXISTS + eng "Plugin '%-.192s' already exists" + rus "Плагин '%-.192s' уже существует"
May be "already installed" ? The command is INSTALL PLUGIN. For UDF it was CREATE FUNCTION, that's why "already exists", I suppose. I just wonder if term "installed" is Ok for built-in plugins and those that were loaded by command line.
Whatever. Use something else. I just think that "exists" is kinda weird. Plugin exists (as, say, ha_tokudb.so) even if you didn't install it.
diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc index dc40870..21efeb0 100644 --- a/sql/sql_plugin.cc +++ b/sql/sql_plugin.cc @@ -1546,6 +1546,9 @@ int plugin_init(int *argc, char **argv, int flags) /* First we register builtin plugins */ + if (global_system_variables.log_warnings >= 9) + sql_print_information("Initializing mandatory plugins"); + for (builtins= mysql_mandatory_plugins; *builtins || mandatory; builtins++) { if (!*builtins)
Here you should've put "Initializing optional plugins". Actually, a better message would be "Initializing other built-in plugins" or " Initializing remaining built-in plugins". Sorry, I missed this. But do we need to distinguish between mandatory and optional built-ins? May be change "Initializing mandatory plugins" with "Initializing built-in plugins"?
That's fine too. I was only saying that not all built-in plugins are mandatory. Regards, Sergei
participants (2)
-
Sergei Golubchik -
Sergey Vojtovich