----- Original Message -----
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.
Yes please.
Or you can do that, whatever you prefer. Either way, I'd still like to see my questions answered first.
k
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.
You're quite right.
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?
I was unsure if you wanted this additional output.
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.
Where it is called from is if ((!opt_systables_only && (run_mysqlcheck_views() || run_mysqlcheck_fixnames() || run_mysqlcheck_upgrade())) || run_sql_fix_privilege_tables()) So separating it was going to be invasive, however it would be easier to follow.
+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?
repair view corrects the checksum if broken, adds the mariadb version if it isn't there. The only downside I can think of is in galera TOI mode it will reduce parallel operation for a very quick DDL.
+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 :)
I got a laugh out of it too.
--- 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,
This was Sanja's bit (c8dbef22a)
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?
FWIW - I agree.
Or just leave it as is? may be nobody cares...
It adds supporting and documenting the syntax which is a bit of a maintenance burden and looks odd and adhoc for no real benefit.
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.
recreating the frm?
A view is open at this point, you've just calculated its checksum and tested whether it has mariadb_version field (in view_repair()).
But some of these aren't necessary true(?). I was assuming this was avoiding transient errors. Sanja?
+ + 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.
fair call
+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.
Quite right. its probably something i didn't revert when changing the error messages back to the standard ones. -- -- Daniel Black, Engineer @ Open Query (http://openquery.com.au) Remote expertise & maintenance for MySQL/MariaDB server environments.