| Thread Previous • Date Previous • Date Next • Thread Next |
On 09.02.15 15:19, Sergei Golubchik wrote:
Hi, Sanja! On Feb 09, sanja@xxxxxxxxxxxx wrote:At file:///home/bell/maria/bzr/work-maria-5.5-MDEV-6916-check_view/ ------------------------------------------------------------ revno: 4407 revision-id: sanja@xxxxxxxxxxxx-20150209014828-kx3cr36hgrvjhtrg parent: svoj@xxxxxxxxxxx-20150114135038-v50g2cul4vce63h8 committer: sanja@xxxxxxxxxxxx branch nick: work-maria-5.5-MDEV-6916-check_view timestamp: Mon 2015-02-09 02:48:28 +0100 message: MDEV-6916: Upgrade from MySQL to MariaDB breaks already created viewsCHECK/REPAIR commands and mysql_upgradesupport for upgrade from MySQL server support.=== modified file 'client/mysql_upgrade.c' --- a/client/mysql_upgrade.c 2014-03-27 21:26:58 +0000 +++ b/client/mysql_upgrade.c 2015-02-09 01:48:28 +0000 @@ -150,6 +151,14 @@ static struct my_option my_long_options[ &opt_not_used, &opt_not_used, 0, GET_BOOL, NO_ARG, 1, 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', + "Skip automatic detection MySQL and assume that we upgrade it", + &opt_mysql_upgrade, &opt_mysql_upgrade, 0, GET_BOOL, NO_ARG, + 0, 0, 0, 0, 0, 0}, + {"skip-mysql-upgrade", 'Y', + "Skip view algorithm upgrade from MySQL", + &opt_skip_mysql_upgrade, &opt_skip_mysql_upgrade, 0, GET_BOOL, NO_ARG, + 0, 0, 0, 0, 0, 0},I remember I've already commented on that in my previous review. Here you are, again: === Those two are very bad option names. First, $ mysql_upgrade --mysql-upgrade looks plain weird. $ mysql_upgrade --skip-mysql-upgrade is even worse. And what about $ mysql_upgrade --mysql-upgrade --skip-mysql-upgrade Second, "skip" is the prefix automatically recognized by my_getopt. Normally, --skip-xxx is the same as --xxx=0. But now you're introducing --skip-mysql-upgrade which is very different from --skip-mysql-upgrade. While simultaneously adding support for --skip-skip-mysql-upgrade. A better option would be --fix-view-algorithm = { AUTO | ALWAYS | NEVER } === In fact, I'd rather remove this option completely. You can simply add if (!is_mysql()) return; to run_mysqlcheck_views()
Ok let it be no arguments.
{"version-check", 'k', "Run this program only if its \'server version\' " "matches the version of the server to which it's connecting, (enabled by " "default); use --skip-version-check to avoid this check. Note: the \'server " @@ -344,6 +353,14 @@ get_one_option(int optid, const struct m case OPT_DEFAULT_AUTH: /* --default-auth */ add_one_option(&conn_args, opt, argument); break; + case 'y': + opt_mysql_upgrade= 1; + add_option= FALSE; + break; + case 'Y': + opt_skip_mysql_upgrade= 1; + add_option= FALSE;This is a wrong way of implementing command-line options. Let my_getopt do its job and don't set values manually.
above code is full exactly the same examples that I did it this way. But arguing has no sens because it will be no arguments.
+ break; }if (add_option)@@ -754,6 +771,23 @@ static int run_mysqlcheck_upgrade(void) NULL); }+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. ===
OK
+ print_conn_args("mysqlcheck"); + return run_tool(mysqlcheck_path, + NULL, /* Send output from mysqlcheck directly to screen */ + "--no-defaults", + ds_args.str, + "--all-databases", + "--mysql-upgrade", + opt_verbose ? "--verbose": "", + opt_silent ? "--silent": "", + "2>&1", + NULL); +}static int run_mysqlcheck_fixnames(void){ === 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 mcase '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?
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?
+ }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.
-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?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.
I can add 2 lists and processing for CHECK/RAPAIR VIEW/TABLE but usage will be limited IMHO.
+ tables, options); table_name= tables; } else { char *ptr, *org;- org= ptr= strmov(strmov(query, op), " TABLE ");+ org= ptr= strmov(strmov(query, op), + (opt_mysql_upgrade ? " VIEW " : " TABLE ")); ptr= fix_table_name(ptr, tables); strmake(table_name_buff, org, min((int) sizeof(table_name_buff)-1, (int) (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.
+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/handler.h' --- a/sql/handler.h 2015-01-13 18:28:03 +0000 +++ b/sql/handler.h 2015-02-09 01:48:28 +0000 @@ -42,6 +42,7 @@// the following is for checking tables +#define HA_ADMIN_VIEW_REPAIR_IS_DONE 2#define HA_ADMIN_ALREADY_DONE 1 #define HA_ADMIN_OK 0 #define HA_ADMIN_NOT_IMPLEMENTED -1 @@ -56,6 +57,7 @@ #define HA_ADMIN_NEEDS_UPGRADE -10 #define HA_ADMIN_NEEDS_ALTER -11 #define HA_ADMIN_NEEDS_CHECK -12 +#define HA_ADMIN_NEEDS_REPAIR -13/* Bits in table_flags() to show what database can do */ === 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.+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.+ { + 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?+ { + 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?retry_share:mysql_mutex_lock(&LOCK_open); if (!(share= get_table_share_with_discover(thd, table_list, key,- key_length, OPEN_VIEW, + key_length, + (OPEN_VIEW | + ((table_list->required_type == + FRMTYPE_VIEW) ? + OPEN_VIEW_ONLY : 0)), &error, hash_value))) { === modified file 'sql/sql_table.cc' --- a/sql/sql_table.cc 2015-01-14 11:10:13 +0000 +++ b/sql/sql_table.cc 2015-02-09 01:48:28 +0000 @@ -28,7 +28,7 @@ #include "sql_base.h" // open_table_uncached, lock_table_names #include "lock.h" // mysql_unlock_tables #include "strfunc.h" // find_type2, find_set -#include "sql_view.h" // view_checksum +#include "sql_view.h" // view_checkYou've modified only the comment, not the actual C++ code that uses view_checksum in this file. So, either the comment is wrong (view_checksum is not used in this file) and should be removed, or the whole #include is unnecessary.
OK
#include "sql_truncate.h" // regenerate_locked_table #include "sql_partition.h" // mem_alloc_error, // generate_partition_syntax, === modified file 'sql/sql_view.cc' --- a/sql/sql_view.cc 2014-12-21 18:23:28 +0000 +++ b/sql/sql_view.cc 2015-02-09 01:48:28 +0000 @@ -729,6 +729,26 @@ err: }+static void make_view_filename(LEX_STRING *dir, char *dir_buff,+ size_t dir_buff_len, + LEX_STRING *path, char *path_buff, + size_t path_buff_len, + LEX_STRING *file, + TABLE_LIST *view) +{ + /* print file name */ + dir->length= build_table_filename(dir_buff, dir_buff_len - 1, + view->db, "", "", 0); + dir->str= dir_buff; + + path->length= build_table_filename(path_buff, path_buff_len - 1, + view->db, view->table_name, reg_ext, 0); + path->str= path_buff; + + file->str= path->str + dir->length; + file->length= path->length - dir->length; +} + /* number of required parameters for making view */ static const int required_view_parameters= 15;@@ -791,6 +811,81 @@ 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 (view->algorithm == VIEW_ALGORITHM_UNDEFINED && + !wrong_checksum && view->mariadb_version) + DBUG_RETURN(HA_ADMIN_OK);Eh? Why do you care what view->algorithm is? It doesn't matter whether it's undefined, merge, or temptable - if mariadb_version is set, the algorithm is always correct.
it was checked by upper function.
+ 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); + } + + if (swap_alg && view->algorithm != VIEW_ALGORITHM_UNDEFINED)only if mariadb_version is not set.
it was checked by upper function and swap_alg parameter was set (or not)
+ { + 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; + } + 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': algorithm swapped to '%s'", + view->db, view->table_name, + (view->algorithm == VIEW_ALGORITHM_MERGE)? + "MERGE":"TEMPTABLE"); + + + DBUG_RETURN(HA_ADMIN_VIEW_REPAIR_IS_DONE); +} + + /* Register VIEW (write .frm & process .frm's history backups)@@ -927,17 +1022,9 @@ static int mysql_register_view(THD *thd,} loop_out: /* print file name */ - dir.length= build_table_filename(dir_buff, sizeof(dir_buff) - 1, - view->db, "", "", 0); - dir.str= dir_buff; - - path.length= build_table_filename(path_buff, sizeof(path_buff) - 1, - view->db, view->table_name, reg_ext, 0); - path.str= path_buff; - - file.str= path.str + dir.length; - file.length= path.length - dir.length; - + 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; @@ -1941,6 +2028,63 @@ int view_checksum(THD *thd, TABLE_LIST * 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_NO_MARIADB_SERVER_FIELD, + ER(ER_NO_MARIADB_SERVER_FIELD), + view->db, + view->table_name); + DBUG_RETURN(HA_ADMIN_NEEDS_REPAIR); + } + 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) && + (!view->mariadb_version)); + bool wrong_checksum= view_checksum(thd, view); + if (wrong_checksum || swap_alg) + { + DBUG_RETURN(mariadb_fix_view(thd, view, wrong_checksum, swap_alg));First, don't call functions from DBUG_RETURN, this produces weird debug traces and somebody will have to fix this later. Always int res= mariadb_fix_view(thd, view, wrong_checksum, swap_alg); DBUG_RETURN(res); Alternatively, feel free to fix dbug macros to support function calls in DBUG_RETURN() :)
OK (new code style)
Second, REPAIR VIEW viewname - without FROM MYSQL - should work too, it should add mariadb_version field without changing the algorithm.
yes it should (at least code was written so)
and write binlog also? I just tried to remove that syntax for view (maybe wrong but it was not discussed before)+ } + DBUG_RETURN(HA_ADMIN_OK); +} + /* rename view === modified file 'sql/sql_yacc.yy' --- a/sql/sql_yacc.yy 2014-11-08 18:54:42 +0000 +++ b/sql/sql_yacc.yy 2015-02-09 01:48:28 +0000 @@ -1145,6 +1145,7 @@ bool my_yyoverflow(short **a, YYSTYPE ** %token MULTIPOINT %token MULTIPOLYGON %token MUTEX_SYM +%token MYSQL_SYM %token MYSQL_ERRNO_SYM %token NAMES_SYM /* SQL-2003-N */ %token NAME_SYM /* SQL-2003-N */ @@ -7191,11 +7192,16 @@ opt_checksum_type: ;repair:- REPAIR opt_no_write_to_binlog table_or_tables + REPAIR opt_no_write_to_binlog table_or_view { LEX *lex=Lex; lex->sql_command = SQLCOM_REPAIR; lex->no_write_to_binlog= $2; + if (lex->no_write_to_binlog && lex->only_view) + { + my_parse_error(ER(ER_SYNTAX_ERROR)); + MYSQL_YYABORT;Why? REPAIR NO_WRITE_TO_BINLOG VIEW makes perfect sense to me, why did you want to disallow it?
+ } lex->check_opt.init(); lex->alter_info.reset(); /* Will be overriden during execution. */Regards, Sergei
| Thread Previous • Date Previous • Date Next • Thread Next |
