Hi, Sanja! Ok, I've seen all four patches. Specific comments about the code are below and in other emails. But the main question is - why are you doing it, what problem does it solve. As you said, the use case is - a user upgrades *from MySQL* and *runs mysql_upgrade*. To cover this use case mysql_upgrade needs to 1) detect whether it's an upgrade from MySQL, and to 2) run a command to change the algorithm of all views. You've done a lot of changes that aren't related to that. If this is, indeed, the use case, than you only need to: 1. add MySQL detection in mysql_upgrade. You have it, is_mysql() function. 2. add the code to toggle the algorithm of all views. See? No command line options, no FORCE, no CHECK, nothing. Now, let's consider extensions to this minimal functionality. 3. mariadb-version in the view frm file. this is a good idea, makes views a lot more future-proof. ok. I don't see why other extensions might be needed. Now, how to toggle the algorithm. The most natural solution would be to use ALTER VIEW. Like ALTER VIEW v1 ALGORITHM=MERGE. Unfortunately, ALTER VIEW doesn't work that way, I don't know why it was implemented in that crazy way in MySQL, but it's mostly useless. CHECK should not change its arguments. REPAIR? May be. I'd still prefer ALTER, but I don't know whether it can be extended in 5.5 in a safe way. If not then REPAIR ... AFTER UPGRADE is probably the second best. Or even REPAIR VIEW ... AFTER UPGRADE. It won't conflict with anything, doesn't add new keywords. Drawback - we'll be stuck with that hack for quite a while :( Even better would be to make mysql_upgrade to scan the data directory and rewrite view frms. No hacks or new grammar in the server. No new features in 5.5 GA. Nothing to maintain for years. Almost perfect. But it requires mysql_upgrade to have write access to the datadir. It kind of needs it already, but it's optional. With this approach it'll be less optional than before. So, I don't know... The most reasonable and future-proof solution would be to use ALTER TABLE. But I don't know whether we'll be able to do it in 5.5, looks a bit risky. Then there are reasonably safe hacks like REPAIR VIEW. And most safe and quite clean mysql_upgrade only approach, that needs write access to the datadir. Opinions? Anyway, here are comments about the code, below: On Dec 11, sanja@askmonty.org wrote:
At file:///home/bell/maria/bzr/work-maria-5.5-MDEV-6916/
------------------------------------------------------------ revno: 4384 revision-id: sanja@askmonty.org-20141211180458-j8ajlibnvvydmj40 parent: sanja@askmonty.org-20141207094825-tx163unk3mm4gkql committer: sanja@askmonty.org branch nick: work-maria-5.5-MDEV-6916 timestamp: Thu 2014-12-11 19:04:58 +0100 message: MDEV-6916: Upgrade from MySQL to MariaDB breaks already created views
mysql_upgrade now fix views from mysql.
(+ fix new view detection)
=== modified file 'client/mysql_upgrade.c' --- a/client/mysql_upgrade.c 2014-03-27 21:26:58 +0000 +++ b/client/mysql_upgrade.c 2014-12-11 18:04:58 +0000 @@ -40,7 +40,8 @@ static char mysql_path[FN_REFLEN]; static char mysqlcheck_path[FN_REFLEN];
static my_bool opt_force, opt_verbose, debug_info_flag, debug_check_flag, - opt_systables_only, opt_version_check; + opt_systables_only, opt_version_check, + opt_mysql_upgrade= 0, opt_skip_mysql_upgrade= 0; static my_bool opt_not_used, opt_silent; static uint my_end_arg= 0; static char *opt_user= (char*)"root"; @@ -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},
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 }
{"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 " @@ -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");
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", + "--view-upgrade", + opt_verbose ? "--verbose": "", + opt_silent ? "--silent": "", + "2>&1", + NULL); +}
=== modified file 'client/mysqlcheck.c' --- a/client/mysqlcheck.c 2014-02-17 10:00:51 +0000 +++ b/client/mysqlcheck.c 2014-12-11 18:04:58 +0000 @@ -42,7 +42,8 @@ static my_bool opt_alldbs = 0, opt_check opt_medium_check = 0, opt_quick = 0, opt_all_in_1 = 0, opt_silent = 0, opt_auto_repair = 0, ignore_errors = 0, tty_password= 0, opt_frm= 0, debug_info_flag= 0, debug_check_flag= 0, - opt_fix_table_names= 0, opt_fix_db_names= 0, opt_upgrade= 0; + opt_fix_table_names= 0, opt_fix_db_names= 0, opt_upgrade= 0, + opt_mariadb_upgrade= 0, opt_f_mariadb_upgrade= 0; static my_bool opt_write_binlog= 1, opt_flush_tables= 0; static uint verbose = 0, opt_mysql_port=0; static int my_end_arg; @@ -196,6 +197,12 @@ 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}, + {"view-upgrade", 'f', + "Fix view algorithm view field if it is not new MariaDB view.",
"Repair algorithm value for views created by MySQL (or MariaDB before 5.5.41)"
+ 0, 0, 0, GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0}, + {"force-view-upgrade", 'F', + "Swap view algorithm view field.",
"Always change view algorithm"
+ 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} };
Regards, Sergei