maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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@xxxxxxxxxxxx wrote:
> At file:///home/bell/maria/bzr/work-maria-5.5-MDEV-6916/
> 
> ------------------------------------------------------------
> revno: 4384
> revision-id: sanja@xxxxxxxxxxxx-20141211180458-j8ajlibnvvydmj40
> parent: sanja@xxxxxxxxxxxx-20141207094825-tx163unk3mm4gkql
> committer: sanja@xxxxxxxxxxxx
> 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