MDEV-39343 mysql_upgrade: also require Grant_tables_intact before skipping

mysql_upgrade now skips only if both the recorded version matches and
Grant_tables_intact is ON, so restoring an older-version dump's grant
tables onto an already-upgraded server is detected and re-upgraded.

client/mysql_upgrade.c

@@ -793,25 +793,52 @@ static int faulty_server_versions(const char *version)
   return 0;
 }
 
+/* Read Grant_tables_intact ("ON" or "OFF") using "mysql" */
+static my_bool grant_tables_are_intact(void)
+{
+  DYNAMIC_STRING ds_status;
+  char value[NAME_CHAR_LEN + 1];
+  my_bool intact= FALSE;
+
+  if (init_dynamic_string(&ds_status, NULL, NAME_CHAR_LEN, NAME_CHAR_LEN))
+    die("Out of memory");
+
+  if (run_query("show status like 'Grant_tables_intact'",
+                &ds_status, FALSE) ||
+      extract_variable_from_show(&ds_status, value))
+  {
+    if (opt_verbose)
+      verbose("Could not read Grant_tables_intact. Assuming the grant "
+              "tables need checking.");
+  }
+  else
+    intact= (strcmp(value, "ON") == 0);
+
+  dynstr_free(&ds_status);
+  return intact;
+}
+
+
 /*
   Read the content of mysql_upgrade_info file and
   compare the version number form file against
   version number which mysql_upgrade was compiled for
 
-  NOTE
-  This is an optimization to avoid running mysql_upgrade
-  when it's already been performed for the particular
-  version of MariaDB.
+  This is also used, with silent=1, just to verify that
+  finish_mysql_upgrade_info_file() successfully wrote the file - so it must
+  depend only on the file's content, never on live server state, and must
+  not print the "already upgraded" success message itself (that belongs to
+  upgrade_already_done(), once it has also checked Grant_tables_intact).
 
   In case the MariaDBL server can't return the upgrade info
   file it's always better to report that the upgrade hasn't
   been performed.
 
-  @return 0   Upgrade has already been run on this version
-  @return > 0 Upgrade has to be run
+  @return 0   file's recorded version matches the current version
+  @return > 0 upgrade has to be run
 */
 
-static int upgrade_already_done(int silent)
+static int version_file_matches(int silent)
 {
   const char *version = MYSQL_SERVER_VERSION;
   const char *s;
@@ -867,6 +894,64 @@ static int upgrade_already_done(int silent)
         "lower version is not supported!",
         upgrade_from_version, version);
   }
+  return 0;
+}
+
+
+static void flush_privileges_for_check(void)
+{
+  run_query("FLUSH PRIVILEGES", NULL, TRUE);
+}
+
+/*
+  Decide whether mysql_upgrade has to be run: the recorded version in
+  mysql_upgrade_info must match the version mysql_upgrade was compiled for,
+  AND the grant tables' structure must match what this server expects
+  (Grant_tables_intact) - e.g. a dump or restore may have replaced them
+  with an older version's, independently of what the version file says.
+
+  NOTE
+  This is an optimization to avoid running mysql_upgrade
+  when it's already been performed for the particular
+  version of MariaDB.
+
+  @return 0   Upgrade has already been run on this version
+  @return > 0 Upgrade has to be run
+*/
+
+static int upgrade_already_done(int silent)
+{
+  const char *version = MYSQL_SERVER_VERSION;
+  const char *s;
+  int file_result= version_file_matches(silent);
+
+  if (file_result)
+    return file_result;
+
+  /*
+    The recorded version matches, so the only thing that could still require
+    an upgrade is the grant tables' live structure - e.g. a dump restore may
+    have just replaced them with an older version's, without a server
+    restart or an explicit FLUSH PRIVILEGES to refresh Grant_tables_intact.
+    Only now, when that's the deciding factor, force a FLUSH PRIVILEGES so
+    the check below reflects the grant tables' current on-disk structure
+    rather than a value cached since the last startup or flush.
+  */
+  flush_privileges_for_check();
+
+  if (!grant_tables_are_intact())
+  {
+    if (opt_verbose)
+      verbose("The recorded version (%s) matches, but the grant tables' "
+              "structure doesn't match what this server expects - a dump "
+              "or restore likely replaced them with an older version's. "
+              "Check required!", upgrade_from_version);
+    return 4;
+  }
+
+  s= strchr(version, '.');
+  s= strchr(s + 1, '.');
+
   if (!silent)
   {
     if (strcmp(upgrade_from_version, version))
@@ -901,7 +986,7 @@ static void finish_mysql_upgrade_info_file(void)
     Check if the upgrade_info_file was properly created/updated
     It's not a fatal error -> just print a message if it fails
   */
-  if (upgrade_already_done(1))
+  if (version_file_matches(1))
     fprintf(stderr,
             "Could not write to the upgrade info file '%s' in "
             "the MariaDB Servers datadir, errno: %d\n",
@@ -1513,8 +1598,8 @@ int main(int argc, char **argv)
     printf("The --upgrade-system-tables option was used, user tables won't be touched.\n");
 
   /*
-    Read the mysql_upgrade_info file to check if mysql_upgrade
-    already has been run for this installation of MariaDB
+    Read the mysql_upgrade_info file (and Grant_tables_intact) to check if
+    mysql_upgrade already has been run for this installation of MariaDB
   */
   if (!opt_force && !upgrade_already_done(0))
     goto end;                                   /* Upgrade already done */

mysql-test/include/load_dump_and_upgrade.inc

@@ -11,8 +11,7 @@
 # On 10.4 the local root was additionally given an empty password,
 # to avoid using unix_socket.
 
-# Older dumps have genuinely narrower grant tables than this server expects
-# (e.g. 10.4's roles_mapping/proxies_priv columns are shorter than today's),
+# Older dumps have genuinely narrower grant tables than this server expects,
 # and dropping Delete_history_priv further below also shrinks mysql.db by
 # one column - FLUSH PRIVILEGES during the upgrade will correctly log both
 # message shapes for each, which Table_check_intact/Grant_tables_intact are

mysql-test/main/grant_4332.test

@@ -10,8 +10,6 @@
 set global sql_mode="";
 set local sql_mode="";
 
-# The narrowed User columns below make Table_check_intact log a mismatch
-# on the FLUSH PRIVILEGES just after - expected, not a new bug.
 call mtr.add_suppression("Incorrect definition of table");
 
 alter table mysql.user modify User char(16) binary not null default '';

mysql-test/main/grant_tables_intact.result

@@ -32,13 +32,6 @@ SHOW STATUS LIKE 'Grant_tables_intact';
 Variable_name	Value
 Grant_tables_intact	ON
 #
-# mysql.roles_mapping, mysql.proxies_priv, mysql.tables_priv,
-# mysql.columns_priv, mysql.procs_priv and mysql.global_priv all
-# have min_columns pinned to their current column count (no
-# tolerance at all), so dropping a column would instead trip the
-# pre-existing ER_CANNOT_LOAD_FROM_TABLE_V2 check in
-# Grant_table_base::set_table() before Grant_tables_intact's own
-# logic ever runs (see grant_repair.test for that mechanism).
 # A type change on Host, instead, leaves the column count and key
 # structure untouched, so the table still loads and our check
 # actually gets exercised.

mysql-test/main/grant_tables_intact.test

@@ -32,13 +32,6 @@ FLUSH PRIVILEGES;
 SHOW STATUS LIKE 'Grant_tables_intact';
 
 --echo #
---echo # mysql.roles_mapping, mysql.proxies_priv, mysql.tables_priv,
---echo # mysql.columns_priv, mysql.procs_priv and mysql.global_priv all
---echo # have min_columns pinned to their current column count (no
---echo # tolerance at all), so dropping a column would instead trip the
---echo # pre-existing ER_CANNOT_LOAD_FROM_TABLE_V2 check in
---echo # Grant_table_base::set_table() before Grant_tables_intact's own
---echo # logic ever runs (see grant_repair.test for that mechanism).
 --echo # A type change on Host, instead, leaves the column count and key
 --echo # structure untouched, so the table still loads and our check
 --echo # actually gets exercised.

mysql-test/main/mysql_upgrade_grant_tables_intact.result

@@ -0,0 +1,68 @@
+#
+# mysql_upgrade also requires Grant_tables_intact, not just
+# a matching recorded version, before it decides no upgrade is needed.
+#
+call mtr.add_suppression("is wrong. Expected");
+call mtr.add_suppression("Incorrect definition of table");
+# Establish a clean baseline: mysql_upgrade_info records the current
+# version, and the grant tables are intact.
+SHOW STATUS LIKE 'Grant_tables_intact';
+Variable_name	Value
+Grant_tables_intact	ON
+# Already upgraded: --check-if-upgrade-is-needed must say so.
+This installation of MariaDB is already upgraded to X.Y.Z-MariaDB.
+There is no need to run mysql_upgrade again.
+#
+# Simulate restoring mysql.db from an older version's dump onto this
+# already-upgraded server: the recorded version is untouched, but the
+# live table is now missing a column this server expects.
+#
+ALTER TABLE mysql.db DROP COLUMN Delete_history_priv;
+FLUSH PRIVILEGES;
+SHOW STATUS LIKE 'Grant_tables_intact';
+Variable_name	Value
+Grant_tables_intact	OFF
+# --check-if-upgrade-is-needed must now report an upgrade is needed,
+# even though the recorded version still matches.
+# A plain run (no --force) must actually perform the upgrade - and so
+# repair the table - instead of skipping it just because the recorded
+# version matches.
+SHOW STATUS LIKE 'Grant_tables_intact';
+Variable_name	Value
+Grant_tables_intact	ON
+SELECT COUNT(*) AS `Delete_history_priv restored` FROM information_schema.columns WHERE table_schema='mysql' AND table_name='db' AND column_name='Delete_history_priv';
+Delete_history_priv restored
+1
+# Already upgraded again, as normal.
+This installation of MariaDB is already upgraded to X.Y.Z-MariaDB.
+There is no need to run mysql_upgrade again.
+#
+# MDEV-39343: Restoring from a mysqldump from older version makes mysql_upgrade version test fail
+#
+# Back up the current (upgraded) mysql schema, to restore afterward.
+#
+# Restore a MariaDB 10.4 mysql schema directly over the live tables -
+# exactly like restoring a mariadb-dump backup taken on an older
+# server. mysql_upgrade_info, on the filesystem, is untouched by this
+# and still records the CURRENT version.
+#
+FLUSH PRIVILEGES;
+SHOW STATUS LIKE 'Grant_tables_intact';
+Variable_name	Value
+Grant_tables_intact	OFF
+#
+# Before this fix, mysql_upgrade trusted the recorded version alone
+# and would have reported no upgrade needed here - leaving the
+# older, just-restored schema un-upgraded. It must now correctly
+# detect that an upgrade is needed.
+#
+# A plain run (no --force) must therefore actually perform the
+# upgrade, instead of skipping it.
+SHOW STATUS LIKE 'Grant_tables_intact';
+Variable_name	Value
+Grant_tables_intact	ON
+# Already upgraded now.
+This installation of MariaDB is already upgraded to X.Y.Z-MariaDB.
+There is no need to run mysql_upgrade again.
+# Restore the original (current-version) mysql schema backup.
+# End of 10.11 tests

mysql-test/main/mysql_upgrade_grant_tables_intact.test

@@ -0,0 +1,100 @@
+--source include/mysql_upgrade_preparation.inc
+--source include/have_innodb.inc
+
+--echo #
+--echo # mysql_upgrade also requires Grant_tables_intact, not just
+--echo # a matching recorded version, before it decides no upgrade is needed.
+--echo #
+
+call mtr.add_suppression("is wrong. Expected");
+call mtr.add_suppression("Incorrect definition of table");
+
+--echo # Establish a clean baseline: mysql_upgrade_info records the current
+--echo # version, and the grant tables are intact.
+--disable_result_log
+--exec $MYSQL_UPGRADE --force 2>&1
+--enable_result_log
+SHOW STATUS LIKE 'Grant_tables_intact';
+
+--echo # Already upgraded: --check-if-upgrade-is-needed must say so.
+--replace_result $MYSQL_SERVER_VERSION X.Y.Z-MariaDB
+--error 1
+--exec $MYSQL_UPGRADE --check-if-upgrade-is-needed 2>&1
+
+--echo #
+--echo # Simulate restoring mysql.db from an older version's dump onto this
+--echo # already-upgraded server: the recorded version is untouched, but the
+--echo # live table is now missing a column this server expects.
+--echo #
+ALTER TABLE mysql.db DROP COLUMN Delete_history_priv;
+FLUSH PRIVILEGES;
+SHOW STATUS LIKE 'Grant_tables_intact';
+
+--echo # --check-if-upgrade-is-needed must now report an upgrade is needed,
+--echo # even though the recorded version still matches.
+--exec $MYSQL_UPGRADE --check-if-upgrade-is-needed 2>&1
+
+--echo # A plain run (no --force) must actually perform the upgrade - and so
+--echo # repair the table - instead of skipping it just because the recorded
+--echo # version matches.
+--disable_result_log
+--exec $MYSQL_UPGRADE 2>&1
+--enable_result_log
+
+SHOW STATUS LIKE 'Grant_tables_intact';
+SELECT COUNT(*) AS `Delete_history_priv restored` FROM information_schema.columns WHERE table_schema='mysql' AND table_name='db' AND column_name='Delete_history_priv';
+
+--echo # Already upgraded again, as normal.
+--replace_result $MYSQL_SERVER_VERSION X.Y.Z-MariaDB
+--error 1
+--exec $MYSQL_UPGRADE --check-if-upgrade-is-needed 2>&1
+
+--echo #
+--echo # MDEV-39343: Restoring from a mysqldump from older version makes mysql_upgrade version test fail
+--echo #
+
+--echo # Back up the current (upgraded) mysql schema, to restore afterward.
+--exec $MYSQL_DUMP mysql > $MYSQL_TMP_DIR/mysql_database_backup_39343
+
+# Workaround for MDEV-22127 (can't import a dump created before 10.4)
+--exec $MYSQL -e "DROP TABLE IF EXISTS mysql.global_priv; DROP VIEW IF EXISTS mysql.user"
+
+--echo #
+--echo # Restore a MariaDB 10.4 mysql schema directly over the live tables -
+--echo # exactly like restoring a mariadb-dump backup taken on an older
+--echo # server. mysql_upgrade_info, on the filesystem, is untouched by this
+--echo # and still records the CURRENT version.
+--echo #
+--exec $MYSQL mysql < $MYSQLTEST_VARDIR/std_data/mysql_database_10.4.dump
+
+FLUSH PRIVILEGES;
+SHOW STATUS LIKE 'Grant_tables_intact';
+
+--echo #
+--echo # Before this fix, mysql_upgrade trusted the recorded version alone
+--echo # and would have reported no upgrade needed here - leaving the
+--echo # older, just-restored schema un-upgraded. It must now correctly
+--echo # detect that an upgrade is needed.
+--echo #
+--exec $MYSQL_UPGRADE --check-if-upgrade-is-needed 2>&1
+
+--echo # A plain run (no --force) must therefore actually perform the
+--echo # upgrade, instead of skipping it.
+--disable_result_log
+--exec $MYSQL_UPGRADE 2>&1
+--enable_result_log
+
+SHOW STATUS LIKE 'Grant_tables_intact';
+
+--echo # Already upgraded now.
+--replace_result $MYSQL_SERVER_VERSION X.Y.Z-MariaDB
+--error 1
+--exec $MYSQL_UPGRADE --check-if-upgrade-is-needed 2>&1
+
+--echo # Restore the original (current-version) mysql schema backup.
+--exec $MYSQL mysql < $MYSQL_TMP_DIR/mysql_database_backup_39343
+
+--let $MYSQLD_DATADIR= `select @@datadir`
+--remove_file $MYSQLD_DATADIR/mysql_upgrade_info
+
+--echo # End of 10.11 tests

scripts/mysql_system_tables_fix.sql

@@ -782,13 +782,14 @@ alter table procs_priv   modify User         char(128)  binary not null default
 alter table proc         modify definer      varchar(384) collate utf8mb3_bin not null default '';
 alter table proxies_priv modify User         char(128)  COLLATE utf8mb3_bin not null default '', modify Host char(255) binary DEFAULT '';
 alter table proxies_priv modify Proxied_user char(128)  COLLATE utf8mb3_bin not null default '';
+alter table proxies_priv modify Proxied_host char(255)  binary not null default '';
 alter table proxies_priv modify Grantor      varchar(384) COLLATE utf8mb3_bin not null default '';
 alter table servers      modify Username     char(128)                   not null default '';
 alter table procs_priv   modify Grantor      varchar(384) COLLATE utf8mb3_bin not null default '';
 alter table tables_priv  modify Grantor      varchar(384) COLLATE utf8mb3_bin not null default '';
 # MDEV-33726 longer names from MDEV-24312 extension
 alter table if exists global_priv modify Host char(255) binary DEFAULT '', modify User char(128)  binary not null default '';
-alter table if exists roles_mapping modify Host char(255) binary not null DEFAULT '', modify User char(128)  binary not null default '';
+alter table if exists roles_mapping modify Host char(255) binary not null DEFAULT '', modify User char(128)  binary not null default '', modify Role char(128) binary not null default '';
 
 # Activate the new, possible modified privilege tables
 # This should not be needed, but gives us some extra testing that the above

sql/sql_acl.cc

@@ -69,9 +69,7 @@ bool using_global_priv_table= true;
   Set at the end of acl_reload()/grant_reload() (so recomputed on every
   server startup and every FLUSH PRIVILEGES) to reflect whether the
   on-disk grant tables' structure matches what this server binary
-  expects. Exposed read-only via SHOW STATUS (see acl_statistics /
-  status_vars in mysqld.cc) so a remote mariadb-upgrade can check it
-  without local filesystem access.
+  expects.
 */
 my_bool grant_tables_intact= FALSE;
 static Table_check_intact_log_error table_intact;
@@ -2986,18 +2984,22 @@ void acl_free(bool end)
 
 
 /*
-  Check that a grant table has the structure this server expects.
+Check that a grant table has the structure this server expects.
 
-  A table that wasn't opened is reported in the error log and treated as
-  not intact, the same as a table with a wrong structure.
-  Table_check_intact::check() must not be called with a NULL TABLE.
+  If the table doesn't exist (it wasn't opened), false is returned
+  without writing anything to the error log. If the table exists but has
+  the wrong structure, Table_check_intact::check() writes the details to
+  the error log and false is returned.
+
+  Table_check_intact::check() must not be called with a NULL TABLE, so the
+  existence check has to come first.
 
   @param table      Grant table, as returned by Grant_tables
-  @param name       Name of the table in the mysql schema, for the message
+  @param name       Name of the table in the mysql schema (currently unused)
   @param table_def  Expected structure (see grant_table_defs.cc)
 
-  @retval true   Table is intact
-  @retval false  Table is missing or its structure is wrong
+  @retval true   Table exists and its structure is as expected
+  @retval false  Table doesn't exist or its structure is wrong
 */
 
 static bool grant_table_is_intact(const Grant_table_base &table,
@@ -3006,7 +3008,6 @@ static bool grant_table_is_intact(const Grant_table_base &table,
 {
   if (!table.table_exists())
   {
-    sql_print_error("Table mysql.%s is missing", name);
     return false;
   }
   return !table_intact.check(table.table(), table_def);
@@ -3115,13 +3116,6 @@ bool acl_reload(THD *thd)
     /*
       Check every table, even if an earlier one isn't intact, so that all
       the problems are written to the error log.
-
-      The 'user' view is never opened here, it is only a client-facing
-      compatibility layer. Instead user_table() is mysql.global_priv, or the
-      old tabular mysql.user table on installations that haven't been
-      upgraded to global_priv yet (using_global_priv_table is false, set in
-      User_table_tabular::setup_sysvars()). Only global_priv has a
-      definition to compare with, so the legacy table is not checked.
     */
     bool db_intact=
       grant_table_is_intact(tables.db_table(), "db", &mysql_db_table_def);
@@ -8456,14 +8450,7 @@ bool grant_reload(THD *thd)
     my_hash_free(&old_package_body_priv_hash);
     free_root(&old_mem,MYF(0));
 
-    /*
-      acl_reload() always runs immediately before grant_reload(), both at
-      server startup and on FLUSH PRIVILEGES so it's safe to AND our tables'
-      results onto the value it just set rather than overwrite it.
-
-      Check every table, even if an earlier one isn't intact, so that all
-      the problems are written to the error log.
-    */
+    /* Check every table, even if an earlier one isn't intact. */
     bool tables_priv_intact=
       grant_table_is_intact(tables.tables_priv_table(), "tables_priv",
                             &mysql_tables_priv_table_def);