maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Sanja!

On Feb 09, Oleksandr Byelkin wrote:
> On 09.02.15 15:19, Sergei Golubchik wrote:
> > On Feb 09, sanja@xxxxxxxxxxxx wrote:
> >> At file:///home/bell/maria/bzr/work-maria-5.5-MDEV-6916-check_view/
> >>
> >> === modified file 'client/mysqlcheck.c'
> >> --- a/client/mysqlcheck.c	2014-02-17 10:00:51 +0000
> >> +++ b/client/mysqlcheck.c	2015-02-09 01:48:28 +0000
> >> @@ -196,6 +197,9 @@ static struct my_option my_long_options[
> >>      NO_ARG, 0, 0, 0, 0, 0, 0},
> >>     {"version", 'V', "Output version information and exit.", 0, 0, 0, GET_NO_ARG,
> >>      NO_ARG, 0, 0, 0, 0, 0, 0},
> >> +  {"mysql-upgrade", 'y',
> >> +   "Fix view algorithm view field if it is not new MariaDB view.",
> >> +   0, 0, 0, GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0},
> >>     {0, 0, 0, 0, 0, 0, GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0}
> >>   };
> >>   
> >> @@ -332,7 +336,13 @@ get_one_option(int optid, const struct m
> >>     case 'v':
> >>       verbose++;
> >>       break;
> >> -  case 'V': print_version(); exit(0);
> >> +  case 'V':
> >> +    print_version(); exit(0);
> >> +    break;
> >> +  case 'y':
> >> +    what_to_do= DO_REPAIR;
> >> +    opt_mysql_upgrade= 1;
> >> +    break;
> > See above.
> could you show me example of wat you mean by my_getop() doing its job?

Any option that is not mentioned in this switch. For example,
--all-databases, --all-in-1, --auto-repair, --character-sets-dir,
--compress, and so on, in fact almost all options are not set explicitly
in the switch.

> >>     case OPT_MYSQL_PROTOCOL:
> >>       opt_protocol= find_type_or_exit(argument, &sql_protocol_typelib,
> >>                                       opt->name);
> >> @@ -595,7 +605,15 @@ static int process_all_tables_in_db(char
> >>       for (end = tables + 1; (row = mysql_fetch_row(res)) ;)
> >>       {
> >>         if ((num_columns == 2) && (strcmp(row[1], "VIEW") == 0))
> >> -        continue;
> >> +      {
> >> +        if (!opt_mysql_upgrade)
> >> +          continue;
> >> +      }
> >> +      else
> >> +      {
> >> +        if (opt_mysql_upgrade)
> >> +          continue;
> >
> > Eh? If --mysql-upgrade is specified you skip all tables and only upgrade
> > views? Then the option should absolutely be called --fix-view-algorithm
> So should I rename it?

Yes, please (but see below).

Note that here we're talking about mysqlcheck.  As for the option in
mysql_upgrade - you've already agreed to remove it.

> >> +      }
> >>   
> >>         end= fix_table_name(end, row[0]);
> >>         *end++= ',';
> >> @@ -756,10 +782,12 @@ static int handle_request_for_tables(cha
> >>       if (opt_upgrade)            end = strmov(end, " FOR UPGRADE");
> >>       break;
> >>     case DO_REPAIR:
> >> -    op= (opt_write_binlog) ? "REPAIR" : "REPAIR NO_WRITE_TO_BINLOG";
> >> +    op= ((opt_write_binlog || opt_mysql_upgrade) ?
> >> +         "REPAIR" : "REPAIR NO_WRITE_TO_BINLOG");
> > Really? For view upgrades you want it to be written to binlog?
> > This is a very questionable idea.
> 
> It just has no that syntax because writing to binlog looks like nonsens.

Okay. Then I'd at least allow the syntax, for consistency. It won't do
anything if you aren't going to write REPAIR VIEW to binlog anyway.

> >>       if (opt_quick)              end = strmov(end, " QUICK");
> >>       if (opt_extended)           end = strmov(end, " EXTENDED");
> >>       if (opt_frm)                end = strmov(end, " USE_FRM");
> >> +    if (opt_mysql_upgrade)      end = strmov(end, " FROM MYSQL");
> >>       break;
> >>     case DO_ANALYZE:
> >>       op= (opt_write_binlog) ? "ANALYZE" : "ANALYZE NO_WRITE_TO_BINLOG";
> >> @@ -776,14 +804,17 @@ static int handle_request_for_tables(cha
> >>     if (opt_all_in_1)
> >>     {
> >>       /* No backticks here as we added them before */
> >> -    query_length= sprintf(query, "%s TABLE %s %s", op, tables, options);
> >> +    query_length= sprintf(query, "%s %s %s %s", op,
> >> +                          (opt_mysql_upgrade ? "VIEW" : "TABLE"),
> > this is wrong, because mysqlcheck --mysql-upgrade
> > (or, really, mysqcheck --fix-view-algorithm=xxx) should check/repair
> > *both* tables and views.
> -mysql-upgrade (i.e. REPAIR TABLE ... FROM MYSQL) with table is error 
> now because it have nothing to do. What it should do for tables?
> 
> I can add 2 lists and processing for CHECK/RAPAIR VIEW/TABLE but usage 
> will be limited IMHO.

yes, I meant two lists in my review.

But on the other hand... When mysqlcheck is invoked from mysql_upgrade,
you wouldn't want it to repair all tables. Okay, let's keep your
implementation and only repair views when --fix-view-algorithm is set.

In that case, the option should better be --upgrade-views, and defined as

  --upgrade-views = { no, yes, from_mysql }

note that "no" and "yes" must be in that exactly order, first and
second.

> >> +                          tables, options);
> >>       table_name= tables;
> >>     }
> >>     else
> >>     {
> >>       char *ptr, *org;
> >>   
> >> === modified file 'mysql-test/r/view.result'
> >> --- a/mysql-test/r/view.result	2014-12-21 18:23:28 +0000
> >> +++ b/mysql-test/r/view.result	2015-02-09 01:48:28 +0000
> > ...
> >> +check view v1,v2,v3,v4,v5 FOR UPGRADE;
> >> +Table	Op	Msg_type	Msg_text
> >> +test.v1	check	Note	view 'test.v1' has no field mariadb server in its .frm file
> >> +test.v1	check	status	needs repair
> >> +test.v2	check	Note	view 'test.v2' has no field mariadb server in its .frm file
> >> +test.v2	check	status	needs repair
> >> +test.v3	check	Note	view 'test.v3' has no field mariadb server in its .frm file
> >> +test.v3	check	status	needs repair
> >> +test.v4	check	note	View text checksum failed
> >> +test.v5	check	status	OK
> >> +repair view v1,v2,v3,v4,v5 FROM MYSQL;
> > where's the test for "repair view" without FROM MYSQL ?
> I'll add.

Please add also a test to show how REPAIR VIEW is replicated. Doesn't
have to be a replication test, really. I'd say that "show binlog events"
after the REPAIR VIEW should suffice. Use include/show_binlog_events.inc

> >> +Table	Op	Msg_type	Msg_text
> >> +test.v1	repair	status	view is repaired
> >> +test.v2	repair	status	view is repaired
> >> +test.v3	repair	status	view is repaired
> >> +test.v4	repair	status	view is repaired
> >> +test.v5	repair	status	OK
> >>
> >> === modified file 'sql/share/errmsg-utf8.txt'
> >> --- a/sql/share/errmsg-utf8.txt	2014-05-27 06:45:01 +0000
> >> +++ b/sql/share/errmsg-utf8.txt	2015-02-09 01:48:28 +0000
> >> @@ -6565,3 +6565,9 @@ ER_QUERY_EXCEEDED_ROWS_EXAMINED_LIMIT
> >>   ER_NO_SUCH_TABLE_IN_ENGINE 42S02
> >>           eng "Table '%-.192s.%-.192s' doesn't exist in engine"
> >>           swe "Det finns ingen tabell som heter '%-.192s.%-.192s' i handlern"
> >> +ER_NO_MARIADB_SERVER_FIELD
> >> +        eng "view '%-.192s.%-.192s' has no field mariadb server in its .frm file"
> > First, it's "mariadb_version", not "mariadb_server".
> > Second, I'm not sure such a specific condition needs a dedicated error code.
> >
> > And last - we absolutely cannot add new error messages in 5.5, because
> > 10.0 is already GA. I've already written that in my previous review.

Did you miss this comment or you will do the change?

> >> +ER_VIEW_REPAIR_IS_DONE
> >> +        eng "view is repaired"
> >> +ER_NEEDS_REPAIR
> >> +        eng "needs repair"
> >>
> >> === modified file 'sql/sql_admin.cc'
> >> --- a/sql/sql_admin.cc	2014-05-03 16:12:17 +0000
> >> +++ b/sql/sql_admin.cc	2015-02-09 01:48:28 +0000
> >> @@ -867,6 +879,22 @@ send_result_message:
> >>         fatal_error=1;
> >>         break;
> >>       }
> >> +    case HA_ADMIN_VIEW_REPAIR_IS_DONE:
> > Why did you need that, what's wrong with HA_ADMIN_OK?
> > Btw, I've already written that in my previous review.

Did you miss this comment or you will do the change?

> >
> >> +    {
> >> +      protocol->store(STRING_WITH_LEN("status"), system_charset_info);
> >> +      protocol->store(ER(ER_VIEW_REPAIR_IS_DONE),
> >> +                      strlen(ER(ER_VIEW_REPAIR_IS_DONE)),
> >> +                      system_charset_info);
> >> +      break;
> >> +    }
> >> +    case HA_ADMIN_NEEDS_REPAIR:
> > Why did you need that, what's wrong with HA_ADMIN_NEEDS_UPGRADE?

Did you miss this comment or you will do the change?

> >
> >> +    {
> >> +      protocol->store(STRING_WITH_LEN("status"), system_charset_info);
> >> +      protocol->store(ER(ER_NEEDS_REPAIR),
> >> +                      strlen(ER(ER_NEEDS_REPAIR)),
> >> +                      system_charset_info);
> >> +      break;
> >> +    }
> >>   
> >>       default:				// Probably HA_ADMIN_INTERNAL_ERROR
> >>         {
> >> === modified file 'sql/sql_base.cc'
> >> --- a/sql/sql_base.cc	2014-10-31 13:07:29 +0000
> >> +++ b/sql/sql_base.cc	2015-02-09 01:48:28 +0000
> >> @@ -3019,12 +3020,17 @@ bool open_table(THD *thd, TABLE_LIST *ta
> >>     else if (table_list->open_strategy == TABLE_LIST::OPEN_STUB)
> >>       DBUG_RETURN(FALSE);
> >>   
> >> +
> > second empty line?

Did you miss this comment or you will do the change?

> >
> >>   retry_share:
> >>   
> >>     mysql_mutex_lock(&LOCK_open);
> >>   
> >>     if (!(share= get_table_share_with_discover(thd, table_list, key,
> >> -                                             key_length, OPEN_VIEW,
> >> +                                             key_length,

Regards,
Sergei