maria-developers team mailing list archive

[Daniel Black <daniel.black@xxxxxxxxxxxxx>]

For review:

Hi All,

list of all changes: https://github.com/openquery/mariadb-server/commits/MDEV-6916-maria-5.5-check_view-r4408

side by side comparision of Sanja/my work against 5.5 head:

https://github.com/MariaDB/server/compare/5.5...openquery:MDEV-6916-maria-5.5-check_view-r4408?expand=1

One clarification on how to make mysqlcheck to upgrade views only needed.

Original test files mysql-test/std_data/mysql_upgrade/* needed.

Small addition of binlog tests needed.


----- Original Message -----
> 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.

fixed https://github.com/openquery/mariadb-server/commit/87f5bae0b56253df965230f585bcb58352b4ef01


>> +static int run_mysqlcheck_views(void)
>> +{
>> +  if (!opt_mysql_upgrade)
>> +    return 0;
>> +  verbose("Phase 0: Fixing views");
> Again, was in my previous review:
> ===
> No "Phase 0" please, renumber all phases to start from 1.

Took opportunity to make adding/changing phases to be lest conflicts in merge

https://github.com/openquery/mariadb-server/commit/25872e2802a4ae32d4f18132d569599295c963cd


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

I've changed this behaviour to handle tables and views.

ref: https://github.com/MariaDB/server/compare/5.5...openquery:MDEV-6916-maria-5.5-check_view-r4408?expand=1


> > > Then the option should absolutely be called --fix-view-algorithm
> > So should I rename it?
> 
> Yes, 

done:

https://github.com/openquery/mariadb-server/commit/e5191dd11beaec44230f97ec1402439ffd27333b


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

done:

https://github.com/openquery/mariadb-server/commit/ebd3c6ccbe43b691515cb05cf4919ff46f15c7f0

> 
> > >> +      }
> > >>   
> > >>         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.

consistency restored:

https://github.com/openquery/mariadb-server/commit/9b067a3e9fa253f90e8cb05b2c9d084900637a1d

Is this appropriate too? Its consistent (even though I consider opt_write_binlog=true a bad default)

https://github.com/openquery/mariadb-server/commit/96e277aed81d41c15621c0842fddfe027d844c6e


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

Done:

https://github.com/openquery/mariadb-server/commit/9b067a3e9fa253f90e8cb05b2c9d084900637a1d

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

Done:

https://github.com/openquery/mariadb-server/commit/9b067a3e9fa253f90e8cb05b2c9d084900637a1d

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

Not done yet.

--skip-tables or --{update|repair}-views-only perhaps?


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

and the 'from_mysql' means don't check tables? This doesn't sound intuitive. More correctly its a --repair-views.


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


I'll fix the test cases when I get the mysql-test/std_data/mysql_upgrade/* files.


> > >> +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?

Done:

https://github.com/openquery/mariadb-server/commit/4409e04d891fba62a3f4ed291c96ee34d645dbec


> > >> +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?


Done:

https://github.com/openquery/mariadb-server/commit/4409e04d891fba62a3f4ed291c96ee34d645dbec

> > >
> > >> +    {
> > >> +      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?

Done:

https://github.com/openquery/mariadb-server/commit/4409e04d891fba62a3f4ed291c96ee34d645dbec


> > >> +    {
> > >> +      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?

Done:

https://github.com/openquery/mariadb-server/commit/4409e04d891fba62a3f4ed291c96ee34d645dbec



-- 
-- 
Daniel Black, Engineer @ Open Query (http://openquery.com.au)
Remote expertise & maintenance for MySQL/MariaDB server environments.