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, [email protected] 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. _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
