maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Daniel!

On Apr 12, Daniel Black wrote:
> 
> For review:
> 
> Hi All,
> 
> list of all changes: https://github.com/openquery/mariadb-server/commits/MDEV-6916-maria-5.5-check_view-r4408
> 
> side by side comparision of Sanja/my work against 5.5 head:
> 
> https://github.com/MariaDB/server/compare/5.5...openquery:MDEV-6916-maria-5.5-check_view-r4408?expand=1

Great, thanks a lot!

Here, few comments are below:
 
> >> +static int run_mysqlcheck_views(void)
> >> +{
> >> +  if (!opt_mysql_upgrade)
> >> +    return 0;
> >> +  verbose("Phase 0: Fixing views");
> > Again, was in my previous review:
> > ===
> > 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.

> > 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.
> 
> Not done yet.
> 
> --skip-tables or --{update|repair}-views-only perhaps?
> 
> 
> > 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.
> 
> and the 'from_mysql' means don't check tables? This doesn't sound
> intuitive. More correctly its a --repair-views.

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.

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.

Regards,
Sergei