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

[parser] support create-table flag #1383

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
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
10 changes: 6 additions & 4 deletions go/base/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,10 +77,12 @@ func NewThrottleCheckResult(throttle bool, reason string, reasonHint ThrottleRea
type MigrationContext struct {
Uuid string

DatabaseName string
OriginalTableName string
AlterStatement string
AlterStatementOptions string // anything following the 'ALTER TABLE [schema.]table' from AlterStatement
DatabaseName string
OriginalTableName string
AlterStatement string
AlterStatementOptions string // anything following the 'ALTER TABLE [schema.]table' from AlterStatement
CreateTableStatement string
CreateTableStatementBody string // anything following the 'CREATE TABLE [schema.]table' from CreateTableStatement

countMutex sync.Mutex
countTableRowsCancelFunc func()
Expand Down
45 changes: 38 additions & 7 deletions go/cmd/gh-ost/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@ func main() {

flag.StringVar(&migrationContext.DatabaseName, "database", "", "database name (mandatory)")
flag.StringVar(&migrationContext.OriginalTableName, "table", "", "table name (mandatory)")
flag.StringVar(&migrationContext.AlterStatement, "alter", "", "alter statement (mandatory)")
flag.StringVar(&migrationContext.AlterStatement, "alter", "", "alter statement (mandatory if no '--create-table' statement provided)")
flag.StringVar(&migrationContext.CreateTableStatement, "create-table", "", "create table statement (mandatory if no '--alter' statement provided)")
flag.BoolVar(&migrationContext.AttemptInstantDDL, "attempt-instant-ddl", false, "Attempt to use instant DDL for this migration first")
storageEngine := flag.String("storage-engine", "innodb", "Specify table storage engine (default: 'innodb'). When 'rocksdb': the session transaction isolation level is changed from REPEATABLE_READ to READ_COMMITTED.")

Expand Down Expand Up @@ -191,17 +192,29 @@ func main() {
migrationContext.Log.Fatale(err)
}

if migrationContext.AlterStatement == "" {
log.Fatal("--alter must be provided and statement must not be empty")
if migrationContext.AlterStatement == "" && migrationContext.CreateTableStatement == "" {
log.Fatal("--alter or --create-table must be provided and statement must not be empty")
}

if migrationContext.AlterStatement != "" && migrationContext.CreateTableStatement != "" {
log.Fatal("--alter and --create-table cannot both be provided at the same time")
}

var parser sql.Parser
if migrationContext.CreateTableStatement != "" {
confirmNoAlterStatementFlags(migrationContext)
parser = sql.NewParserFromCreateTableStatement(migrationContext.AlterStatement)
migrationContext.CreateTableStatementBody = parser.GetOptions()
} else {
parser = sql.NewParserFromAlterStatement(migrationContext.AlterStatement)
migrationContext.AlterStatementOptions = parser.GetOptions()
}
parser := sql.NewParserFromAlterStatement(migrationContext.AlterStatement)
migrationContext.AlterStatementOptions = parser.GetAlterStatementOptions()

if migrationContext.DatabaseName == "" {
if parser.HasExplicitSchema() {
migrationContext.DatabaseName = parser.GetExplicitSchema()
} else {
log.Fatal("--database must be provided and database name must not be empty, or --alter must specify database name")
log.Fatal(fmt.Sprintf("--database must be provided and database name must not be empty, or --%v must specify database name", parser.Type().String()))
}
}

Expand All @@ -213,7 +226,7 @@ func main() {
if parser.HasExplicitTable() {
migrationContext.OriginalTableName = parser.GetExplicitTable()
} else {
log.Fatal("--table must be provided and table name must not be empty, or --alter must specify table name")
log.Fatal(fmt.Sprintf("--table must be provided and table name must not be empty, or --%v must specify table name", parser.Type().String()))
}
}
migrationContext.Noop = !(*executeFlag)
Expand Down Expand Up @@ -321,3 +334,21 @@ func main() {
}
fmt.Fprintln(os.Stdout, "# Done")
}

func confirmNoAlterStatementFlags(migrationContext *base.MigrationContext) {
if migrationContext.AttemptInstantDDL {
migrationContext.Log.Fatal("--attempt-instant-ddl cannot be used with --create-table")
}
if migrationContext.ApproveRenamedColumns {
migrationContext.Log.Fatal("--approve-renamed-columns cannot be used with --create-table")
}
if migrationContext.SkipRenamedColumns {
migrationContext.Log.Fatal("--skip-renamed-columns cannot be used with --create-table")
}
if migrationContext.DiscardForeignKeys {
migrationContext.Log.Fatal("--discard-foreign-keys cannot be used with --create-table")
}
if migrationContext.SkipForeignKeyChecks {
migrationContext.Log.Fatal("--skip-foreign-key-checks cannot be used with --create-table")
}
}
25 changes: 19 additions & 6 deletions go/logic/applier.go
Original file line number Diff line number Diff line change
Expand Up @@ -218,14 +218,24 @@ func (this *Applier) AttemptInstantDDL() error {
return err
}

func (this *Applier) generateCreateGhostTableQuery() string {
query := fmt.Sprintf(`create /* gh-ost */ table %s.%s `,
sql.EscapeName(this.migrationContext.DatabaseName),
sql.EscapeName(this.migrationContext.GetGhostTableName()))
if this.migrationContext.CreateTableStatementBody != "" {
query += this.migrationContext.CreateTableStatementBody
} else {
query += fmt.Sprintf(`like %s.%s`,
sql.EscapeName(this.migrationContext.DatabaseName),
sql.EscapeName(this.migrationContext.OriginalTableName),
)
}
return query
}

// 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),
)
query := this.generateCreateGhostTableQuery()
this.migrationContext.Log.Infof("Creating ghost table %s.%s",
sql.EscapeName(this.migrationContext.DatabaseName),
sql.EscapeName(this.migrationContext.GetGhostTableName()),
Expand Down Expand Up @@ -261,6 +271,9 @@ func (this *Applier) CreateGhostTable() error {

// AlterGhost applies `alter` statement on ghost table
func (this *Applier) AlterGhost() error {
if this.migrationContext.AlterStatement == "" {
return nil
}
query := fmt.Sprintf(`alter /* gh-ost */ table %s.%s %s`,
sql.EscapeName(this.migrationContext.DatabaseName),
sql.EscapeName(this.migrationContext.GetGhostTableName()),
Expand Down
23 changes: 23 additions & 0 deletions go/logic/applier_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -183,3 +183,26 @@ func TestApplierInstantDDL(t *testing.T) {
test.S(t).ExpectEquals(stmt, "ALTER /* gh-ost */ TABLE `test`.`mytable` ADD INDEX (foo), ALGORITHM=INSTANT")
})
}

func TestApplierCreateGhostTable(t *testing.T) {
t.Run("default", func(t *testing.T) {
migrationContext := base.NewMigrationContext()
migrationContext.DatabaseName = "test"
migrationContext.OriginalTableName = "mytable"
applier := NewApplier(migrationContext)

stmt := applier.generateCreateGhostTableQuery()
test.S(t).ExpectEquals(stmt, "create /* gh-ost */ table `test`.`_mytable_gho` like `test`.`mytable`")
})

t.Run("withCustomCreateTable", func(t *testing.T) {
migrationContext := base.NewMigrationContext()
migrationContext.DatabaseName = "test"
migrationContext.OriginalTableName = "mytable"
migrationContext.CreateTableStatementBody = "(id VARCHAR(24), PRIMARY KEY(id))"
applier := NewApplier(migrationContext)

stmt := applier.generateCreateGhostTableQuery()
test.S(t).ExpectEquals(stmt, "create /* gh-ost */ table `test`.`_mytable_gho` (id VARCHAR(24), PRIMARY KEY(id))")
})
}
1 change: 1 addition & 0 deletions go/logic/hooks.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ func (this *HooksExecutor) applyEnvironmentVariables(extraVariables ...string) [
env = append(env, fmt.Sprintf("GH_OST_GHOST_TABLE_NAME=%s", this.migrationContext.GetGhostTableName()))
env = append(env, fmt.Sprintf("GH_OST_OLD_TABLE_NAME=%s", this.migrationContext.GetOldTableName()))
env = append(env, fmt.Sprintf("GH_OST_DDL=%s", this.migrationContext.AlterStatement))
env = append(env, fmt.Sprintf("GH_OST_CREATE_TABLE=%s", this.migrationContext.CreateTableStatement))
env = append(env, fmt.Sprintf("GH_OST_ELAPSED_SECONDS=%f", this.migrationContext.ElapsedTime().Seconds()))
env = append(env, fmt.Sprintf("GH_OST_ELAPSED_COPY_SECONDS=%f", this.migrationContext.ElapsedRowCopyTime().Seconds()))
estimatedRows := atomic.LoadInt64(&this.migrationContext.RowsEstimate) + atomic.LoadInt64(&this.migrationContext.RowsDeltaEstimate)
Expand Down
77 changes: 55 additions & 22 deletions go/logic/migrator.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ import (
)

var (
ErrMigratorUnsupportedRenameAlter = errors.New("ALTER statement seems to RENAME the table. This is not supported, and you should run your RENAME outside gh-ost.")
ErrMigratorUnsupportedRenameAlter = errors.New("ALTER statement seems to RENAME the table. This is not supported, and you should run your RENAME outside of gh-ost.")
ErrMigratorUnsupportedRenameCreateTable = errors.New("CREATE TABLE statement seems to RENAME the table. This is not supported, and you should first run your RENAME outside of gh-ost.")
)

type ChangelogState string
Expand Down Expand Up @@ -69,7 +70,7 @@ const (
// Migrator is the main schema migration flow manager.
type Migrator struct {
appVersion string
parser *sql.AlterTableParser
parser sql.Parser
inspector *Inspector
applier *Applier
eventsStreamer *EventsStreamer
Expand All @@ -94,21 +95,27 @@ type Migrator struct {
finishedMigrating int64
}

func getParser(context *base.MigrationContext) sql.Parser {
if context.CreateTableStatement != "" {
return sql.NewCreateTableParser(context.CreateTableStatement)
}
return sql.NewAlterTableParser(context.AlterStatement)
}

func NewMigrator(context *base.MigrationContext, appVersion string) *Migrator {
migrator := &Migrator{
appVersion: appVersion,
hooksExecutor: NewHooksExecutor(context),
migrationContext: context,
parser: sql.NewAlterTableParser(),
parser: getParser(context),
ghostTableMigrated: make(chan bool),
firstThrottlingCollected: make(chan bool, 3),
rowCopyComplete: make(chan error),
allEventsUpToLockProcessed: make(chan string),

copyRowsQueue: make(chan tableWriteFunc),
applyEventsQueue: make(chan *applyEventStruct, base.MaxEventsBatchSize),
handledChangelogStates: make(map[string]bool),
finishedMigrating: 0,
copyRowsQueue: make(chan tableWriteFunc),
applyEventsQueue: make(chan *applyEventStruct, base.MaxEventsBatchSize),
handledChangelogStates: make(map[string]bool),
finishedMigrating: 0,
}
return migrator
}
Expand Down Expand Up @@ -258,7 +265,14 @@ func (this *Migrator) listenOnPanicAbort() {
this.migrationContext.Log.Fatale(err)
}

// validateAlterStatement validates the `alter` statement meets criteria.
func (this *Migrator) validateStatement() error {
if this.parser.Type() == sql.ParserTypeAlterTable {
return this.validateAlterStatement()
}
return this.validateCreateTableStatement()
}

// validateAlterStatement validates that the `alter` statement meets criteria.
// At this time this means:
// - column renames are approved
// - no table rename allowed
Expand All @@ -277,6 +291,23 @@ func (this *Migrator) validateAlterStatement() (err error) {
return nil
}

// validateCreateTableStatement validates that the `create table` statement meets criteria.
// At this time this means:
// - no table rename allowed
// - no foreign keys
func (this *Migrator) validateCreateTableStatement() (err error) {
if this.migrationContext.OriginalTableName != this.parser.GetExplicitTable() {
return ErrMigratorUnsupportedRenameCreateTable
}
if this.parser.HasForeignKeys() {
if this.migrationContext.DiscardForeignKeys {
return errors.New("CREATE TABLE includes FOREIGN KEYS but --discard-foreign-keys was set. You should instead remove the foreign keys from the table definition")
}
return errors.New("CREATE TABLE statement seems to FOREIGN KEYS on the table. Child-side foreign keys are not supported. Bailing out")
}
return nil
}

func (this *Migrator) countTableRows() (err error) {
if !this.migrationContext.CountTableRows {
// Not counting; we stay with an estimate
Expand Down Expand Up @@ -336,15 +367,15 @@ func (this *Migrator) Migrate() (err error) {
if err := this.hooksExecutor.onStartup(); err != nil {
return err
}
if err := this.parser.ParseAlterStatement(this.migrationContext.AlterStatement); err != nil {
if err := this.parser.ParseStatement(); err != nil {
return err
}
if err := this.validateAlterStatement(); err != nil {
if err := this.validateStatement(); err != nil {
return err
}

// After this point, we'll need to teardown anything that's been started
// so we don't leave things hanging around
// so we don't leave things hanging around
defer this.teardown()

if err := this.initiateInspector(); err != nil {
Expand Down Expand Up @@ -1139,18 +1170,20 @@ func (this *Migrator) initiateApplier() error {
return err
}

if err := this.applier.AlterGhost(); err != nil {
this.migrationContext.Log.Errorf("Unable to ALTER ghost table, see further error details. Bailing out")
return err
}

if this.migrationContext.OriginalTableAutoIncrement > 0 && !this.parser.IsAutoIncrementDefined() {
// Original table has AUTO_INCREMENT value and the -alter statement does not indicate any override,
// so we should copy AUTO_INCREMENT value onto our ghost table.
if err := this.applier.AlterGhostAutoIncrement(); err != nil {
this.migrationContext.Log.Errorf("Unable to ALTER ghost table AUTO_INCREMENT value, see further error details. Bailing out")
if this.parser.Type() == sql.ParserTypeAlterTable {
if err := this.applier.AlterGhost(); err != nil {
this.migrationContext.Log.Errorf("Unable to ALTER ghost table, see further error details. Bailing out")
return err
}

if this.migrationContext.OriginalTableAutoIncrement > 0 && !this.parser.IsAutoIncrementDefined() {
// Original table has AUTO_INCREMENT value and the -alter statement does not indicate any override,
// so we should copy AUTO_INCREMENT value onto our ghost table.
if err := this.applier.AlterGhostAutoIncrement(); err != nil {
this.migrationContext.Log.Errorf("Unable to ALTER ghost table AUTO_INCREMENT value, see further error details. Bailing out")
return err
}
}
}
this.applier.WriteChangelogState(string(GhostTableMigrated))
go this.applier.InitiateHeartbeat()
Expand Down
15 changes: 10 additions & 5 deletions go/logic/migrator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -114,17 +114,19 @@ func TestMigratorOnChangelogEvent(t *testing.T) {
func TestMigratorValidateStatement(t *testing.T) {
t.Run("add-column", func(t *testing.T) {
migrationContext := base.NewMigrationContext()
migrationContext.AlterStatement = `ALTER TABLE test ADD test_new VARCHAR(64) NOT NULL`
migrator := NewMigrator(migrationContext, "1.2.3")
tests.S(t).ExpectNil(migrator.parser.ParseAlterStatement(`ALTER TABLE test ADD test_new VARCHAR(64) NOT NULL`))
tests.S(t).ExpectNil(migrator.parser.ParseStatement())

tests.S(t).ExpectNil(migrator.validateAlterStatement())
tests.S(t).ExpectEquals(len(migrator.migrationContext.DroppedColumnsMap), 0)
})

t.Run("drop-column", func(t *testing.T) {
migrationContext := base.NewMigrationContext()
migrationContext.AlterStatement = `ALTER TABLE test DROP abc`
migrator := NewMigrator(migrationContext, "1.2.3")
tests.S(t).ExpectNil(migrator.parser.ParseAlterStatement(`ALTER TABLE test DROP abc`))
tests.S(t).ExpectNil(migrator.parser.ParseStatement())

tests.S(t).ExpectNil(migrator.validateAlterStatement())
tests.S(t).ExpectEquals(len(migrator.migrationContext.DroppedColumnsMap), 1)
Expand All @@ -134,8 +136,9 @@ func TestMigratorValidateStatement(t *testing.T) {

t.Run("rename-column", func(t *testing.T) {
migrationContext := base.NewMigrationContext()
migrationContext.AlterStatement = `ALTER TABLE test CHANGE test123 test1234 bigint unsigned`
migrator := NewMigrator(migrationContext, "1.2.3")
tests.S(t).ExpectNil(migrator.parser.ParseAlterStatement(`ALTER TABLE test CHANGE test123 test1234 bigint unsigned`))
tests.S(t).ExpectNil(migrator.parser.ParseStatement())

err := migrator.validateAlterStatement()
tests.S(t).ExpectNotNil(err)
Expand All @@ -145,18 +148,20 @@ func TestMigratorValidateStatement(t *testing.T) {

t.Run("rename-column-approved", func(t *testing.T) {
migrationContext := base.NewMigrationContext()
migrationContext.AlterStatement = `ALTER TABLE test CHANGE test123 test1234 bigint unsigned`
migrator := NewMigrator(migrationContext, "1.2.3")
migrator.migrationContext.ApproveRenamedColumns = true
tests.S(t).ExpectNil(migrator.parser.ParseAlterStatement(`ALTER TABLE test CHANGE test123 test1234 bigint unsigned`))
tests.S(t).ExpectNil(migrator.parser.ParseStatement())

tests.S(t).ExpectNil(migrator.validateAlterStatement())
tests.S(t).ExpectEquals(len(migrator.migrationContext.DroppedColumnsMap), 0)
})

t.Run("rename-table", func(t *testing.T) {
migrationContext := base.NewMigrationContext()
migrationContext.AlterStatement = `ALTER TABLE test RENAME TO test_new`
migrator := NewMigrator(migrationContext, "1.2.3")
tests.S(t).ExpectNil(migrator.parser.ParseAlterStatement(`ALTER TABLE test RENAME TO test_new`))
tests.S(t).ExpectNil(migrator.parser.ParseStatement())

err := migrator.validateAlterStatement()
tests.S(t).ExpectNotNil(err)
Expand Down
2 changes: 1 addition & 1 deletion go/sql/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ const (
)

// EscapeName will escape a db/table/column/... name by wrapping with backticks.
// It is not fool proof. I'm just trying to do the right thing here, not solving
// It is not full proof. I'm just trying to do the right thing here, not solving
// SQL injection issues, which should be irrelevant for this tool.
func EscapeName(name string) string {
if unquoted, err := strconv.Unquote(name); err == nil {
Expand Down
Loading