From 0ccf2ce00798bb36ea7b72347186297b82b88f93 Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Sun, 8 Sep 2024 13:31:13 -0400 Subject: [PATCH] SidecarDB: Don't ignore table charset and collation (#16670) Signed-off-by: Matt Lord --- go/vt/sidecardb/schema/misc/heartbeat.sql | 2 +- .../schema/misc/reparent_journal.sql | 2 +- .../schema/onlineddl/schema_migrations.sql | 2 +- .../sidecardb/schema/schemaengine/tables.sql | 2 +- go/vt/sidecardb/schema/schemaengine/udfs.sql | 2 +- go/vt/sidecardb/schema/schemaengine/views.sql | 2 +- .../sidecardb/schema/twopc/dt_participant.sql | 2 +- go/vt/sidecardb/schema/twopc/dt_state.sql | 2 +- go/vt/sidecardb/schema/twopc/redo_state.sql | 2 +- .../sidecardb/schema/twopc/redo_statement.sql | 2 +- go/vt/sidecardb/schema/vdiff/vdiff.sql | 2 +- go/vt/sidecardb/schema/vdiff/vdiff_log.sql | 2 +- go/vt/sidecardb/schema/vdiff/vdiff_table.sql | 2 +- .../schema/vreplication/copy_state.sql | 2 +- .../schema/vreplication/post_copy_action.sql | 2 +- .../vreplication/resharding_journal.sql | 2 +- .../schema/vreplication/schema_version.sql | 2 +- .../schema/vreplication/vreplication.sql | 2 +- .../schema/vreplication/vreplication_log.sql | 2 +- go/vt/sidecardb/sidecardb.go | 2 +- go/vt/sidecardb/sidecardb_test.go | 86 +++++++++++++++++++ 21 files changed, 106 insertions(+), 20 deletions(-) diff --git a/go/vt/sidecardb/schema/misc/heartbeat.sql b/go/vt/sidecardb/schema/misc/heartbeat.sql index 35668f2c0ab..b95feb57684 100644 --- a/go/vt/sidecardb/schema/misc/heartbeat.sql +++ b/go/vt/sidecardb/schema/misc/heartbeat.sql @@ -20,4 +20,4 @@ CREATE TABLE IF NOT EXISTS heartbeat tabletUid INT UNSIGNED NOT NULL, ts BIGINT UNSIGNED NOT NULL, PRIMARY KEY (`keyspaceShard`) -) engine = InnoDB +) ENGINE = InnoDB CHARSET = utf8mb4 diff --git a/go/vt/sidecardb/schema/misc/reparent_journal.sql b/go/vt/sidecardb/schema/misc/reparent_journal.sql index 81e47c69dc5..e9f2f9c4aea 100644 --- a/go/vt/sidecardb/schema/misc/reparent_journal.sql +++ b/go/vt/sidecardb/schema/misc/reparent_journal.sql @@ -22,4 +22,4 @@ CREATE TABLE IF NOT EXISTS reparent_journal `replication_position` varbinary(64000) DEFAULT NULL, PRIMARY KEY (`time_created_ns`) -) ENGINE = InnoDB +) ENGINE = InnoDB CHARSET = utf8mb4 diff --git a/go/vt/sidecardb/schema/onlineddl/schema_migrations.sql b/go/vt/sidecardb/schema/onlineddl/schema_migrations.sql index 0e1b8ecde11..85d87edefc2 100644 --- a/go/vt/sidecardb/schema/onlineddl/schema_migrations.sql +++ b/go/vt/sidecardb/schema/onlineddl/schema_migrations.sql @@ -84,4 +84,4 @@ CREATE TABLE IF NOT EXISTS schema_migrations KEY `table_complete_idx` (`migration_status`, `keyspace`(64), `mysql_table`(64), `completed_timestamp`), KEY `migration_context_idx` (`migration_context`(64)), KEY `reverted_uuid_idx` (`reverted_uuid`) -) ENGINE = InnoDB +) ENGINE = InnoDB CHARSET = utf8mb4 diff --git a/go/vt/sidecardb/schema/schemaengine/tables.sql b/go/vt/sidecardb/schema/schemaengine/tables.sql index 3aadc7c9635..1c4f6117ef2 100644 --- a/go/vt/sidecardb/schema/schemaengine/tables.sql +++ b/go/vt/sidecardb/schema/schemaengine/tables.sql @@ -21,4 +21,4 @@ CREATE TABLE IF NOT EXISTS tables CREATE_STATEMENT longtext, CREATE_TIME BIGINT, PRIMARY KEY (TABLE_SCHEMA, TABLE_NAME) -) engine = InnoDB +) ENGINE = InnoDB CHARSET = utf8mb4 diff --git a/go/vt/sidecardb/schema/schemaengine/udfs.sql b/go/vt/sidecardb/schema/schemaengine/udfs.sql index 90c6143fbd6..53fa8280252 100644 --- a/go/vt/sidecardb/schema/schemaengine/udfs.sql +++ b/go/vt/sidecardb/schema/schemaengine/udfs.sql @@ -20,4 +20,4 @@ CREATE TABLE IF NOT EXISTS udfs FUNCTION_RETURN_TYPE varchar(20) CHARACTER SET `utf8mb3` COLLATE `utf8mb3_bin` NOT NULL, FUNCTION_TYPE varchar(20) CHARACTER SET `utf8mb3` COLLATE `utf8mb3_bin` NOT NULL, PRIMARY KEY (FUNCTION_NAME) -) engine = InnoDB +) ENGINE = InnoDB CHARSET = utf8mb4 diff --git a/go/vt/sidecardb/schema/schemaengine/views.sql b/go/vt/sidecardb/schema/schemaengine/views.sql index dd242e6567f..97a6fa7a2f3 100644 --- a/go/vt/sidecardb/schema/schemaengine/views.sql +++ b/go/vt/sidecardb/schema/schemaengine/views.sql @@ -21,4 +21,4 @@ CREATE TABLE IF NOT EXISTS views CREATE_STATEMENT longtext, VIEW_DEFINITION longtext NOT NULL, PRIMARY KEY (TABLE_SCHEMA, TABLE_NAME) -) engine = InnoDB +) ENGINE = InnoDB CHARSET = utf8mb4 diff --git a/go/vt/sidecardb/schema/twopc/dt_participant.sql b/go/vt/sidecardb/schema/twopc/dt_participant.sql index 9f2408497eb..3fe98c726ee 100644 --- a/go/vt/sidecardb/schema/twopc/dt_participant.sql +++ b/go/vt/sidecardb/schema/twopc/dt_participant.sql @@ -21,4 +21,4 @@ CREATE TABLE IF NOT EXISTS dt_participant keyspace varchar(256) NOT NULL, shard varchar(256) NOT NULL, primary key(dtid, id) -) ENGINE = InnoDB +) ENGINE = InnoDB CHARSET = utf8mb4 diff --git a/go/vt/sidecardb/schema/twopc/dt_state.sql b/go/vt/sidecardb/schema/twopc/dt_state.sql index 9247ea5fa3c..7315ea5aa23 100644 --- a/go/vt/sidecardb/schema/twopc/dt_state.sql +++ b/go/vt/sidecardb/schema/twopc/dt_state.sql @@ -21,4 +21,4 @@ CREATE TABLE IF NOT EXISTS dt_state time_created bigint NOT NULL, primary key(dtid), key (time_created) -) ENGINE = InnoDB +) ENGINE = InnoDB CHARSET = utf8mb4 diff --git a/go/vt/sidecardb/schema/twopc/redo_state.sql b/go/vt/sidecardb/schema/twopc/redo_state.sql index 7e583d7fdcd..975124e0320 100644 --- a/go/vt/sidecardb/schema/twopc/redo_state.sql +++ b/go/vt/sidecardb/schema/twopc/redo_state.sql @@ -19,4 +19,4 @@ CREATE TABLE IF NOT EXISTS redo_state( state bigint NOT NULL, time_created bigint NOT NULL, primary key(dtid) -) ENGINE = InnoDB +) ENGINE = InnoDB CHARSET = utf8mb4 diff --git a/go/vt/sidecardb/schema/twopc/redo_statement.sql b/go/vt/sidecardb/schema/twopc/redo_statement.sql index 9208a0fce65..7f28e7fb0c9 100644 --- a/go/vt/sidecardb/schema/twopc/redo_statement.sql +++ b/go/vt/sidecardb/schema/twopc/redo_statement.sql @@ -19,4 +19,4 @@ CREATE TABLE IF NOT EXISTS redo_statement( id bigint NOT NULL, statement mediumblob NOT NULL, primary key(dtid, id) -) ENGINE = InnoDB +) ENGINE = InnoDB CHARSET = utf8mb4 diff --git a/go/vt/sidecardb/schema/vdiff/vdiff.sql b/go/vt/sidecardb/schema/vdiff/vdiff.sql index 52392bde427..862e6e0e9d3 100644 --- a/go/vt/sidecardb/schema/vdiff/vdiff.sql +++ b/go/vt/sidecardb/schema/vdiff/vdiff.sql @@ -33,4 +33,4 @@ CREATE TABLE IF NOT EXISTS vdiff UNIQUE KEY `uuid_idx` (`vdiff_uuid`), KEY `state` (`state`), KEY `ks_wf_idx` (`keyspace`(64), `workflow`(64)) -) ENGINE = InnoDB +) ENGINE = InnoDB CHARSET = utf8mb4 diff --git a/go/vt/sidecardb/schema/vdiff/vdiff_log.sql b/go/vt/sidecardb/schema/vdiff/vdiff_log.sql index dbc110e5b3a..bedb7824295 100644 --- a/go/vt/sidecardb/schema/vdiff/vdiff_log.sql +++ b/go/vt/sidecardb/schema/vdiff/vdiff_log.sql @@ -21,4 +21,4 @@ CREATE TABLE IF NOT EXISTS vdiff_log `created_at` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP, `message` text NOT NULL, PRIMARY KEY (`id`) -) ENGINE = InnoDB +) ENGINE = InnoDB CHARSET = utf8mb4 diff --git a/go/vt/sidecardb/schema/vdiff/vdiff_table.sql b/go/vt/sidecardb/schema/vdiff/vdiff_table.sql index 580f1ba96ee..2296398e430 100644 --- a/go/vt/sidecardb/schema/vdiff/vdiff_table.sql +++ b/go/vt/sidecardb/schema/vdiff/vdiff_table.sql @@ -27,4 +27,4 @@ CREATE TABLE IF NOT EXISTS vdiff_table `created_at` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP, `updated_at` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, PRIMARY KEY (`vdiff_id`, `table_name`) -) ENGINE = InnoDB +) ENGINE = InnoDB CHARSET = utf8mb4 diff --git a/go/vt/sidecardb/schema/vreplication/copy_state.sql b/go/vt/sidecardb/schema/vreplication/copy_state.sql index 8f27bc9dc86..f3e577156b9 100644 --- a/go/vt/sidecardb/schema/vreplication/copy_state.sql +++ b/go/vt/sidecardb/schema/vreplication/copy_state.sql @@ -22,4 +22,4 @@ CREATE TABLE IF NOT EXISTS copy_state `lastpk` varbinary(2000) DEFAULT NULL, PRIMARY KEY (`id`), KEY `vrepl_id` (`vrepl_id`,`table_name`) -) ENGINE = InnoDB +) ENGINE = InnoDB CHARSET = utf8mb4 diff --git a/go/vt/sidecardb/schema/vreplication/post_copy_action.sql b/go/vt/sidecardb/schema/vreplication/post_copy_action.sql index 85bb44923b0..15a28c0d5fe 100644 --- a/go/vt/sidecardb/schema/vreplication/post_copy_action.sql +++ b/go/vt/sidecardb/schema/vreplication/post_copy_action.sql @@ -21,4 +21,4 @@ CREATE TABLE IF NOT EXISTS post_copy_action( action JSON NOT NULL, UNIQUE KEY (vrepl_id, table_name), PRIMARY KEY(id) -) ENGINE = InnoDB +) ENGINE = InnoDB CHARSET = utf8mb4 diff --git a/go/vt/sidecardb/schema/vreplication/resharding_journal.sql b/go/vt/sidecardb/schema/vreplication/resharding_journal.sql index 5a3dbd64890..fed943a5064 100644 --- a/go/vt/sidecardb/schema/vreplication/resharding_journal.sql +++ b/go/vt/sidecardb/schema/vreplication/resharding_journal.sql @@ -20,4 +20,4 @@ CREATE TABLE IF NOT EXISTS resharding_journal `db_name` varbinary(255) DEFAULT NULL, `val` blob, PRIMARY KEY (`id`) -) ENGINE = InnoDB +) ENGINE = InnoDB CHARSET = utf8mb4 diff --git a/go/vt/sidecardb/schema/vreplication/schema_version.sql b/go/vt/sidecardb/schema/vreplication/schema_version.sql index 2b7cbc08dec..be327215071 100644 --- a/go/vt/sidecardb/schema/vreplication/schema_version.sql +++ b/go/vt/sidecardb/schema/vreplication/schema_version.sql @@ -22,4 +22,4 @@ CREATE TABLE IF NOT EXISTS schema_version ddl BLOB DEFAULT NULL, schemax LONGBLOB NOT NULL, PRIMARY KEY (id) -) ENGINE = InnoDB +) ENGINE = InnoDB CHARSET = utf8mb4 diff --git a/go/vt/sidecardb/schema/vreplication/vreplication.sql b/go/vt/sidecardb/schema/vreplication/vreplication.sql index 293997056fd..6670b671f4f 100644 --- a/go/vt/sidecardb/schema/vreplication/vreplication.sql +++ b/go/vt/sidecardb/schema/vreplication/vreplication.sql @@ -47,4 +47,4 @@ CREATE TABLE IF NOT EXISTS vreplication `options` json NOT NULL, PRIMARY KEY (`id`), KEY `workflow_idx` (`workflow`(64)) -) ENGINE = InnoDB +) ENGINE = InnoDB CHARSET = utf8mb4 diff --git a/go/vt/sidecardb/schema/vreplication/vreplication_log.sql b/go/vt/sidecardb/schema/vreplication/vreplication_log.sql index 19360fb0c04..950c52ab4aa 100644 --- a/go/vt/sidecardb/schema/vreplication/vreplication_log.sql +++ b/go/vt/sidecardb/schema/vreplication/vreplication_log.sql @@ -26,4 +26,4 @@ CREATE TABLE IF NOT EXISTS vreplication_log `count` bigint NOT NULL DEFAULT '1', PRIMARY KEY (`id`), KEY `vrepl_id_idx` (`vrepl_id`) -) ENGINE = InnoDB +) ENGINE = InnoDB CHARSET = utf8mb4 diff --git a/go/vt/sidecardb/sidecardb.go b/go/vt/sidecardb/sidecardb.go index 0947355cd46..9cbe0540033 100644 --- a/go/vt/sidecardb/sidecardb.go +++ b/go/vt/sidecardb/sidecardb.go @@ -396,7 +396,7 @@ func (si *schemaInit) getCurrentSchema(tableName string) (string, error) { // or an ALTER if the table exists but has a different schema. func (si *schemaInit) findTableSchemaDiff(tableName, current, desired string) (string, error) { hints := &schemadiff.DiffHints{ - TableCharsetCollateStrategy: schemadiff.TableCharsetCollateIgnoreAlways, + TableCharsetCollateStrategy: schemadiff.TableCharsetCollateIgnoreEmpty, AlterTableAlgorithmStrategy: schemadiff.AlterTableAlgorithmStrategyCopy, } env := schemadiff.NewEnv(si.env, si.coll) diff --git a/go/vt/sidecardb/sidecardb_test.go b/go/vt/sidecardb/sidecardb_test.go index 70b89f17056..78210f19150 100644 --- a/go/vt/sidecardb/sidecardb_test.go +++ b/go/vt/sidecardb/sidecardb_test.go @@ -250,3 +250,89 @@ func TestAlterTableAlgorithm(t *testing.T) { }) } } + +// TestTableSchemaDiff ensures that the diff produced by schemaInit.findTableSchemaDiff +// is resulting in the expected alter table statements in a variety of scenarios. +func TestTableSchemaDiff(t *testing.T) { + si := &schemaInit{ + env: vtenv.NewTestEnv(), + } + + type testCase struct { + name string + table string + oldSchema string + newSchema string + expectNoDiff bool + expectedAlter string + } + testCases := []testCase{ + { + name: "modify table charset", + table: "t1", + oldSchema: "create table if not exists _vt.t1(i int) charset=utf8mb4", + newSchema: "create table if not exists _vt.t(i int) charset=utf8mb3", + expectedAlter: "alter table _vt.t1 charset utf8mb3, algorithm = copy", + }, + { + name: "empty charset", + table: "t1", + oldSchema: "create table if not exists _vt.t1(i int) charset=utf8mb4", + newSchema: "create table if not exists _vt.t(i int)", + expectNoDiff: true, // We're not specifying an explicit charset in the new schema, so we shouldn't see a diff. + }, + { + name: "modify table engine", + table: "t1", + oldSchema: "create table if not exists _vt.t1(i int) engine=myisam", + newSchema: "create table if not exists _vt.t(i int) engine=innodb", + expectedAlter: "alter table _vt.t1 engine innodb, algorithm = copy", + }, + { + name: "add, modify, transfer PK", + table: "t1", + oldSchema: "create table _vt.t1 (i int primary key, i1 varchar(10)) charset utf8mb4", + newSchema: "create table _vt.t1 (i int, i1 varchar(20) character set utf8mb3 collate utf8mb3_bin, i2 int, primary key (i2)) charset utf8mb4", + expectedAlter: "alter table _vt.t1 drop primary key, modify column i1 varchar(20) character set utf8mb3 collate utf8mb3_bin, add column i2 int, add primary key (i2), algorithm = copy", + }, + { + name: "modify visibility and add comment", + table: "t1", + oldSchema: "create table if not exists _vt.t1(c1 int, c2 int, c3 varchar(100)) charset utf8mb4", + newSchema: "create table if not exists _vt.t1(c1 int, c2 int, c3 varchar(100) invisible comment 'hoping to drop') charset utf8mb4", + expectedAlter: "alter table _vt.t1 modify column c3 varchar(100) comment 'hoping to drop' invisible, algorithm = copy", + }, + { + name: "add PK and remove index", + table: "t1", + oldSchema: "create table if not exists _vt.t1(c1 int, c2 int, c3 varchar(100), key (c2)) charset utf8mb4", + newSchema: "create table if not exists _vt.t1(c1 int primary key, c2 int, c3 varchar(100)) charset utf8mb4", + expectedAlter: "alter table _vt.t1 drop key c2, add primary key (c1), algorithm = copy", + }, + { + name: "add generated col", + table: "t1", + oldSchema: "create table if not exists _vt.t1(c1 int primary key) charset utf8mb4", + newSchema: "create table if not exists _vt.t1(c1 int primary key, c2 varchar(10) generated always as ('hello')) charset utf8mb4", + expectedAlter: "alter table _vt.t1 add column c2 varchar(10) as ('hello') virtual, algorithm = copy", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + diff, err := si.findTableSchemaDiff(tc.table, tc.oldSchema, tc.newSchema) + require.NoError(t, err) + if tc.expectNoDiff { + require.Empty(t, diff) + return + } + stmt, err := si.env.Parser().Parse(diff) + require.NoError(t, err) + alter, ok := stmt.(*sqlparser.AlterTable) + require.True(t, ok) + require.NotNil(t, alter) + t.Logf("alter: %s", sqlparser.String(alter)) + require.Equal(t, strings.ToLower(tc.expectedAlter), strings.ToLower(sqlparser.String(alter))) + }) + } +}