>> > No "Phase 0" please, renumber all phases to start from 1. >> >> Took opportunity to make adding/changing phases to be lest conflicts in merge >> >> https://github.com/openquery/mariadb-server/commit/25872e2802a4ae32d4f18132d569599295c963cd > >I don't know whether it's a good idea. > > It means that, for example "Running 'mysql_fix_privilege_tables'..." > won't always be Phase 3/3, but depending on command-line options it > might be a Phase 2 or 1 (well, may be mysql_fix_privilege_tables phase > cannot - but there certainly are phases that are run conditionally). > > I personally don't know how bad it is, perhaps it's fine to renumber > phases dynamically when some phases are skipped. So I have no opinion. > I'm just saying that phases might be renumbered dynamically after your > patch.
Well consistent phase numbering to activity is a good goal. So is knowing why phases that just don't occur. So is having a total phases that is actually reached. So produce output for every phase, even if it just says we're skipping it: https://github.com/MariaDB/server/commit/808608cb3f4e10ba81679c94b2a299a32cf5e448 > I believe I meant these three variants: > > --upgrade-views = no don't change view frm's > --upgrade-views = yes upgrade view frms adding mariadb_version > field and if is_mysql() returned true > also toggle the view algorithm > --upgrade-views = from_mysql always toggle the view algorithm, even > if is_mysql() returned false. Right is_mysql is in mysql_upgrade and --upgrade-view is mysqlcheck. Always exclusing if there is a mariadb-version in in the frm I assume. > The last case is useful if a user has migrated from MySQL some time ago, > so it's a mariadb-to-mariadb upgrade but views stay wrong from older > mysql-to-mariadb upgrade. Added: https://github.com/MariaDB/server/commit/97e0aeaf72cde117b8d9551f68d407fa13aafbe1 I'm almost thinking mysqlcheck should default to --upgrade-views=yes? At least it will do a checksum but otherwise won't rewrite a view unless there is missing mariadb-version note mysql_upgrade: is_mysql detected run with --upgrade_views=from_mysql --upgrade-system-tables - upgrade_views not run otherwise --upgrade_views=yes to get old views upgraded (and checksum checked). https://github.com/MariaDB/server/commit/4987080ddb42abf1a9111bd6a79e7bed746b66ff > > 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 Has been renamed to --upgrade-views and it doesn't skip all tables. This is done with --skip-fix-tables https://github.com/MariaDB/server/commit/fc277cd4ba6c506a6f6c71e04c462b24a3fcfb26 > > --- 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 ? done: https://github.com/MariaDB/server/commit/c584058f8f227a2004f9fdf6fd0897e11fd53eda > Please add also a test to show how REPAIR VIEW is replicated. Doesn't > have to be a replication test, really. I'd say that "show binlog events" > after the REPAIR VIEW should suffice. Use ---- done: https://github.com/MariaDB/server/commit/c584058f8f227a2004f9fdf6fd0897e11fd53eda > > === 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. it was unnecessary - removed https://github.com/MariaDB/server/commit/7229b19c67d12ff579c64f1184e951015aa2f9c9 > > === 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 (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. corrected: https://github.com/MariaDB/server/commit/29721d7d5f85fe323d70f1856aa1e5294e14074a > > + > > + 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. Already covered earlier in the function > > +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 fixed. like below. (https://github.com/MariaDB/server/commit/8a827d530a67d40332668d31683a45f583974935) > 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() :) I was too lazy. sorry. > Second, REPAIR VIEW viewname - without FROM MYSQL - should work too, it > should add mariadb_version field without changing the algorithm. ok. Done - https://github.com/MariaDB/server/commit/76c18f7e76dff6f5c5f8986e0480074a3100fdbe > > === 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? corrected: https://github.com/MariaDB/server/commit/28b173134ee1634a2489d3cef6faf2f3ea1f6ab4 -- -- Daniel Black, Engineer @ Open Query (http://openquery.com.au) Remote expertise & maintenance for MySQL/MariaDB server environments. _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
