On 09.02.15 15:19, Sergei Golubchik wrote:
Hi, Sanja!
On Feb 09, [email protected] wrote:
At file:///home/bell/maria/bzr/work-maria-5.5-MDEV-6916-check_view/
------------------------------------------------------------
revno: 4407
revision-id: [email protected]
parent: [email protected]
committer: [email protected]
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()
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 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.
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.
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.
-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?
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_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.
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)
+ }
+ 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?
and write binlog also? I just tried to remove that syntax for view
(maybe wrong but it was not discussed before)
+ }
lex->check_opt.init();
lex->alter_info.reset();
/* Will be overriden during execution. */
Regards,
Sergei
_______________________________________________
Mailing list: https://launchpad.net/~maria-developers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
