From bc747f5e32f65113b3e9ce5df6881e91a471d9ff Mon Sep 17 00:00:00 2001 From: Clemens Kolbitsch Date: Mon, 20 Jul 2020 18:19:18 -0700 Subject: [PATCH] Work around CREATE TABLE GCP bug GCP cloud-SQL DBs do not seem to allow using gh-ost due to the way that we create tables (using `CREATE TABLE LIKE `). This is a confirmed GCP bug: https://issuetracker.google.com/issues/158129960 For the time being, this commit adds an option to enable GCP support, using a different CREATE TABLE statement. --- RELEASE_VERSION | 2 +- doc/command-line-flags.md | 4 +++ doc/requirements-and-limitations.md | 2 +- go/base/context.go | 3 +- go/base/utils.go | 2 +- go/cmd/gh-ost/main.go | 3 +- go/logic/applier.go | 14 +++----- go/logic/inspect.go | 4 +-- go/logic/migrator.go | 50 +++++++++++++++++++++++++++-- 9 files changed, 65 insertions(+), 19 deletions(-) diff --git a/RELEASE_VERSION b/RELEASE_VERSION index f1339850d..9eb8304f9 100644 --- a/RELEASE_VERSION +++ b/RELEASE_VERSION @@ -1 +1 @@ -1.0.49 +1.0.49-0lastline1 diff --git a/doc/command-line-flags.md b/doc/command-line-flags.md index 629e9f958..e6a949e32 100644 --- a/doc/command-line-flags.md +++ b/doc/command-line-flags.md @@ -127,6 +127,10 @@ Table name prefix to be used on the temporary tables. Add this flag when executing on a 1st generation Google Cloud Platform (GCP). +### gcpv2 + +Add this flag when executing on a 2nd generation Google Cloud Platform (GCP). + ### heartbeat-interval-millis Default 100. See [`subsecond-lag`](subsecond-lag.md) for details. diff --git a/doc/requirements-and-limitations.md b/doc/requirements-and-limitations.md index 096e2c9a9..d5b3610d0 100644 --- a/doc/requirements-and-limitations.md +++ b/doc/requirements-and-limitations.md @@ -39,7 +39,7 @@ The `SUPER` privilege is required for `STOP SLAVE`, `START SLAVE` operations. Th - For example, you may not migrate `MyTable` if another table called `MYtable` exists in the same schema. - Amazon RDS works, but has its own [limitations](rds.md). -- Google Cloud SQL works, `--gcp` flag required. +- Google Cloud SQL works, `--gcp` or `--gcpv2` flag required for running on a 1st/2nd generation Cloud SQL, respectively. - Aliyun RDS works, `--aliyun-rds` flag required. - Multisource is not supported when migrating via replica. It _should_ work (but never tested) when connecting directly to master (`--allow-on-master`) diff --git a/go/base/context.go b/go/base/context.go index 52d02dd34..db3ae3ff5 100644 --- a/go/base/context.go +++ b/go/base/context.go @@ -94,7 +94,8 @@ type MigrationContext struct { IsTungsten bool DiscardForeignKeys bool AliyunRDS bool - GoogleCloudPlatform bool + GoogleCloudPlatformV1 bool + GoogleCloudPlatformV2 bool config ContextConfig configMutex *sync.Mutex diff --git a/go/base/utils.go b/go/base/utils.go index 99536f82e..6b82a1b8e 100644 --- a/go/base/utils.go +++ b/go/base/utils.go @@ -76,7 +76,7 @@ func ValidateConnection(db *gosql.DB, connectionConfig *mysql.ConnectionConfig, } // AliyunRDS set users port to "NULL", replace it by gh-ost param // GCP set users port to "NULL", replace it by gh-ost param - if migrationContext.AliyunRDS || migrationContext.GoogleCloudPlatform { + if migrationContext.AliyunRDS || migrationContext.GoogleCloudPlatformV1 { port = connectionConfig.Key.Port } else { portQuery := `select @@global.port` diff --git a/go/cmd/gh-ost/main.go b/go/cmd/gh-ost/main.go index 33fc6f4cb..8ca571cfc 100644 --- a/go/cmd/gh-ost/main.go +++ b/go/cmd/gh-ost/main.go @@ -76,7 +76,8 @@ func main() { flag.BoolVar(&migrationContext.SkipForeignKeyChecks, "skip-foreign-key-checks", false, "set to 'true' when you know for certain there are no foreign keys on your table, and wish to skip the time it takes for gh-ost to verify that") flag.BoolVar(&migrationContext.SkipStrictMode, "skip-strict-mode", false, "explicitly tell gh-ost binlog applier not to enforce strict sql mode") flag.BoolVar(&migrationContext.AliyunRDS, "aliyun-rds", false, "set to 'true' when you execute on Aliyun RDS.") - flag.BoolVar(&migrationContext.GoogleCloudPlatform, "gcp", false, "set to 'true' when you execute on a 1st generation Google Cloud Platform (GCP).") + flag.BoolVar(&migrationContext.GoogleCloudPlatformV1, "gcp", false, "set to 'true' when you execute on a 1st generation Google Cloud Platform (GCP).") + flag.BoolVar(&migrationContext.GoogleCloudPlatformV2, "gcpv2", false, "set to 'true' when you execute on a 2nd generation Google Cloud Platform (GCP).") executeFlag := flag.Bool("execute", false, "actually execute the alter & migrate the table. Default is noop: do some tests and exit") flag.BoolVar(&migrationContext.TestOnReplica, "test-on-replica", false, "Have the migration run on a replica, not on the master. At the end of migration replication is stopped, and tables are swapped and immediately swap-revert. Replication remains stopped and you can compare the two tables for building trust") diff --git a/go/logic/applier.go b/go/logic/applier.go index 5fb795bac..5f4f196fc 100644 --- a/go/logic/applier.go +++ b/go/logic/applier.go @@ -89,7 +89,7 @@ func (this *Applier) InitDBConnections() (err error) { if err := this.validateAndReadTimeZone(); err != nil { return err } - if !this.migrationContext.AliyunRDS && !this.migrationContext.GoogleCloudPlatform { + if !this.migrationContext.AliyunRDS && !this.migrationContext.GoogleCloudPlatformV1 { if impliedKey, err := mysql.GetInstanceKey(this.db); err != nil { return err } else { @@ -168,18 +168,12 @@ func (this *Applier) ValidateOrDropExistingTables() error { } // CreateGhostTable creates the ghost table on the applier host -func (this *Applier) CreateGhostTable() error { - query := fmt.Sprintf(`create /* gh-ost */ table %s.%s like %s.%s`, - sql.EscapeName(this.migrationContext.DatabaseName), - sql.EscapeName(this.migrationContext.GetGhostTableName()), - sql.EscapeName(this.migrationContext.DatabaseName), - sql.EscapeName(this.migrationContext.OriginalTableName), - ) - log.Infof("Creating ghost table %s.%s", +func (this *Applier) CreateGhostTable(createTableStatement string) error { + log.Infof("Creating ghost table `%s`.`%s`", sql.EscapeName(this.migrationContext.DatabaseName), sql.EscapeName(this.migrationContext.GetGhostTableName()), ) - if _, err := sqlutils.ExecNoPrepare(this.db, query); err != nil { + if _, err := sqlutils.ExecNoPrepare(this.db, createTableStatement); err != nil { return err } log.Infof("Ghost table created") diff --git a/go/logic/inspect.go b/go/logic/inspect.go index 31184b0c2..a239d4e84 100644 --- a/go/logic/inspect.go +++ b/go/logic/inspect.go @@ -53,7 +53,7 @@ func (this *Inspector) InitDBConnections() (err error) { if err := this.validateConnection(); err != nil { return err } - if !this.migrationContext.AliyunRDS && !this.migrationContext.GoogleCloudPlatform { + if !this.migrationContext.AliyunRDS && !this.migrationContext.GoogleCloudPlatformV1 { if impliedKey, err := mysql.GetInstanceKey(this.db); err != nil { return err } else { @@ -729,7 +729,7 @@ func (this *Inspector) getSharedColumns(originalColumns, ghostColumns *sql.Colum } // showCreateTable returns the `show create table` statement for given table -func (this *Inspector) showCreateTable(tableName string) (createTableStatement string, err error) { +func (this *Inspector) ShowCreateTable(tableName string) (createTableStatement string, err error) { var dummy string query := fmt.Sprintf(`show /* gh-ost */ create table %s.%s`, sql.EscapeName(this.migrationContext.DatabaseName), sql.EscapeName(tableName)) err = this.db.QueryRow(query).Scan(&dummy, &createTableStatement) diff --git a/go/logic/migrator.go b/go/logic/migrator.go index b1a238fda..1c5b0d4dd 100644 --- a/go/logic/migrator.go +++ b/go/logic/migrator.go @@ -1048,6 +1048,46 @@ func (this *Migrator) initiateThrottler() error { return nil } +func (this *Migrator) getCreateGhostTableStatement() (string, error) { + fullOriginalTableName := fmt.Sprintf(`%s.%s`, + sql.EscapeName(this.migrationContext.DatabaseName), + sql.EscapeName(this.migrationContext.OriginalTableName), + ) + fullGhostTableName := fmt.Sprintf(`%s.%s`, + sql.EscapeName(this.migrationContext.DatabaseName), + sql.EscapeName(this.migrationContext.GetGhostTableName()), + ) + + var createGhostTableStatement string + // XXX: It would be better to have a unified way of creating tables - is there a downside to + // using the rename? The replace strategy seems a bit "simple", but it is how other tools do + // it too (e.g., ghostferry) + if this.migrationContext.GoogleCloudPlatformV2 { + createTableStatement, err := this.inspector.ShowCreateTable(this.migrationContext.OriginalTableName) + if err != nil { + return "", err + } + + createGhostTableStatement = strings.Replace( + createTableStatement, + // NOTE: MySQL always gives a CREATE TABLE using the quotes table name without + // the database name + fmt.Sprintf("CREATE TABLE %s", sql.EscapeName(this.migrationContext.OriginalTableName)), + fmt.Sprintf("CREATE TABLE %s", fullGhostTableName), + 1, + ) + log.Debugf("Using GCP-compatible create statement: %s", createGhostTableStatement) + } else { + createGhostTableStatement = fmt.Sprintf(`create /* gh-ost */ table %s like %s`, + fullGhostTableName, + fullOriginalTableName, + ) + log.Debugf("Using default create statement: %s", createGhostTableStatement) + } + + return createGhostTableStatement, nil +} + func (this *Migrator) initiateApplier() error { this.applier = NewApplier(this.migrationContext) if err := this.applier.InitDBConnections(); err != nil { @@ -1060,7 +1100,13 @@ func (this *Migrator) initiateApplier() error { log.Errorf("Unable to create changelog table, see further error details. Perhaps a previous migration failed without dropping the table? OR is there a running migration? Bailing out") return err } - if err := this.applier.CreateGhostTable(); err != nil { + + createTableStatement, err := this.getCreateGhostTableStatement() + if err != nil { + log.Errorf("Unable to get ghost table schema, see further error details.") + return err + } + if err := this.applier.CreateGhostTable(createTableStatement); err != nil { log.Errorf("Unable to create ghost table, see further error details. Perhaps a previous migration failed without dropping the table? Bailing out") return err } @@ -1262,7 +1308,7 @@ func (this *Migrator) finalCleanup() error { atomic.StoreInt64(&this.migrationContext.CleanupImminentFlag, 1) if this.migrationContext.Noop { - if createTableStatement, err := this.inspector.showCreateTable(this.migrationContext.GetGhostTableName()); err == nil { + if createTableStatement, err := this.inspector.ShowCreateTable(this.migrationContext.GetGhostTableName()); err == nil { log.Infof("New table structure follows") fmt.Println(createTableStatement) } else {