maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

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<i_string> 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