← Back to team overview

maria-developers team mailing list archive

Re: Rev 4407: MDEV-6916: Upgrade from MySQL to MariaDB breaks already created views

 

Hi, Sanja!

On Feb 09, sanja@xxxxxxxxxxxx wrote:
> At file:///home/bell/maria/bzr/work-maria-5.5-MDEV-6916-check_view/
> 
> ------------------------------------------------------------
> revno: 4407
> revision-id: sanja@xxxxxxxxxxxx-20150209014828-kx3cr36hgrvjhtrg
> parent: svoj@xxxxxxxxxxx-20150114135038-v50g2cul4vce63h8
> committer: sanja@xxxxxxxxxxxx
> 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


Follow ups