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-chec...
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....
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/25872e2802a4ae32d4f18132d...
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