maria-developers team mailing list archive

[Daniel Black <daniel.black@xxxxxxxxxxxxx>]

>> > No "Phase 0" please, renumber all phases to start from 1.
>> 
>> Took opportunity to make adding/changing phases to be lest conflicts in merge
>> 
>> https://github.com/openquery/mariadb-server/commit/25872e2802a4ae32d4f18132d569599295c963cd
>
>I don't know whether it's a good idea.
>
> It means that, for example "Running 'mysql_fix_privilege_tables'..."
> won't always be Phase 3/3, but depending on command-line options it
> might be a Phase 2 or 1 (well, may be mysql_fix_privilege_tables phase
> cannot - but there certainly are phases that are run conditionally).
>
> I personally don't know how bad it is, perhaps it's fine to renumber
> phases dynamically when some phases are skipped. So I have no opinion.
> I'm just saying that phases might be renumbered dynamically after your
> patch.

Well consistent phase numbering to activity is a good goal. So is knowing why phases that just don't occur.
So is having a total phases that is actually reached.

So produce output for every phase, even if it just says we're skipping it:

https://github.com/MariaDB/server/commit/808608cb3f4e10ba81679c94b2a299a32cf5e448



> I believe I meant these three variants:
> 
>   --upgrade-views = no           don't change view frm's
>   --upgrade-views = yes          upgrade view frms adding mariadb_version
>                                  field and if is_mysql() returned true
>                                  also toggle the view algorithm
>   --upgrade-views = from_mysql   always toggle the view algorithm, even
>                                  if is_mysql() returned false.

Right is_mysql is in mysql_upgrade and --upgrade-view is mysqlcheck.

Always exclusing if there is a mariadb-version in in the frm I assume.

> The last case is useful if a user has migrated from MySQL some time ago,
> so it's a mariadb-to-mariadb upgrade but views stay wrong from older
> mysql-to-mariadb upgrade.

Added:

https://github.com/MariaDB/server/commit/97e0aeaf72cde117b8d9551f68d407fa13aafbe1

I'm almost thinking mysqlcheck should default to --upgrade-views=yes? At least it 
will do a checksum but otherwise won't rewrite a view unless there is missing mariadb-version


note mysql_upgrade:
is_mysql detected run with --upgrade_views=from_mysql
--upgrade-system-tables - upgrade_views not run
otherwise --upgrade_views=yes to get
old views upgraded (and checksum checked).

https://github.com/MariaDB/server/commit/4987080ddb42abf1a9111bd6a79e7bed746b66ff


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

Has been renamed to --upgrade-views and it doesn't skip all tables. This is done with --skip-fix-tables

https://github.com/MariaDB/server/commit/fc277cd4ba6c506a6f6c71e04c462b24a3fcfb26


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

done:

https://github.com/MariaDB/server/commit/c584058f8f227a2004f9fdf6fd0897e11fd53eda


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

done:

https://github.com/MariaDB/server/commit/c584058f8f227a2004f9fdf6fd0897e11fd53eda


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

it was unnecessary - removed

https://github.com/MariaDB/server/commit/7229b19c67d12ff579c64f1184e951015aa2f9c9


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

corrected:

https://github.com/MariaDB/server/commit/29721d7d5f85fe323d70f1856aa1e5294e14074a


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

Already covered earlier in the function


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

fixed. like below.
(https://github.com/MariaDB/server/commit/8a827d530a67d40332668d31683a45f583974935)

>    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() :)

I was too lazy. sorry.

> Second, REPAIR VIEW viewname - without FROM MYSQL - should work too, it
> should add mariadb_version field without changing the algorithm.

ok. Done - 

https://github.com/MariaDB/server/commit/76c18f7e76dff6f5c5f8986e0480074a3100fdbe

> > === 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?

corrected:

https://github.com/MariaDB/server/commit/28b173134ee1634a2489d3cef6faf2f3ea1f6ab4



-- 
-- 
Daniel Black, Engineer @ Open Query (http://openquery.com.au)
Remote expertise & maintenance for MySQL/MariaDB server environments.