Hi, Sanja! On Feb 09, Oleksandr Byelkin wrote:
On 09.02.15 15:19, Sergei Golubchik wrote:
On Feb 09, sanja@askmonty.org wrote:
At file:///home/bell/maria/bzr/work-maria-5.5-MDEV-6916-check_view/
=== 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. could you show me example of wat you mean by my_getop() doing its job?
Any option that is not mentioned in this switch. For example, --all-databases, --all-in-1, --auto-repair, --character-sets-dir, --compress, and so on, in fact almost all options are not set explicitly in the switch.
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 So should I rename it?
Yes, please (but see below). Note that here we're talking about mysqlcheck. As for the option in mysql_upgrade - you've already agreed to remove it.
+ }
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.
It just has no that syntax because writing to binlog looks like nonsens.
Okay. Then I'd at least allow the syntax, for consistency. It won't do anything if you aren't going to write REPAIR VIEW to binlog anyway.
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. -mysql-upgrade (i.e. REPAIR TABLE ... FROM MYSQL) with table is error now because it have nothing to do. What it should do for tables?
I can add 2 lists and processing for CHECK/RAPAIR VIEW/TABLE but usage will be limited IMHO.
yes, I meant two lists in my review. But on the other hand... When mysqlcheck is invoked from mysql_upgrade, you wouldn't want it to repair all tables. Okay, let's keep your implementation and only repair views when --fix-view-algorithm is set. In that case, the option should better be --upgrade-views, and defined as --upgrade-views = { no, yes, from_mysql } note that "no" and "yes" must be in that exactly order, first and second.
+ tables, options); table_name= tables; } else { char *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 ? I'll add.
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 include/show_binlog_events.inc
+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/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.
Did you miss this comment or you will do the change?
+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.
Did you miss this comment or you will do the change?
+ { + 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?
Did you miss this comment or you will do the change?
+ { + 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?
Did you miss this comment or you will do the change?
retry_share:
mysql_mutex_lock(&LOCK_open);
if (!(share= get_table_share_with_discover(thd, table_list, key, - key_length, OPEN_VIEW, + key_length,
Regards, Sergei