← 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, Daniel!

Thanks!

I've now reviewed the fix as a whole, all patches combined.
There weren't many comments, but still there were some. And questions.

If you'd like I can apply the latest changes (finishing touches, so to
say) myself - it can speed things up. Or you can do that, whatever you
prefer. Either way, I'd still like to see my questions answered first.

> diff --git a/client/client_priv.h b/client/client_priv.h
> index e206804..a66c388 100644
> --- a/client/client_priv.h
> +++ b/client/client_priv.h
> @@ -79,6 +79,7 @@ enum options_client
>    OPT_SLAP_COMMIT,
>    OPT_SLAP_DETACH,
>    OPT_SLAP_NO_DROP,
> +  OPT_UPGRADE_VIEWS,

please remove. see below

>    OPT_MYSQL_REPLACE_INTO, OPT_BASE64_OUTPUT_MODE, OPT_SERVER_ID,
>    OPT_FIX_TABLE_NAMES, OPT_FIX_DB_NAMES, OPT_SSL_VERIFY_SERVER_CERT,
>    OPT_AUTO_VERTICAL_OUTPUT,
> diff --git a/client/mysqlcheck.c b/client/mysqlcheck.c
> index a454943..43c7ff0 100644
> --- a/client/mysqlcheck.c
> +++ b/client/mysqlcheck.c
> @@ -57,6 +58,20 @@ static uint opt_protocol=0;
>  
>  enum operations { DO_CHECK=1, DO_REPAIR, DO_ANALYZE, DO_OPTIMIZE, DO_UPGRADE };
>  
> +typedef enum {
> +  UPGRADE_VIEWS_NO=0,
> +  UPGRADE_VIEWS_YES=1,
> +  UPGRADE_VIEWS_FROM_MYSQL=2,
> +  UPGRADE_VIEWS_COUNT

UPGRADE_VIEWS_COUNT is unused, please remove

> +} enum_upgrade_views;
> +const char *upgrade_views_opts[]=
> +{"NO", "YES", "FROM_MYSQL", NullS};
> +TYPELIB upgrade_views_typelib=
> +  { array_elements(upgrade_views_opts) - 1, "",
> +    upgrade_views_opts, NULL };
> +static enum_upgrade_views opt_upgrade_views= UPGRADE_VIEWS_NO;
> +static char *opt_upgrade_views_str= NullS;
> +
>  static struct my_option my_long_options[] =
>  {
>    {"all-databases", 'A',
> @@ -196,6 +211,15 @@ 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},
> +  {"upgrade-views", OPT_UPGRADE_VIEWS,
> +   "Repairs/Upgrades views. 'No' (default) doesn't do anything to views. "
> +   "'Yes' repairs the checksum and adds a MariaDB version to the frm
> table defination (if missing)."
> +   "Using 'from_mysql' will, when upgrading from MySQL, and ensure the view algorithms are the"
> +   "same as what they where previously.",

"definition"
And the whole message could be reworded. for example

  "Repair views. One of: NO, YES (correct the checksum, if necessary,
  add the mariadb-version field), FROM_MYSQL (same as YES and toggle the
  algorithm MERGE<->TEMPTABLE)."

In 10.1 mysqld --help automatically adds "One of: <list of values>" to
all enum options, but only if the first option is not already present in
the help text in the same letter case (it uses strstr for that).

It's 5.5 but better to mention options in the same leter case and use
the same "One of:" style to blend in better.

> +   &opt_upgrade_views_str, &opt_upgrade_views_str, 0, GET_STR, OPT_ARG, 0, 0, 0, 0, 0, 0},

not GET_STR but GET_ENUM with 0 instead of OPT_UPGRADE_VIEWS

> +  {"fix-tables", 'Y',
> +   "Fix tables. Usually the default however mysql_upgrade will run with --skip-fix-tables.",

"Usually the default..." is redundant, my_getopt will say it automatically

btw, it's mysqlcheck tool, and these two options are symmetrical, in a
sense.  so it'd be better to have matching names. may be

   --process-tables = no/yes
   --process-views = no/yes/from_mysql ?

it's a bit strange to have --process-views=from_mysql. A more logical
variant could be --process-views=upgrade_from_mysql, but it looks too long
for me.  on the other hand, upgrade_from_mysql value is not for manual
use, it's supposed to be specified by mysql_upgrade, so it doesn't mater
if it's long

> +   &opt_fix_tables, &opt_fix_tables, 0, GET_BOOL, NO_ARG, 1, 0, 0, 0, 0, 0},
>    {0, 0, 0, 0, 0, 0, GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0}
>  };
>  
> @@ -332,7 +356,18 @@ get_one_option(int optid, const struct my_option *opt __attribute__((unused)),
>    case 'v':
>      verbose++;
>      break;
> -  case 'V': print_version(); exit(0);
> +  case 'V':
> +    print_version(); exit(0);
> +    break;
> +  case OPT_UPGRADE_VIEWS:
> +    if (argument == NULL)
> +      opt_upgrade_views= UPGRADE_VIEWS_NO;
> +    else
> +    {
> +      opt_upgrade_views= (enum_upgrade_views)
> +        (find_type_or_exit(argument, &upgrade_views_typelib, opt->name)-1);
> +    }
> +    break;

remove, use GET_ENUM

>    case OPT_MYSQL_PROTOCOL:
>      opt_protocol= find_type_or_exit(argument, &sql_protocol_typelib,
>                                      opt->name);
> @@ -363,6 +398,23 @@ static int get_options(int *argc, char ***argv)
>    if ((ho_error=handle_options(argc, argv, my_long_options, get_one_option)))
>      exit(ho_error);
>  
> +  if (what_to_do==DO_REPAIR && !(opt_upgrade_views || opt_fix_tables))
> +  {
> +    fprintf(stderr, "Error:  %s doesn't support repair command without either upgrade-views or fix-tables specified.\n",

I'd reformulate. it's not "doesn't support",  it's more like "command
makes no sense", so I'd say "Nothing to repair when both
--process-tables=NO and --process-views=NO"

> +            my_progname);
> +    exit(1);
> +  }
> +  if (opt_upgrade_views && what_to_do!=DO_REPAIR)
> +  {
> +    if (!what_to_do)
> +      what_to_do= DO_REPAIR;
> +    else
> +    {
> +      fprintf(stderr, "Error:  %s doesn't support non-repair command with option upgrade-views.\n",
> +              my_progname);

Better "%s of views is not supported", "analyzing"/"optimizing"/"checking"
(depending on what_to_do)

although I don't see why CHECK VIEW should not be supported

> +      exit(1);
> +    }
> +  }
>    if (!what_to_do)
>    {
>      size_t pnlen= strlen(my_progname);
> @@ -463,8 +515,41 @@ static int process_databases(char **db_names)
>  } /* process_databases */
>  
>  
> +/* returns: -1 for error, 1 for view, 0 for table */
> +static int is_view(const char *table, uint length)
> +{
> +  char *query, *ptr;
> +  MYSQL_RES *res;
> +  MYSQL_FIELD *field;
> +  int view;
> +  DBUG_ENTER("is_view");
> +
> +  if (!(query =(char *) my_malloc((sizeof(char)*(length+110)), MYF(MY_WME))))
> +    DBUG_RETURN(-1);
> +  ptr= strmov(query, "SHOW CREATE TABLE `");
> +  ptr= strxmov(ptr, table, NullS);
> +  ptr= strxmov(ptr, "`", NullS);

Better use SHOW FULL TABLES WHERE TABLES_IN_MYSQL='xxxx';
That's the statement one should normally use to know whether xxxx is a
view or a table.  SHOW FULL TABLES only does just that, it doesn't even
parse the frm file (and doesn't open the table in the engine, of course).

> +  if (mysql_query(sock, query))
> +  {
> +    fprintf(stderr, "Failed to %s\n", query);
> +    fprintf(stderr, "Error: %s\n", mysql_error(sock));
> +    my_free(query);
> +    DBUG_RETURN(-1);
> +  }
> +  res= mysql_store_result(sock);
> +  field= mysql_fetch_field(res);
> +  view= (strcmp(field->name,"View")==0) ? 1 : 0;
> +  mysql_free_result(res);
> +  my_free(query);
> +
> +  DBUG_RETURN(view);
> +}
> +
>  static int process_selected_tables(char *db, char **table_names, int tables)
>  {
> +  int view;
> +  char *table;
> +  uint table_len;
>    DBUG_ENTER("process_selected_tables");
>  
>    if (use_db(db))
> @@ -494,12 +579,24 @@ static int process_selected_tables(char *db, char **table_names, int tables)
>        *end++= ',';
>      }
>      *--end = 0;
> -    handle_request_for_tables(table_names_comma_sep + 1, (uint) (tot_length - 1));
> +    table= table_names_comma_sep + 1;
> +    table_len= tot_length - 1;
> +    view= is_view(table, table_len);
> +    if (view < 0)
> +      DBUG_RETURN(1);
> +    handle_request_for_tables(table, table_len, (view==1));

This looks wrong. table_names_comma_sep is a comma separated list of
names, like

   "t1, t2, v1, v2, t3, v3, etc"

but you check the first element in the list and assume that they all are
of the same type.

Let's just say that in --all-in-one mode we don't support mixing objects
of different types.  Then it becomes:

 if (opt_fix_tables && opt_upgrade_views)
 { fprintf(strerr, "not supported..."); return 1; }

   handle_request_for_tables(table_names_comma_sep + 1, (uint) (tot_length - 1),
                             opt_upgrade_views);

alternatively, of course, you can walk the list, create two strings - one
for table and one for views - that is, do the same as in
process_all_tables_in_db()

>      my_free(table_names_comma_sep);
>    }
>    else
>      for (; tables > 0; tables--, table_names++)
> -      handle_request_for_tables(*table_names, fixed_name_length(*table_names));
> +    {
> +      table= *table_names;
> +      table_len= fixed_name_length(*table_names);
> +      view= is_view(table, table_len);
> +      if (view < 0)
> +        continue;
> +      handle_request_for_tables(table, table_len, (view==1));
> +    }
>    DBUG_RETURN(0);
>  } /* process_selected_tables */
>  
> @@ -748,10 +887,13 @@ static int handle_request_for_tables(char *tables, uint length)
>      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) ?
> +         "REPAIR" : "REPAIR NO_WRITE_TO_BINLOG";
>      if (opt_quick)              end = strmov(end, " QUICK");
>      if (opt_extended)           end = strmov(end, " EXTENDED");
>      if (opt_frm)                end = strmov(end, " USE_FRM");
> +    if (opt_upgrade_views==UPGRADE_VIEWS_FROM_MYSQL && view)
> +                                end = strmov(end, " FROM MYSQL");

I'd protect other opt's with !view:

  if (!view)
  {
    if (opt_quick) ....
    ...
  } else {
    if (opt_upgrade_views == UPGRADE_VIEWS_FROM_MYSQL)
      end = strmov(end, " FROM MYSQL");
  }

>      break;
>    case DO_ANALYZE:
>      op= (opt_write_binlog) ? "ANALYZE" : "ANALYZE NO_WRITE_TO_BINLOG";
> @@ -785,7 +928,8 @@ static int handle_request_for_tables(char *tables, uint length)
>    }
>    if (mysql_real_query(sock, query, query_length))
>    {
> -    sprintf(message, "when executing '%s TABLE ... %s'", op, options);
> +    sprintf(message, "when executing '%s%s... %s'", op, tab_view, options);
> +    /* sprintf(message, "when executing '%s'", query); */

forgotten debug comment?

>      DBerror(sock, message);
>      my_free(query);
>      DBUG_RETURN(1);
> diff --git a/client/mysql_upgrade.c b/client/mysql_upgrade.c
> index 5fb3b13..97e2ad92 100644
> --- a/client/mysql_upgrade.c
> +++ b/client/mysql_upgrade.c
> @@ -752,12 +755,67 @@ static int run_mysqlcheck_upgrade(void)
>                    opt_write_binlog ? "--write-binlog" : "--skip-write-binlog",
>                    "2>&1",
>                    NULL);
> +  if (retch || opt_systables_only)
> +    verbose("Phase %d/%d: Skipping 'mysql_fix_privilege_tables'... not needed", phase++, phases_total);

This is a bit strange. I'd expect something like

  if (....)
  { run some step }
  else
  { verbose("step skipped"); }

that's what you have below in run_mysqlcheck_views.
but printing the skip message here is weird and confusing.

better to break if() below (that calls run_sql_fix_privilege_tables)
and print the message there.

> +  return retch;
>  }
>  
> +#define EVENTS_STRUCT_LEN 7000
> +
> +static my_bool is_mysql()
> +{
> +  my_bool ret= TRUE;
> +  DYNAMIC_STRING ds_events_struct;
> +
> +  if (init_dynamic_string(&ds_events_struct, NULL,
> +                          EVENTS_STRUCT_LEN, EVENTS_STRUCT_LEN))
> +    die("Out of memory");
> +
> +  if (run_query("show create table mysql.event",
> +                &ds_events_struct, FALSE) ||
> +      strstr(ds_events_struct.str, "IGNORE_BAD_TABLE_OPTIONS") != NULL)
> +    ret= FALSE;
> +  else
> +    verbose("MySQL upgrade detected");
> +
> +  dynstr_free(&ds_events_struct);
> +  return(ret);
> +}
> +
> +static int run_mysqlcheck_views(void)
> +{
> +  const char *upgrade_views="--upgrade-views=YES";
> +  if (is_mysql())
> +  {
> +    upgrade_views="--upgrade-views=FROM_MYSQL";
> +    verbose("Phase %d/%d: Fixing views from mysql", phase++, phases_total);
> +  }
> +  else if (opt_systables_only)
> +  {
> +    verbose("Phase %d/%d: Fixing views - skipped - not required", phase++, phases_total);
> +    return 0;
> +  }
> +  else
> +    verbose("Phase %d/%d: Fixing views", phase++, phases_total);
> +
> +  print_conn_args("mysqlcheck");
> +  return run_tool(mysqlcheck_path,
> +                  NULL, /* Send output from mysqlcheck directly to screen */
> +                  "--no-defaults",
> +                  ds_args.str,
> +                  "--all-databases",
> +                  upgrade_views,
> +                  "--skip-fix-tables",
> +                  opt_verbose ? "--verbose": "",
> +                  opt_silent ? "--silent": "",
> +                  opt_write_binlog ? "--write-binlog" : "--skip-write-binlog",
> +                  "2>&1",
> +                  NULL);
> +}
>  
>  static int run_mysqlcheck_fixnames(void)
>  {
> -  verbose("Phase 1/3: Fixing table and database names");
> +  verbose("Phase %d/%d: Fixing table and database names", phase++, phases_total);
>    print_conn_args("mysqlcheck");
>    return run_tool(mysqlcheck_path,
>                    NULL, /* Send output from mysqlcheck directly to screen */
> diff --git a/mysql-test/r/mysql_upgrade_view.result b/mysql-test/r/mysql_upgrade_view.result
> new file mode 100644
> index 0000000..aa39319
> --- /dev/null
> +++ b/mysql-test/r/mysql_upgrade_view.result
> @@ -0,0 +1,205 @@
> +set sql_log_bin=0;
> +drop table if exists t1,v1,v2,v3,v4,v1badcheck;
> +drop view if exists t1,v1,v2,v3,v4,v1badcheck;
> +create table t1(a int);
> +create table kv(k varchar(30) NOT NULL PRIMARY KEY,v  varchar(50));
> +flush tables;
> +Phase 1/4: Fixing views
> +test.v1                                            OK
> +test.v1badcheck                                    OK
> +test.v2                                            OK
> +test.v3                                            OK
> +Phase 2/4: Fixing table and database names
> +Phase 3/4: Checking and upgrading tables
> +Processing databases
> +information_schema
> +mtr
> +mtr.global_suppressions                            OK
> +mtr.test_suppressions                              OK
> +mysql
> +mysql.columns_priv                                 OK
> +mysql.db                                           OK
> +mysql.event                                        OK
> +mysql.func                                         OK
> +mysql.help_category                                OK
> +mysql.help_keyword                                 OK
> +mysql.help_relation                                OK
> +mysql.help_topic                                   OK
> +mysql.host                                         OK
> +mysql.ndb_binlog_index                             OK
> +mysql.plugin                                       OK
> +mysql.proc                                         OK
> +mysql.procs_priv                                   OK
> +mysql.proxies_priv                                 OK
> +mysql.servers                                      OK
> +mysql.tables_priv                                  OK
> +mysql.time_zone                                    OK
> +mysql.time_zone_leap_second                        OK
> +mysql.time_zone_name                               OK
> +mysql.time_zone_transition                         OK
> +mysql.time_zone_transition_type                    OK
> +mysql.user                                         OK
> +performance_schema
> +test
> +test.kv                                            OK
> +test.t1                                            OK
> +Phase 4/4: Running 'mysql_fix_privilege_tables'...
> +OK
> +show create view v1;
> +View	Create View	character_set_client	collation_connection
> +v1	CREATE ALGORITHM=MERGE DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v1` AS select `t1`.`a` AS `a` from `t1`	utf8	utf8_general_ci
> +show create view v2;
> +View	Create View	character_set_client	collation_connection
> +v2	CREATE ALGORITHM=TEMPTABLE DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v2` AS select `t1`.`a` AS `a` from `t1`	utf8	utf8_general_ci
> +show create view v3;
> +View	Create View	character_set_client	collation_connection
> +v3	CREATE ALGORITHM=UNDEFINED DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v3` AS select `t1`.`a` AS `a` from `t1`	utf8	utf8_general_ci
> +set sql_log_bin=1;
> +REPAIR VIEW v1,v2;
> +Table	Op	Msg_type	Msg_text
> +test.v1	repair	status	OK
> +test.v2	repair	status	OK
> +REPAIR VIEW v1badcheck;
> +Table	Op	Msg_type	Msg_text
> +test.v1badcheck	repair	status	OK
> +REPAIR NO_WRITE_TO_BINLOG VIEW v3;
> +Table	Op	Msg_type	Msg_text
> +test.v3	repair	status	OK
> +set sql_log_bin=0;
> +show binlog events from <binlog_start>;
> +Log_name	Pos	Event_type	Server_id	End_log_pos	Info
> +master-bin.000001	#	Query	#	#	use `test`; REPAIR VIEW v1,v2
> +master-bin.000001	#	Query	#	#	use `test`; REPAIR VIEW v1badcheck

Hm. As far as I remember, Sanja said that it makes no sense to write
REPAIR VIEW to binlog, and that's why he didn't support
NO_WRITE_TO_BINLOG. And I replied that let's support NO_WRITE_TO_BINLOG
syntax for consistency, but not write REPAIR VIEW to binlog regardless,
if that makes no sense.

Do you think it *does* make sense to write REPAIR VIEW to binlog? I agree
that it's consistent with REPAIR TABLE and will get us less bug reports
about "REPAIR VIEW not in binlog".  But can it be useful?

> +LOAD DATA INFILE 'MYSQLD_DATADIR/test/v1.frm' REPLACE INTO TABLE kv FIELDS TERMINATED BY '=';
> +SELECT k,v from kv where k in ('md5','algorithm');

nice :)

> +k	v
> +algorithm	1
> +md5	5e6eaf216e7b016fcedfd4e1113517af
> +SELECT k from kv where k ='mariadb-version';
> +k
> +mariadb-version
> +truncate table kv;
> +LOAD DATA INFILE 'MYSQLD_DATADIR/test/v2.frm' REPLACE INTO TABLE kv FIELDS TERMINATED BY '=';
> +SELECT k,v from kv where k in ('md5','algorithm');
> +k	v
> +algorithm	2
> +md5	5e6eaf216e7b016fcedfd4e1113517af
> +SELECT k from kv where k ='mariadb-version';
> +k
> +mariadb-version
> +truncate table kv;
> +LOAD DATA INFILE 'MYSQLD_DATADIR/test/v3.frm' REPLACE INTO TABLE kv FIELDS TERMINATED BY '=';
> +SELECT k,v from kv where k in ('md5','algorithm');
> +k	v
> +algorithm	0
> +md5	5e6eaf216e7b016fcedfd4e1113517af
> +SELECT k from kv where k ='mariadb-version';
> +k
> +mariadb-version
> +truncate table kv;
> +LOAD DATA INFILE 'MYSQLD_DATADIR/test/v1badcheck.frm' REPLACE INTO TABLE kv FIELDS TERMINATED BY '=';
> +SELECT k,v from kv where k in ('md5','algorithm');
> +k	v
> +algorithm	1
> +md5	5e6eaf216e7b016fcedfd4e1113517af
> +SELECT k from kv where k ='mariadb-version';
> +k
> +mariadb-version
> +truncate table kv;
> +drop view if exists v1,v2,v3,v1badcheck;
> +flush tables;
> +create algorithm=temptable view v4 as select a from t1;
> +show create view v1;
> +View	Create View	character_set_client	collation_connection
> +v1	CREATE ALGORITHM=MERGE DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v1` AS select `t1`.`a` AS `a` from `t1`	utf8	utf8_general_ci
> +show create view v2;
> +View	Create View	character_set_client	collation_connection
> +v2	CREATE ALGORITHM=TEMPTABLE DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v2` AS select `t1`.`a` AS `a` from `t1`	utf8	utf8_general_ci
> +show create view v3;
> +View	Create View	character_set_client	collation_connection
> +v3	CREATE ALGORITHM=UNDEFINED DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v3` AS select `t1`.`a` AS `a` from `t1`	utf8	utf8_general_ci
> +show create view v4;
> +View	Create View	character_set_client	collation_connection
> +v4	CREATE ALGORITHM=TEMPTABLE DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v4` AS select `t1`.`a` AS `a` from `t1`	latin1	latin1_swedish_ci
> +MySQL upgrade detected
> +Phase 1/4: Fixing views from mysql
> +test.v1                                            OK
> +test.v2                                            OK
> +test.v3                                            OK
> +test.v4                                            OK
> +Phase 2/4: Fixing table and database names
> +Phase 3/4: Checking and upgrading tables
> +Processing databases
> +information_schema
> +mtr
> +mtr.global_suppressions                            OK
> +mtr.test_suppressions                              OK
> +mysql
> +mysql.columns_priv                                 OK
> +mysql.db                                           OK
> +mysql.ev_bk                                        OK
> +mysql.event                                        OK
> +mysql.func                                         OK
> +mysql.help_category                                OK
> +mysql.help_keyword                                 OK
> +mysql.help_relation                                OK
> +mysql.help_topic                                   OK
> +mysql.host                                         OK
> +mysql.ndb_binlog_index                             OK
> +mysql.plugin                                       OK
> +mysql.proc                                         OK
> +mysql.procs_priv                                   OK
> +mysql.proxies_priv                                 OK
> +mysql.servers                                      OK
> +mysql.tables_priv                                  OK
> +mysql.time_zone                                    OK
> +mysql.time_zone_leap_second                        OK
> +mysql.time_zone_name                               OK
> +mysql.time_zone_transition                         OK
> +mysql.time_zone_transition_type                    OK
> +mysql.user                                         OK
> +performance_schema
> +test
> +test.kv                                            OK
> +test.t1                                            OK
> +Phase 4/4: Running 'mysql_fix_privilege_tables'...
> +OK
> +show create view v1;
> +View	Create View	character_set_client	collation_connection
> +v1	CREATE ALGORITHM=TEMPTABLE DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v1` AS select `t1`.`a` AS `a` from `t1`	utf8	utf8_general_ci
> +show create view v2;
> +View	Create View	character_set_client	collation_connection
> +v2	CREATE ALGORITHM=MERGE DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v2` AS select `t1`.`a` AS `a` from `t1`	utf8	utf8_general_ci
> +show create view v3;
> +View	Create View	character_set_client	collation_connection
> +v3	CREATE ALGORITHM=UNDEFINED DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v3` AS select `t1`.`a` AS `a` from `t1`	utf8	utf8_general_ci
> +show create view v4;
> +View	Create View	character_set_client	collation_connection
> +v4	CREATE ALGORITHM=TEMPTABLE DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v4` AS select `t1`.`a` AS `a` from `t1`	latin1	latin1_swedish_ci
> +LOAD DATA INFILE 'MYSQLD_DATADIR/test/v1.frm' REPLACE INTO TABLE kv FIELDS TERMINATED BY '=';
> +SELECT k,v from kv where k in ('md5','algorithm');
> +k	v
> +algorithm	2
> +md5	5e6eaf216e7b016fcedfd4e1113517af
> +SELECT k from kv where k ='mariadb-version';
> +k
> +mariadb-version
> +truncate table kv;
> +drop view if exists v1,v2,v3;
> +flush tables;
> +test.v1                                            OK
> +test.v2                                            OK
> +test.v3                                            OK
> +test.v4                                            OK
> +show binlog events from <binlog_start>;
> +Log_name	Pos	Event_type	Server_id	End_log_pos	Info
> +master-bin.000001	#	Query	#	#	use `test`; REPAIR VIEW v1,v2
> +master-bin.000001	#	Query	#	#	use `test`; REPAIR VIEW v1badcheck
> +master-bin.000001	#	Query	#	#	use `test`; REPAIR VIEW `v1`  FROM MYSQL
> +master-bin.000001	#	Query	#	#	use `test`; REPAIR VIEW `v2`  FROM MYSQL
> +master-bin.000001	#	Query	#	#	use `test`; REPAIR VIEW `v3`  FROM MYSQL
> +master-bin.000001	#	Query	#	#	use `test`; REPAIR VIEW `v4`  FROM MYSQL
> +flush tables;
> +drop table if exists kv;
> +drop view v1,v2,v3,v4;
> +drop table t1;
> diff --git a/sql/sql_admin.cc b/sql/sql_admin.cc
> index 92aa414..65292a5 100644
> --- a/sql/sql_admin.cc
> +++ b/sql/sql_admin.cc
> @@ -380,7 +381,18 @@ static bool mysql_admin_table(THD* thd, TABLE_LIST* tables,
>        lex->query_tables_own_last= 0;
>  
>        if (view_operator_func == NULL)
> +      {
>          table->required_type=FRMTYPE_TABLE;
> +        DBUG_ASSERT(!lex->only_view);
> +      }
> +      else if (lex->only_view)
> +      {
> +        table->required_type= FRMTYPE_VIEW;
> +      }
> +      else if (!lex->only_view && lex->sql_command == SQLCOM_REPAIR)
> +      {
> +        table->required_type= FRMTYPE_TABLE;
> +      }

That's a bit inconsistent, CHECK TABLE historically works for views, but
REPAIR TABLE doesn't, one must use REPAIR VIEW, while CHECK VIEW is
optional. But I don't like if REPAIR TABLE would work for views. May be
issue a deprecation warning for CHECK TABLE? Or just leave it as is?  may
be nobody cares...

>  
>        if (lex->sql_command == SQLCOM_CHECK ||
>            lex->sql_command == SQLCOM_REPAIR ||
> diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy
> index 257001c..dfbc6c4 100644
> --- a/sql/sql_yacc.yy
> +++ b/sql/sql_yacc.yy
> @@ -13786,7 +13807,18 @@ lock:
>  
>  table_or_tables:
>            TABLE_SYM
> +         { Lex->only_view= FALSE; }
> +        | TABLES
> +         { Lex->only_view= FALSE; }
> +        ;
> +
> +table_or_view:
> +          TABLE_SYM
> +         { Lex->only_view= FALSE; }
>          | TABLES
> +         { Lex->only_view= FALSE; }
> +        | VIEW_SYM
> +         { Lex->only_view= TRUE; }

reuse table_or_tables here

>          ;
>  
>  table_lock_list:
> diff --git a/sql/sql_view.cc b/sql/sql_view.cc
> index 09784f7..24f01ae 100644
> --- a/sql/sql_view.cc
> +++ b/sql/sql_view.cc
> @@ -791,6 +811,86 @@ 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 (!wrong_checksum && view->mariadb_version)
> +    DBUG_RETURN(HA_ADMIN_OK);
> +
> +  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);
> +  }

I wonder if that's necessary. A view is open at this point, you've just
calculated its checksum and tested whether it has mariadb_version field
(in view_repair()).

> +
> +  if (swap_alg && view->algorithm != VIEW_ALGORITHM_UNDEFINED)
> +  {
> +    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;
> +  }
> +  else
> +    swap_alg= 0;
> +  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': versioned to %llu%s%s",
> +                        view->db, view->table_name, view->mariadb_version,
> +                        (wrong_checksum ? ", and checksum corrected" : ""),
> +                        (swap_alg ?
> +                          ((view->algorithm == VIEW_ALGORITHM_MERGE) ?
> +                            ", and algorithm swapped to 'MERGE'"
> +                           : ", and algorithm swapped to 'TEMPTABLE'")

I don't like how it looks "and algorithm swapped to 'MERGE'".
I think "changed to" or "corrected to be" or "restored to be" looks better.

> +                         : ""));
> +
> +
> +  DBUG_RETURN(HA_ADMIN_OK);
> +}
> +
> +
>  /*
>    Register VIEW (write .frm & process .frm's history backups)
>  
> @@ -1941,6 +2033,63 @@ int view_checksum(THD *thd, TABLE_LIST *view)
>            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_TABLE_NEEDS_UPGRADE,
> +                            ER(ER_TABLE_NEEDS_UPGRADE),
> +                            view->db,
> +                            view->table_name);

Is that necessary? I'd expect the caller (mysql_admin_table() to do that,
when it gets HA_ADMIN_NEEDS_UPGRADE. There is nothing view specific in
this warning message that you're printing.

> +    DBUG_RETURN(HA_ADMIN_NEEDS_UPGRADE);
> +  }
> +  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);
> +  bool wrong_checksum= view_checksum(thd, view);

view_checksum() != HA_ADMIN_OK

> +  int ret;
> +  if (wrong_checksum || swap_alg || (!view->mariadb_version))
> +  {
> +    ret= mariadb_fix_view(thd, view, wrong_checksum, swap_alg);
> +    DBUG_RETURN(ret);
> +  }
> +  DBUG_RETURN(HA_ADMIN_OK);
> +}
> +
>  /*
>    rename view

Regards,
Sergei


Follow ups

References