Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SidecarDB: Don't ignore table charset and collation #16670

Merged
merged 5 commits into from
Sep 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion go/vt/sidecardb/schema/misc/heartbeat.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion go/vt/sidecardb/schema/misc/reparent_journal.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion go/vt/sidecardb/schema/onlineddl/schema_migrations.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion go/vt/sidecardb/schema/schemaengine/tables.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion go/vt/sidecardb/schema/schemaengine/udfs.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion go/vt/sidecardb/schema/schemaengine/views.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion go/vt/sidecardb/schema/twopc/dt_participant.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion go/vt/sidecardb/schema/twopc/dt_state.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion go/vt/sidecardb/schema/twopc/redo_state.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion go/vt/sidecardb/schema/twopc/redo_statement.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion go/vt/sidecardb/schema/vdiff/vdiff.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion go/vt/sidecardb/schema/vdiff/vdiff_log.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion go/vt/sidecardb/schema/vdiff/vdiff_table.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion go/vt/sidecardb/schema/vreplication/copy_state.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion go/vt/sidecardb/schema/vreplication/post_copy_action.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion go/vt/sidecardb/schema/vreplication/schema_version.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion go/vt/sidecardb/schema/vreplication/vreplication.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion go/vt/sidecardb/schema/vreplication/vreplication_log.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion go/vt/sidecardb/sidecardb.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and good catch. We might also just as well "upgrade" to TableCharsetCollateStrict - but as this works then that's fine.

AlterTableAlgorithmStrategy: schemadiff.AlterTableAlgorithmStrategyCopy,
}
env := schemadiff.NewEnv(si.env, si.coll)
Expand Down
86 changes: 86 additions & 0 deletions go/vt/sidecardb/sidecardb_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)))
})
}
}
Loading