Hi, Sanja! Comments about the code, see below. Comments about the whole approach - see my other email. On Dec 07, sanja@askmonty.org wrote:

At file:///home/bell/maria/bzr/work-maria-5.5-MDEV-6916/

------------------------------------------------------------ revno: 4383 revision-id: sanja@askmonty.org-20141207094825-tx163unk3mm4gkql parent: sanja@askmonty.org-20141203234414-63zkpeq9eb0n9lc1 committer: sanja@askmonty.org branch nick: work-maria-5.5-MDEV-6916 timestamp: Sun 2014-12-07 10:48:25 +0100 message: MDEV-6916:Upgrade from MySQL to MariaDB breaks already created views (SQL-level commands to support view update)

=== modified file 'mysql-test/r/view.result' --- a/mysql-test/r/view.result 2014-12-03 22:37:59 +0000 +++ b/mysql-test/r/view.result 2014-12-07 09:48:25 +0000 @@ -5398,6 +5398,29 @@ DROP VIEW v1; DROP TABLE t1, t2; create view v1 as select 1; drop view v1; +# +#MDEV-6916:Upgrade from MySQL to MariaDB breaks already created views +# (SQL command for upgrade check) +# +create table t1 (a int); +create algorithm=temptable view v1 as select a from t1; +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` latin1 latin1_swedish_ci +check table v1 force mariadb upgrade; +Table Op Msg_type Msg_text +test.v1 check note View algorithm swap +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` latin1 latin1_swedish_ci

No, I don't like that. FORCE or not, upgrading of a view should not blindly change the alorithm. This functionality is created to solve a specific problem. So when a view has mariadb-version field we know with certainty that the algorithm is correct and it should *not* be changed with or without FORCE. In the test you'll need to copy mysql-created view frm (from std_data). Addendum: ok, I've read till the end. See my comment in view_check() function.

+check table v1 force mariadb upgrade;

Also, I don't like that CHECK changes the algorithm of the view. For tables CHECK only performs the *check* (and updates the version in the frm). It's REPAIR that does the change. To be consistent with the behavior for tables, CHECK ... FOR UPGRADE should only report an error, like "view needs upgrading" and a user should then use REPAIR to actually fix the view.

+Table Op Msg_type Msg_text +test.v1 check note View algorithm swap +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` latin1 latin1_swedish_ci +drop view v1; +drop table t1; # ----------------------------------------------------------------- # -- End of 5.5 tests. # -----------------------------------------------------------------

=== 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 2014-12-07 09:48:25 +0000 @@ -6565,3 +6565,5 @@ 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_VIEW_UPGRADE_IS_DONE + eng "View algorithm swap"

No way, you cannot add a new error message in 5.5 because the next major branch (10.0) is already GA.

=== modified file 'sql/sql_admin.cc' --- a/sql/sql_admin.cc 2014-05-03 16:12:17 +0000 +++ b/sql/sql_admin.cc 2014-12-07 09:48:25 +0000 @@ -314,7 +314,8 @@ static bool mysql_admin_table(THD* thd, HA_CHECK_OPT *), int (handler::*operator_func)(THD *, HA_CHECK_OPT *), - int (view_operator_func)(THD *, TABLE_LIST*)) + int (view_operator_func)(THD *, TABLE_LIST*, + HA_CHECK_OPT *)) { TABLE_LIST *table; SELECT_LEX *select= &thd->lex->select_lex; @@ -521,7 +522,7 @@ static bool mysql_admin_table(THD* thd, ER_CHECK_NO_SUCH_TABLE, ER(ER_CHECK_NO_SUCH_TABLE)); /* if it was a view will check md5 sum */ if (table->view && - view_checksum(thd, table) == HA_ADMIN_WRONG_CHECKSUM) + view_check(thd, table, check_opt) == HA_ADMIN_WRONG_CHECKSUM) push_warning(thd, MYSQL_ERROR::WARN_LEVEL_WARN, ER_VIEW_CHECKSUM, ER(ER_VIEW_CHECKSUM)); if (thd->stmt_da->is_error() && @@ -536,7 +537,7 @@ static bool mysql_admin_table(THD* thd, if (table->view) { DBUG_PRINT("admin", ("calling view_operator_func")); - result_code= (*view_operator_func)(thd, table); + result_code= (*view_operator_func)(thd, table, check_opt); goto send_result; }

@@ -849,6 +850,14 @@ send_result_message: system_charset_info); break; } + case HA_ADMIN_VIEW_UPGRADE_IS_DONE: + { + protocol->store(STRING_WITH_LEN("note"), system_charset_info); + protocol->store(ER(ER_VIEW_UPGRADE_IS_DONE), + strlen(ER(ER_VIEW_UPGRADE_IS_DONE)), + system_charset_info); + break; + }

Don't do that. First, you cannot have ER_VIEW_UPGRADE_IS_DONE. Second, it's simply unnecessary. Return HA_ADMIN_OK from the view_operator_func() and use push_warning inside view_operator_func() to say that the algorithm was changed.

case HA_ADMIN_NEEDS_UPGRADE: case HA_ADMIN_NEEDS_ALTER: @@ -1071,7 +1080,7 @@ bool Check_table_statement::execute(THD

res= mysql_admin_table(thd, first_table, &m_lex->check_opt, "check", lock_type, 0, 0, HA_OPEN_FOR_REPAIR, 0, - &handler::ha_check, &view_checksum); + &handler::ha_check, &view_check);

m_lex->select_lex.table_list.first= first_table; m_lex->query_tables= first_table;

=== modified file 'sql/sql_view.cc' --- a/sql/sql_view.cc 2014-12-03 23:44:14 +0000 +++ b/sql/sql_view.cc 2014-12-07 09:48:25 +0000 @@ -729,9 +729,29 @@ err: DBUG_RETURN(res || thd->is_error()); }

+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; +static const int required_view_parameters= 16;

eh? why?

/* table of VIEW .frm field descriptors @@ -795,6 +815,65 @@ 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) +{ + 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) + 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); + } + + DBUG_ASSERT(view->algorithm == VIEW_ALGORITHM_MERGE || + view->algorithm == VIEW_ALGORITHM_TMPTABLE);

if() instead of an assert, please. while unlikely, it's possible that the file would be changed between two opens.

+ if (view->algorithm == VIEW_ALGORITHM_MERGE) + view->algorithm= VIEW_ALGORITHM_TMPTABLE; + else + view->algorithm= VIEW_ALGORITHM_MERGE; + + if (sql_create_definition_file(&dir, &file, view_file_type, + (uchar*)view, view_parameters)) + { + sql_print_error("View '%-.192s'.'%-.192s': algorithm swap error.",

"View %`s.%`s: cannot change the algorithm from %s to %s" And push_warning_printf() with the same. Or, rather, only push_warning_printf without sql_print_information.

+ view->db, view->table_name); + DBUG_RETURN(HA_ADMIN_INTERNAL_ERROR); + } + sql_print_information("View '%-.192s'.'%-.192s': algorithm swapped to '%s'",

"View %`s.%`s: algorithm changed from %s to %s" And push_warning_printf() with the same. Or, rather, only push_warning_printf without sql_print_information.

+ view->db, view->table_name, + (view->algorithm == VIEW_ALGORITHM_MERGE)? + "MERGE":"TEMPTABLE"); + + + DBUG_RETURN(HA_ADMIN_VIEW_UPGRADE_IS_DONE); +} + + /* Register VIEW (write .frm & process .frm's history backups)

@@ -931,16 +1010,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) @@ -1953,6 +2025,33 @@ int view_checksum(THD *thd, TABLE_LIST * HA_ADMIN_OK); }

you can make view_checksum() static now.

+/** + 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_MARIADB_UPGRADE) && + !view->mariadb_version) || + ((check_opt->sql_flags & (TT_MARIADB_UPGRADE | TT_FORCE))))

What is the point? Why would one ever want to change the algorithm of the provably created-in-mariadb view and pretend that it's an "upgrade"? To change the algorithm one should use ALTER. Fixing the view after an upgrade is not altering it, it's *fixing after an upgrade*. And views that were created in mariadb don't need to be fixed after an upgrade. No FORCE please.

+ { + DBUG_PRINT("XXX", ("Upgrade"));

please either remove this DBUG_PRINT or change it to use a reasonable keyword.

+ DBUG_RETURN(mariadb_fix_view(thd, view)); + } + DBUG_RETURN(HA_ADMIN_OK); +} + === modified file 'sql/sql_yacc.yy' --- a/sql/sql_yacc.yy 2014-11-08 18:54:42 +0000 +++ b/sql/sql_yacc.yy 2014-12-07 09:48:25 +0000 @@ -7299,6 +7300,10 @@ mi_check_type: | EXTENDED_SYM { Lex->check_opt.flags|= T_EXTEND; } | CHANGED { Lex->check_opt.flags|= T_CHECK_ONLY_CHANGED; } | FOR_SYM UPGRADE_SYM { Lex->check_opt.sql_flags|= TT_FOR_UPGRADE; } + | MARIADB_SYM UPGRADE_SYM + { Lex->check_opt.sql_flags|= TT_MARIADB_UPGRADE; } + | FORCE_SYM MARIADB_SYM UPGRADE_SYM + { Lex->check_opt.sql_flags|= (TT_MARIADB_UPGRADE | TT_FORCE); } ;

optimize:

Regards, Sergei