Hi, Daniel! Thanks! I've now reviewed the fix as a whole, all patches combined. There weren't many comments, but still there were some. And questions. If you'd like I can apply the latest changes (finishing touches, so to say) myself - it can speed things up. Or you can do that, whatever you prefer. Either way, I'd still like to see my questions answered first.

diff --git a/client/client_priv.h b/client/client_priv.h index e206804..a66c388 100644 --- a/client/client_priv.h +++ b/client/client_priv.h @@ -79,6 +79,7 @@ enum options_client OPT_SLAP_COMMIT, OPT_SLAP_DETACH, OPT_SLAP_NO_DROP, + OPT_UPGRADE_VIEWS,

please remove. see below

OPT_MYSQL_REPLACE_INTO, OPT_BASE64_OUTPUT_MODE, OPT_SERVER_ID, OPT_FIX_TABLE_NAMES, OPT_FIX_DB_NAMES, OPT_SSL_VERIFY_SERVER_CERT, OPT_AUTO_VERTICAL_OUTPUT, diff --git a/client/mysqlcheck.c b/client/mysqlcheck.c index a454943..43c7ff0 100644 --- a/client/mysqlcheck.c +++ b/client/mysqlcheck.c @@ -57,6 +58,20 @@ static uint opt_protocol=0;

enum operations { DO_CHECK=1, DO_REPAIR, DO_ANALYZE, DO_OPTIMIZE, DO_UPGRADE };

+typedef enum { + UPGRADE_VIEWS_NO=0, + UPGRADE_VIEWS_YES=1, + UPGRADE_VIEWS_FROM_MYSQL=2, + UPGRADE_VIEWS_COUNT

UPGRADE_VIEWS_COUNT is unused, please remove

+} enum_upgrade_views; +const char *upgrade_views_opts[]= +{"NO", "YES", "FROM_MYSQL", NullS}; +TYPELIB upgrade_views_typelib= + { array_elements(upgrade_views_opts) - 1, "", + upgrade_views_opts, NULL }; +static enum_upgrade_views opt_upgrade_views= UPGRADE_VIEWS_NO; +static char *opt_upgrade_views_str= NullS; + static struct my_option my_long_options[] = { {"all-databases", 'A', @@ -196,6 +211,15 @@ 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}, + {"upgrade-views", OPT_UPGRADE_VIEWS, + "Repairs/Upgrades views. 'No' (default) doesn't do anything to views. " + "'Yes' repairs the checksum and adds a MariaDB version to the frm table defination (if missing)." + "Using 'from_mysql' will, when upgrading from MySQL, and ensure the view algorithms are the" + "same as what they where previously.",

"definition" And the whole message could be reworded. for example "Repair views. One of: NO, YES (correct the checksum, if necessary, add the mariadb-version field), FROM_MYSQL (same as YES and toggle the algorithm MERGE<->TEMPTABLE)." In 10.1 mysqld --help automatically adds "One of: <list of values>" to all enum options, but only if the first option is not already present in the help text in the same letter case (it uses strstr for that). It's 5.5 but better to mention options in the same leter case and use the same "One of:" style to blend in better.

+ &opt_upgrade_views_str, &opt_upgrade_views_str, 0, GET_STR, OPT_ARG, 0, 0, 0, 0, 0, 0},

not GET_STR but GET_ENUM with 0 instead of OPT_UPGRADE_VIEWS

+ {"fix-tables", 'Y', + "Fix tables. Usually the default however mysql_upgrade will run with --skip-fix-tables.",

"Usually the default..." is redundant, my_getopt will say it automatically btw, it's mysqlcheck tool, and these two options are symmetrical, in a sense. so it'd be better to have matching names. may be --process-tables = no/yes --process-views = no/yes/from_mysql ? it's a bit strange to have --process-views=from_mysql. A more logical variant could be --process-views=upgrade_from_mysql, but it looks too long for me. on the other hand, upgrade_from_mysql value is not for manual use, it's supposed to be specified by mysql_upgrade, so it doesn't mater if it's long

+ &opt_fix_tables, &opt_fix_tables, 0, GET_BOOL, NO_ARG, 1, 0, 0, 0, 0, 0}, {0, 0, 0, 0, 0, 0, GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0} };

@@ -332,7 +356,18 @@ get_one_option(int optid, const struct my_option *opt __attribute__((unused)), case 'v': verbose++; break; - case 'V': print_version(); exit(0); + case 'V': + print_version(); exit(0); + break; + case OPT_UPGRADE_VIEWS: + if (argument == NULL) + opt_upgrade_views= UPGRADE_VIEWS_NO; + else + { + opt_upgrade_views= (enum_upgrade_views) + (find_type_or_exit(argument, &upgrade_views_typelib, opt->name)-1); + } + break;

remove, use GET_ENUM

case OPT_MYSQL_PROTOCOL: opt_protocol= find_type_or_exit(argument, &sql_protocol_typelib, opt->name); @@ -363,6 +398,23 @@ static int get_options(int *argc, char ***argv) if ((ho_error=handle_options(argc, argv, my_long_options, get_one_option))) exit(ho_error);

+ if (what_to_do==DO_REPAIR && !(opt_upgrade_views || opt_fix_tables)) + { + fprintf(stderr, "Error: %s doesn't support repair command without either upgrade-views or fix-tables specified.\n",

I'd reformulate. it's not "doesn't support", it's more like "command makes no sense", so I'd say "Nothing to repair when both --process-tables=NO and --process-views=NO"

+ my_progname); + exit(1); + } + if (opt_upgrade_views && what_to_do!=DO_REPAIR) + { + if (!what_to_do) + what_to_do= DO_REPAIR; + else + { + fprintf(stderr, "Error: %s doesn't support non-repair command with option upgrade-views.\n", + my_progname);

Better "%s of views is not supported", "analyzing"/"optimizing"/"checking" (depending on what_to_do) although I don't see why CHECK VIEW should not be supported

+ exit(1); + } + } if (!what_to_do) { size_t pnlen= strlen(my_progname); @@ -463,8 +515,41 @@ static int process_databases(char **db_names) } /* process_databases */

+/* returns: -1 for error, 1 for view, 0 for table */ +static int is_view(const char *table, uint length) +{ + char *query, *ptr; + MYSQL_RES *res; + MYSQL_FIELD *field; + int view; + DBUG_ENTER("is_view"); + + if (!(query =(char *) my_malloc((sizeof(char)*(length+110)), MYF(MY_WME)))) + DBUG_RETURN(-1); + ptr= strmov(query, "SHOW CREATE TABLE `"); + ptr= strxmov(ptr, table, NullS); + ptr= strxmov(ptr, "`", NullS);

Better use SHOW FULL TABLES WHERE TABLES_IN_MYSQL='xxxx'; That's the statement one should normally use to know whether xxxx is a view or a table. SHOW FULL TABLES only does just that, it doesn't even parse the frm file (and doesn't open the table in the engine, of course).

+ if (mysql_query(sock, query)) + { + fprintf(stderr, "Failed to %s\n", query); + fprintf(stderr, "Error: %s\n", mysql_error(sock)); + my_free(query); + DBUG_RETURN(-1); + } + res= mysql_store_result(sock); + field= mysql_fetch_field(res); + view= (strcmp(field->name,"View")==0) ? 1 : 0; + mysql_free_result(res); + my_free(query); + + DBUG_RETURN(view); +} + static int process_selected_tables(char *db, char **table_names, int tables) { + int view; + char *table; + uint table_len; DBUG_ENTER("process_selected_tables");

if (use_db(db)) @@ -494,12 +579,24 @@ static int process_selected_tables(char *db, char **table_names, int tables) *end++= ','; } *--end = 0; - handle_request_for_tables(table_names_comma_sep + 1, (uint) (tot_length - 1)); + table= table_names_comma_sep + 1; + table_len= tot_length - 1; + view= is_view(table, table_len); + if (view < 0) + DBUG_RETURN(1); + handle_request_for_tables(table, table_len, (view==1));

This looks wrong. table_names_comma_sep is a comma separated list of names, like "t1, t2, v1, v2, t3, v3, etc" but you check the first element in the list and assume that they all are of the same type. Let's just say that in --all-in-one mode we don't support mixing objects of different types. Then it becomes: if (opt_fix_tables && opt_upgrade_views) { fprintf(strerr, "not supported..."); return 1; } handle_request_for_tables(table_names_comma_sep + 1, (uint) (tot_length - 1), opt_upgrade_views); alternatively, of course, you can walk the list, create two strings - one for table and one for views - that is, do the same as in process_all_tables_in_db()

my_free(table_names_comma_sep); } else for (; tables > 0; tables--, table_names++) - handle_request_for_tables(*table_names, fixed_name_length(*table_names)); + { + table= *table_names; + table_len= fixed_name_length(*table_names); + view= is_view(table, table_len); + if (view < 0) + continue; + handle_request_for_tables(table, table_len, (view==1)); + } DBUG_RETURN(0); } /* process_selected_tables */

@@ -748,10 +887,13 @@ static int handle_request_for_tables(char *tables, uint length) 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) ? + "REPAIR" : "REPAIR NO_WRITE_TO_BINLOG"; if (opt_quick) end = strmov(end, " QUICK"); if (opt_extended) end = strmov(end, " EXTENDED"); if (opt_frm) end = strmov(end, " USE_FRM"); + if (opt_upgrade_views==UPGRADE_VIEWS_FROM_MYSQL && view) + end = strmov(end, " FROM MYSQL");

I'd protect other opt's with !view: if (!view) { if (opt_quick) .... ... } else { if (opt_upgrade_views == UPGRADE_VIEWS_FROM_MYSQL) end = strmov(end, " FROM MYSQL"); }

break; case DO_ANALYZE: op= (opt_write_binlog) ? "ANALYZE" : "ANALYZE NO_WRITE_TO_BINLOG"; @@ -785,7 +928,8 @@ static int handle_request_for_tables(char *tables, uint length) } if (mysql_real_query(sock, query, query_length)) { - sprintf(message, "when executing '%s TABLE ... %s'", op, options); + sprintf(message, "when executing '%s%s... %s'", op, tab_view, options); + /* sprintf(message, "when executing '%s'", query); */

forgotten debug comment?

DBerror(sock, message); my_free(query); DBUG_RETURN(1); diff --git a/client/mysql_upgrade.c b/client/mysql_upgrade.c index 5fb3b13..97e2ad92 100644 --- a/client/mysql_upgrade.c +++ b/client/mysql_upgrade.c @@ -752,12 +755,67 @@ static int run_mysqlcheck_upgrade(void) opt_write_binlog ? "--write-binlog" : "--skip-write-binlog", "2>&1", NULL); + if (retch || opt_systables_only) + verbose("Phase %d/%d: Skipping 'mysql_fix_privilege_tables'... not needed", phase++, phases_total);

This is a bit strange. I'd expect something like if (....) { run some step } else { verbose("step skipped"); } that's what you have below in run_mysqlcheck_views. but printing the skip message here is weird and confusing. better to break if() below (that calls run_sql_fix_privilege_tables) and print the message there.

+ return retch; }

+#define EVENTS_STRUCT_LEN 7000 + +static my_bool is_mysql() +{ + my_bool ret= TRUE; + DYNAMIC_STRING ds_events_struct; + + if (init_dynamic_string(&ds_events_struct, NULL, + EVENTS_STRUCT_LEN, EVENTS_STRUCT_LEN)) + die("Out of memory"); + + if (run_query("show create table mysql.event", + &ds_events_struct, FALSE) || + strstr(ds_events_struct.str, "IGNORE_BAD_TABLE_OPTIONS") != NULL) + ret= FALSE; + else + verbose("MySQL upgrade detected"); + + dynstr_free(&ds_events_struct); + return(ret); +} + +static int run_mysqlcheck_views(void) +{ + const char *upgrade_views="--upgrade-views=YES"; + if (is_mysql()) + { + upgrade_views="--upgrade-views=FROM_MYSQL"; + verbose("Phase %d/%d: Fixing views from mysql", phase++, phases_total); + } + else if (opt_systables_only) + { + verbose("Phase %d/%d: Fixing views - skipped - not required", phase++, phases_total); + return 0; + } + else + verbose("Phase %d/%d: Fixing views", phase++, phases_total); + + print_conn_args("mysqlcheck"); + return run_tool(mysqlcheck_path, + NULL, /* Send output from mysqlcheck directly to screen */ + "--no-defaults", + ds_args.str, + "--all-databases", + upgrade_views, + "--skip-fix-tables", + opt_verbose ? "--verbose": "", + opt_silent ? "--silent": "", + opt_write_binlog ? "--write-binlog" : "--skip-write-binlog", + "2>&1", + NULL); +}

static int run_mysqlcheck_fixnames(void) { - verbose("Phase 1/3: Fixing table and database names"); + verbose("Phase %d/%d: Fixing table and database names", phase++, phases_total); print_conn_args("mysqlcheck"); return run_tool(mysqlcheck_path, NULL, /* Send output from mysqlcheck directly to screen */ diff --git a/mysql-test/r/mysql_upgrade_view.result b/mysql-test/r/mysql_upgrade_view.result new file mode 100644 index 0000000..aa39319 --- /dev/null +++ b/mysql-test/r/mysql_upgrade_view.result @@ -0,0 +1,205 @@ +set sql_log_bin=0; +drop table if exists t1,v1,v2,v3,v4,v1badcheck; +drop view if exists t1,v1,v2,v3,v4,v1badcheck; +create table t1(a int); +create table kv(k varchar(30) NOT NULL PRIMARY KEY,v varchar(50)); +flush tables; +Phase 1/4: Fixing views +test.v1 OK +test.v1badcheck OK +test.v2 OK +test.v3 OK +Phase 2/4: Fixing table and database names +Phase 3/4: Checking and upgrading tables +Processing databases +information_schema +mtr +mtr.global_suppressions OK +mtr.test_suppressions OK +mysql +mysql.columns_priv OK +mysql.db OK +mysql.event OK +mysql.func OK +mysql.help_category OK +mysql.help_keyword OK +mysql.help_relation OK +mysql.help_topic OK +mysql.host OK +mysql.ndb_binlog_index OK +mysql.plugin OK +mysql.proc OK +mysql.procs_priv OK +mysql.proxies_priv OK +mysql.servers OK +mysql.tables_priv OK +mysql.time_zone OK +mysql.time_zone_leap_second OK +mysql.time_zone_name OK +mysql.time_zone_transition OK +mysql.time_zone_transition_type OK +mysql.user OK +performance_schema +test +test.kv OK +test.t1 OK +Phase 4/4: Running 'mysql_fix_privilege_tables'... +OK +show create view v1; +View Create View character_set_client collation_connection +v1 CREATE ALGORITHM=MERGE DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v1` AS select `t1`.`a` AS `a` from `t1` utf8 utf8_general_ci +show create view v2; +View Create View character_set_client collation_connection +v2 CREATE ALGORITHM=TEMPTABLE DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v2` AS select `t1`.`a` AS `a` from `t1` utf8 utf8_general_ci +show create view v3; +View Create View character_set_client collation_connection +v3 CREATE ALGORITHM=UNDEFINED DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v3` AS select `t1`.`a` AS `a` from `t1` utf8 utf8_general_ci +set sql_log_bin=1; +REPAIR VIEW v1,v2; +Table Op Msg_type Msg_text +test.v1 repair status OK +test.v2 repair status OK +REPAIR VIEW v1badcheck; +Table Op Msg_type Msg_text +test.v1badcheck repair status OK +REPAIR NO_WRITE_TO_BINLOG VIEW v3; +Table Op Msg_type Msg_text +test.v3 repair status OK +set sql_log_bin=0; +show binlog events from <binlog_start>; +Log_name Pos Event_type Server_id End_log_pos Info +master-bin.000001 # Query # # use `test`; REPAIR VIEW v1,v2 +master-bin.000001 # Query # # use `test`; REPAIR VIEW v1badcheck

Hm. As far as I remember, Sanja said that it makes no sense to write REPAIR VIEW to binlog, and that's why he didn't support NO_WRITE_TO_BINLOG. And I replied that let's support NO_WRITE_TO_BINLOG syntax for consistency, but not write REPAIR VIEW to binlog regardless, if that makes no sense. Do you think it *does* make sense to write REPAIR VIEW to binlog? I agree that it's consistent with REPAIR TABLE and will get us less bug reports about "REPAIR VIEW not in binlog". But can it be useful?

+LOAD DATA INFILE 'MYSQLD_DATADIR/test/v1.frm' REPLACE INTO TABLE kv FIELDS TERMINATED BY '='; +SELECT k,v from kv where k in ('md5','algorithm');

nice :)

+k v +algorithm 1 +md5 5e6eaf216e7b016fcedfd4e1113517af +SELECT k from kv where k ='mariadb-version'; +k +mariadb-version +truncate table kv; +LOAD DATA INFILE 'MYSQLD_DATADIR/test/v2.frm' REPLACE INTO TABLE kv FIELDS TERMINATED BY '='; +SELECT k,v from kv where k in ('md5','algorithm'); +k v +algorithm 2 +md5 5e6eaf216e7b016fcedfd4e1113517af +SELECT k from kv where k ='mariadb-version'; +k +mariadb-version +truncate table kv; +LOAD DATA INFILE 'MYSQLD_DATADIR/test/v3.frm' REPLACE INTO TABLE kv FIELDS TERMINATED BY '='; +SELECT k,v from kv where k in ('md5','algorithm'); +k v +algorithm 0 +md5 5e6eaf216e7b016fcedfd4e1113517af +SELECT k from kv where k ='mariadb-version'; +k +mariadb-version +truncate table kv; +LOAD DATA INFILE 'MYSQLD_DATADIR/test/v1badcheck.frm' REPLACE INTO TABLE kv FIELDS TERMINATED BY '='; +SELECT k,v from kv where k in ('md5','algorithm'); +k v +algorithm 1 +md5 5e6eaf216e7b016fcedfd4e1113517af +SELECT k from kv where k ='mariadb-version'; +k +mariadb-version +truncate table kv; +drop view if exists v1,v2,v3,v1badcheck; +flush tables; +create algorithm=temptable view v4 as select a from t1; +show create view v1; +View Create View character_set_client collation_connection +v1 CREATE ALGORITHM=MERGE DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v1` AS select `t1`.`a` AS `a` from `t1` utf8 utf8_general_ci +show create view v2; +View Create View character_set_client collation_connection +v2 CREATE ALGORITHM=TEMPTABLE DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v2` AS select `t1`.`a` AS `a` from `t1` utf8 utf8_general_ci +show create view v3; +View Create View character_set_client collation_connection +v3 CREATE ALGORITHM=UNDEFINED DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v3` AS select `t1`.`a` AS `a` from `t1` utf8 utf8_general_ci +show create view v4; +View Create View character_set_client collation_connection +v4 CREATE ALGORITHM=TEMPTABLE DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v4` AS select `t1`.`a` AS `a` from `t1` latin1 latin1_swedish_ci +MySQL upgrade detected +Phase 1/4: Fixing views from mysql +test.v1 OK +test.v2 OK +test.v3 OK +test.v4 OK +Phase 2/4: Fixing table and database names +Phase 3/4: Checking and upgrading tables +Processing databases +information_schema +mtr +mtr.global_suppressions OK +mtr.test_suppressions OK +mysql +mysql.columns_priv OK +mysql.db OK +mysql.ev_bk OK +mysql.event OK +mysql.func OK +mysql.help_category OK +mysql.help_keyword OK +mysql.help_relation OK +mysql.help_topic OK +mysql.host OK +mysql.ndb_binlog_index OK +mysql.plugin OK +mysql.proc OK +mysql.procs_priv OK +mysql.proxies_priv OK +mysql.servers OK +mysql.tables_priv OK +mysql.time_zone OK +mysql.time_zone_leap_second OK +mysql.time_zone_name OK +mysql.time_zone_transition OK +mysql.time_zone_transition_type OK +mysql.user OK +performance_schema +test +test.kv OK +test.t1 OK +Phase 4/4: Running 'mysql_fix_privilege_tables'... +OK +show create view v1; +View Create View character_set_client collation_connection +v1 CREATE ALGORITHM=TEMPTABLE DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v1` AS select `t1`.`a` AS `a` from `t1` utf8 utf8_general_ci +show create view v2; +View Create View character_set_client collation_connection +v2 CREATE ALGORITHM=MERGE DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v2` AS select `t1`.`a` AS `a` from `t1` utf8 utf8_general_ci +show create view v3; +View Create View character_set_client collation_connection +v3 CREATE ALGORITHM=UNDEFINED DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v3` AS select `t1`.`a` AS `a` from `t1` utf8 utf8_general_ci +show create view v4; +View Create View character_set_client collation_connection +v4 CREATE ALGORITHM=TEMPTABLE DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v4` AS select `t1`.`a` AS `a` from `t1` latin1 latin1_swedish_ci +LOAD DATA INFILE 'MYSQLD_DATADIR/test/v1.frm' REPLACE INTO TABLE kv FIELDS TERMINATED BY '='; +SELECT k,v from kv where k in ('md5','algorithm'); +k v +algorithm 2 +md5 5e6eaf216e7b016fcedfd4e1113517af +SELECT k from kv where k ='mariadb-version'; +k +mariadb-version +truncate table kv; +drop view if exists v1,v2,v3; +flush tables; +test.v1 OK +test.v2 OK +test.v3 OK +test.v4 OK +show binlog events from <binlog_start>; +Log_name Pos Event_type Server_id End_log_pos Info +master-bin.000001 # Query # # use `test`; REPAIR VIEW v1,v2 +master-bin.000001 # Query # # use `test`; REPAIR VIEW v1badcheck +master-bin.000001 # Query # # use `test`; REPAIR VIEW `v1` FROM MYSQL +master-bin.000001 # Query # # use `test`; REPAIR VIEW `v2` FROM MYSQL +master-bin.000001 # Query # # use `test`; REPAIR VIEW `v3` FROM MYSQL +master-bin.000001 # Query # # use `test`; REPAIR VIEW `v4` FROM MYSQL +flush tables; +drop table if exists kv; +drop view v1,v2,v3,v4; +drop table t1; diff --git a/sql/sql_admin.cc b/sql/sql_admin.cc index 92aa414..65292a5 100644 --- a/sql/sql_admin.cc +++ b/sql/sql_admin.cc @@ -380,7 +381,18 @@ static bool mysql_admin_table(THD* thd, TABLE_LIST* tables, lex->query_tables_own_last= 0;

if (view_operator_func == NULL) + { table->required_type=FRMTYPE_TABLE; + DBUG_ASSERT(!lex->only_view); + } + else if (lex->only_view) + { + table->required_type= FRMTYPE_VIEW; + } + else if (!lex->only_view && lex->sql_command == SQLCOM_REPAIR) + { + table->required_type= FRMTYPE_TABLE; + }

That's a bit inconsistent, CHECK TABLE historically works for views, but REPAIR TABLE doesn't, one must use REPAIR VIEW, while CHECK VIEW is optional. But I don't like if REPAIR TABLE would work for views. May be issue a deprecation warning for CHECK TABLE? Or just leave it as is? may be nobody cares...

if (lex->sql_command == SQLCOM_CHECK || lex->sql_command == SQLCOM_REPAIR || diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy index 257001c..dfbc6c4 100644 --- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy @@ -13786,7 +13807,18 @@ lock:

table_or_tables: TABLE_SYM + { Lex->only_view= FALSE; } + | TABLES + { Lex->only_view= FALSE; } + ; + +table_or_view: + TABLE_SYM + { Lex->only_view= FALSE; } | TABLES + { Lex->only_view= FALSE; } + | VIEW_SYM + { Lex->only_view= TRUE; }

reuse table_or_tables here

;

table_lock_list: diff --git a/sql/sql_view.cc b/sql/sql_view.cc index 09784f7..24f01ae 100644 --- a/sql/sql_view.cc +++ b/sql/sql_view.cc @@ -791,6 +811,86 @@ static File_option view_parameters[]= static LEX_STRING view_file_type[]= {{(char*) STRING_WITH_LEN("VIEW") }};

+int mariadb_fix_view(THD *thd, TABLE_LIST *view, bool wrong_checksum, + bool swap_alg) +{ + char dir_buff[FN_REFLEN + 1], path_buff[FN_REFLEN + 1]; + LEX_STRING dir, file, path; + DBUG_ENTER("mariadb_fix_view"); + + if (!wrong_checksum && view->mariadb_version) + DBUG_RETURN(HA_ADMIN_OK); + + make_view_filename(&dir, dir_buff, sizeof(dir_buff), + &path, path_buff, sizeof(path_buff), + &file, view); + /* init timestamp */ + if (!view->timestamp.str) + view->timestamp.str= view->timestamp_buffer; + + /* check old .frm */ + { + char path_buff[FN_REFLEN]; + LEX_STRING path; + File_parser *parser; + + path.str= path_buff; + fn_format(path_buff, file.str, dir.str, "", MY_UNPACK_FILENAME); + path.length= strlen(path_buff); + if (access(path.str, F_OK)) + DBUG_RETURN(HA_ADMIN_INVALID); + + if (!(parser= sql_parse_prepare(&path, thd->mem_root, 0))) + DBUG_RETURN(HA_ADMIN_INTERNAL_ERROR); + + if (!parser->ok() || !is_equal(&view_type, parser->type())) + DBUG_RETURN(HA_ADMIN_INVALID); + }

I wonder if that's necessary. A view is open at this point, you've just calculated its checksum and tested whether it has mariadb_version field (in view_repair()).

+ + if (swap_alg && view->algorithm != VIEW_ALGORITHM_UNDEFINED) + { + DBUG_ASSERT(view->algorithm == VIEW_ALGORITHM_MERGE || + view->algorithm == VIEW_ALGORITHM_TMPTABLE); + if (view->algorithm == VIEW_ALGORITHM_MERGE) + view->algorithm= VIEW_ALGORITHM_TMPTABLE; + else + view->algorithm= VIEW_ALGORITHM_MERGE; + } + else + swap_alg= 0; + if (wrong_checksum) + { + if (view->md5.length != 32) + { + if ((view->md5.str= (char *)thd->alloc(32 + 1)) == NULL) + DBUG_RETURN(HA_ADMIN_FAILED); + } + view->calc_md5(view->md5.str); + view->md5.length= 32; + } + view->mariadb_version= MYSQL_VERSION_ID; + + if (sql_create_definition_file(&dir, &file, view_file_type, + (uchar*)view, view_parameters)) + { + sql_print_error("View '%-.192s'.'%-.192s': algorithm swap error.", + view->db, view->table_name); + DBUG_RETURN(HA_ADMIN_INTERNAL_ERROR); + } + sql_print_information("View '%-.192s'.'%-.192s': versioned to %llu%s%s", + view->db, view->table_name, view->mariadb_version, + (wrong_checksum ? ", and checksum corrected" : ""), + (swap_alg ? + ((view->algorithm == VIEW_ALGORITHM_MERGE) ? + ", and algorithm swapped to 'MERGE'" + : ", and algorithm swapped to 'TEMPTABLE'")

I don't like how it looks "and algorithm swapped to 'MERGE'". I think "changed to" or "corrected to be" or "restored to be" looks better.

+ : "")); + + + DBUG_RETURN(HA_ADMIN_OK); +} + + /* Register VIEW (write .frm & process .frm's history backups)

@@ -1941,6 +2033,63 @@ int view_checksum(THD *thd, TABLE_LIST *view) HA_ADMIN_OK); }

+/** + Check view + + @param thd thread handle + @param view view for check + @param check_opt check options + + @retval HA_ADMIN_OK OK + @retval HA_ADMIN_NOT_IMPLEMENTED it is not VIEW + @retval HA_ADMIN_WRONG_CHECKSUM check sum is wrong +*/ +int view_check(THD *thd, TABLE_LIST *view, HA_CHECK_OPT *check_opt) +{ + int res; + DBUG_ENTER("view_check"); + if ((res= view_checksum(thd, view)) != HA_ADMIN_OK) + DBUG_RETURN(res); + if (((check_opt->sql_flags & TT_FOR_UPGRADE) && + !view->mariadb_version)) + { + push_warning_printf(thd, MYSQL_ERROR::WARN_LEVEL_NOTE, + ER_TABLE_NEEDS_UPGRADE, + ER(ER_TABLE_NEEDS_UPGRADE), + view->db, + view->table_name);

Is that necessary? I'd expect the caller (mysql_admin_table() to do that, when it gets HA_ADMIN_NEEDS_UPGRADE. There is nothing view specific in this warning message that you're printing.

+ DBUG_RETURN(HA_ADMIN_NEEDS_UPGRADE); + } + DBUG_RETURN(HA_ADMIN_OK); +} + + +/** + Repair view + + @param thd thread handle + @param view view for check + @param check_opt check options + + @retval HA_ADMIN_OK OK + @retval HA_ADMIN_NOT_IMPLEMENTED it is not VIEW + @retval HA_ADMIN_WRONG_CHECKSUM check sum is wrong +*/ + +int view_repair(THD *thd, TABLE_LIST *view, HA_CHECK_OPT *check_opt) +{ + DBUG_ENTER("view_repair"); + bool swap_alg= (check_opt->sql_flags & TT_FROM_MYSQL); + bool wrong_checksum= view_checksum(thd, view);

view_checksum() != HA_ADMIN_OK

+ int ret; + if (wrong_checksum || swap_alg || (!view->mariadb_version)) + { + ret= mariadb_fix_view(thd, view, wrong_checksum, swap_alg); + DBUG_RETURN(ret); + } + DBUG_RETURN(HA_ADMIN_OK); +} + /* rename view

Regards, Sergei