Bug#33503328: Specific table schema causes table to become corrupted during MySQL 8 upgrade

Summary

Upgrading a table

  CREATE TABLE t1 (c1 int GENERATED ALWAYS AS (0),
                   c2 int) ENGINE=MyISAM;

from 5.7 to 8.x would fail. This would only occur for a
mix of regular columns and generated columns and when using
MyISAM.

The reason for this was that during that particular type
of upgrade for use with the data dictionary, we would align
NULL info one way, and the other way everywhere else. This
was caught by our sanity checks.

Analysis

Using our above table as an example: We always use two loops
to create columns; the first loop processes regular columns,
while the the second loop processes generated columns.
Therefore, we'd process c2 (a natural column) before c1 (a
generated column).

In older code, we'd count the NULL position up in order of
column creation so that c2 (which is the second column in the
CREATE statement, but the first column to be processed on
account of being the first regular column) will be processed
first and get the lower NULL position; c1 will be processed
second (on account of being a generated column and getting
created in the second loop) and get the higher NULL position.

In short, we process all regular columns first, then all
generated columns, and count up NULL position the entire
time.

In fill_columns_from_dd(), we processed columns in the same
order (first all natural columns, then all generated columns),
but would increase the NULL position even when no field was
processed (e.g. when skipping a generated column in the first
loop that only processes regular columns). Therefore, the
NULL positions ended up wrong and tripped our sanity check.

This patch aligns the NULL ordering so that the positions
generated in fill_columns_from_dd() are compatible with the
ordering created by earlier servers and indeed with the
behavior elsewhere in contemporary servers.

Change-Id: I7bd18e0822c1d86b9dd322a5d994200078ebf84c

Author: Tatiana Azundris Nuernberg

mysql-test/r/dd_upgrade_33503328.result

@@ -0,0 +1,33 @@
+########################################################################
+# Bug#33503328: Specific table schema causes table to become corrupted
+#               during MySQL 8 upgrade
+########################################################################
+########################################################################
+# Copy and unzip the datadir, and stop the server.
+########################################################################
+########################################################################
+# Restart the server to trigger upgrade.
+########################################################################
+# restart: --datadir=MYSQLD_DATADIR_UPGRADE --log-error=MYSQLD_LOG --log-error-verbosity=3
+########################################################################
+# Let's see whether we upgrade the MyISAM table t1 correctly.
+########################################################################
+DESC t1;
+Field	Type	Null	Key	Default	Extra
+c1	int	YES		NULL	VIRTUAL GENERATED
+c2	int	YES		NULL	
+SHOW CREATE TABLE t1;
+Table	Create Table
+t1	CREATE TABLE `t1` (
+  `c1` int GENERATED ALWAYS AS (0) VIRTUAL,
+  `c2` int DEFAULT NULL
+) ENGINE=MyISAM DEFAULT CHARSET=latin1
+SELECT * FROM t1;
+c1	c2
+REPAIR TABLE t1;
+Table	Op	Msg_type	Msg_text
+test.t1	repair	status	OK
+########################################################################
+# Stop the server and do cleanup.
+########################################################################
+# restart:

mysql-test/std_data/upgrade/data_50745_33503328.zip

@@ -0,0 +1,68 @@
+--source include/not_valgrind.inc
+
+--echo ########################################################################
+--echo # Bug#33503328: Specific table schema causes table to become corrupted
+--echo #               during MySQL 8 upgrade
+--echo ########################################################################
+
+--let $MYSQLD_LOG= $MYSQLTEST_VARDIR/log/save_dd_upgrade.log
+
+--echo ########################################################################
+--echo # Copy and unzip the datadir, and stop the server.
+--echo ########################################################################
+
+# The datadir is created by building server version 5.7
+# and executing the following SQL statements:
+#
+# USE test;
+# CREATE TABLE `t1` (
+# `c1` int GENERATED ALWAYS AS (0),
+# `c2` int
+# ) ENGINE=MyISAM;
+#
+# (which SHOW CREATE TABLEs as
+#   `c1` int GENERATED ALWAYS AS (0) VIRTUAL,
+#   `c2` int DEFAULT NULL )
+#
+# Then, move data/ to data_50745_33503328/, and finally zip the entire
+# directory (zip -r data_50745_33503328.zip data_50745_33503328).
+
+--copy_file $MYSQLTEST_VARDIR/std_data/upgrade/data_50745_33503328.zip $MYSQL_TMP_DIR/data_50745_33503328.zip
+--file_exists $MYSQL_TMP_DIR/data_50745_33503328.zip
+--exec unzip -qo $MYSQL_TMP_DIR/data_50745_33503328.zip -d $MYSQL_TMP_DIR
+--let $MYSQLD_DATADIR_UPGRADE = $MYSQL_TMP_DIR/data_50745_33503328
+
+--echo ########################################################################
+--echo # Restart the server to trigger upgrade.
+--echo ########################################################################
+--let $shutdown_server_timeout= 300
+--let $wait_counter= 10000
+--let $restart_parameters= restart: --datadir=$MYSQLD_DATADIR_UPGRADE --log-error=$MYSQLD_LOG --log-error-verbosity=3
+--replace_result $MYSQLD_DATADIR_UPGRADE MYSQLD_DATADIR_UPGRADE $MYSQLD_LOG MYSQLD_LOG
+--source include/restart_mysqld.inc
+
+--echo ########################################################################
+--echo # Let's see whether we upgrade the MyISAM table t1 correctly.
+--echo ########################################################################
+
+DESC t1;
+
+# Unpatched server will fail with ER_NOT_KEYFILE.
+SHOW CREATE TABLE t1;
+
+# Unpatched server will fail with ER_NOT_KEYFILE.
+SELECT * FROM t1;
+
+REPAIR TABLE t1;
+
+
+--echo ########################################################################
+--echo # Stop the server and do cleanup.
+--echo ########################################################################
+--let $shutdown_server_timeout= 300
+--source include/shutdown_mysqld.inc
+--remove_file $MYSQL_TMP_DIR/data_50745_33503328.zip
+--force-rmdir $MYSQL_TMP_DIR/data_50745_33503328
+--remove_file $MYSQLD_LOG
+--let $restart_parameters= restart:
+--source include/start_mysqld.inc

mysql-test/t/dd_upgrade_33503328.test

@@ -1142,19 +1142,20 @@ static bool fill_columns_from_dd(THD *thd, TABLE_SHARE *share,
                               rec_pos, field_nr))
         return true;
 
+      /*
+        Account for NULL bits if it's a regular column.
+        If it's a generated column, we do it below so the NULL
+        bits end up in the expected order.
+      */
+      if ((null_bit_pos += column_preamble_bits(col_obj)) > 7) {
+        null_pos++;
+        null_bit_pos -= 8;
+      }
+
       rec_pos += share->field[field_nr]->pack_length_in_rec();
     } else
       has_vgc = true;
 
-    /*
-      Virtual generated columns still need to be accounted in null bits and
-      field_nr calculations, since they reside at the normal place in record
-      preamble and TABLE_SHARE::field array.
-    */
-    if ((null_bit_pos += column_preamble_bits(col_obj)) > 7) {
-      null_pos++;
-      null_bit_pos -= 8;
-    }
     field_nr++;
   }
 
@@ -1167,8 +1168,6 @@ static bool fill_columns_from_dd(THD *thd, TABLE_SHARE *share,
         static_cast<ulong>(rec_pos - share->default_values))
       share->stored_rec_length = (rec_pos - share->default_values);
 
-    null_pos = share->default_values;
-    null_bit_pos = (share->db_create_options & HA_OPTION_PACK_RECORD) ? 0 : 1;
     field_nr = 0;
 
     for (const dd::Column *col_obj2 : tab_obj->columns()) {
@@ -1181,17 +1180,18 @@ static bool fill_columns_from_dd(THD *thd, TABLE_SHARE *share,
                                 rec_pos, field_nr))
           return true;
 
+        /*
+          Account for generated columns -- we do this separately here
+          so the NULL bits end up in the expected order.
+        */
+        if ((null_bit_pos += column_preamble_bits(col_obj2)) > 7) {
+          null_pos++;
+          null_bit_pos -= 8;
+        }
+
         rec_pos += share->field[field_nr]->pack_length_in_rec();
       }
 
-      /*
-        Account for all columns while evaluating null_pos/null_bit_pos and
-        field_nr.
-      */
-      if ((null_bit_pos += column_preamble_bits(col_obj2)) > 7) {
-        null_pos++;
-        null_bit_pos -= 8;
-      }
       field_nr++;
     }
   }