Hi, Sanja!
On Feb 09, sanja(a)askmonty.org wrote:
> At file:///home/bell/maria/bzr/work-maria-5.5-MDEV-6916-check_view/
>
> ------------------------------------------------------------
> revno: 4407
> revision-id: sanja(a)askmonty.org-20150209014828-kx3cr36hgrvjhtrg
> parent: svoj(a)mariadb.org-20150114135038-v50g2cul4vce63h8
> committer: sanja(a)askmonty.org
> 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 views
>
> CHECK/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()
> {"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.
> + 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.
===
> + 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 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.
> 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
> + }
>
> 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.
> 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.
> + 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 ?
> +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_check
You'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.
> #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.
> + 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.
> + {
> + 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() :)
Second, REPAIR VIEW viewname - without FROM MYSQL - should work too, it
should add mariadb_version field without changing the algorithm.
> + }
> + 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
