maria-developers team mailing list archive
-
maria-developers team
-
Mailing list archive
-
Message #08015
Re: Rev 4383: MDEV-6916:Upgrade from MySQL to MariaDB breaks already created views
Hi, Sanja!
Comments about the code, see below.
Comments about the whole approach - see my other email.
On Dec 07, sanja@xxxxxxxxxxxx wrote:
> At file:///home/bell/maria/bzr/work-maria-5.5-MDEV-6916/
>
> ------------------------------------------------------------
> revno: 4383
> revision-id: sanja@xxxxxxxxxxxx-20141207094825-tx163unk3mm4gkql
> parent: sanja@xxxxxxxxxxxx-20141203234414-63zkpeq9eb0n9lc1
> committer: sanja@xxxxxxxxxxxx
> 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
