maria-developers team mailing list archive

[Daniel Black <daniel.black@xxxxxxxxxxxxxxxx>]

----- 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.