From acd78b392f44f166ddb9b6c36ca4d099ea8b94fc Mon Sep 17 00:00:00 2001 From: Shlomi Noach Date: Sun, 23 Apr 2017 08:00:28 +0300 Subject: [PATCH 1/5] adding tests for dropping-then-adding a column --- localtests/drop-null-add-not-null/create.sql | 30 ++++++++++++++++++++ localtests/drop-null-add-not-null/extra_args | 1 + 2 files changed, 31 insertions(+) create mode 100644 localtests/drop-null-add-not-null/create.sql create mode 100644 localtests/drop-null-add-not-null/extra_args diff --git a/localtests/drop-null-add-not-null/create.sql b/localtests/drop-null-add-not-null/create.sql new file mode 100644 index 0000000..cf54559 --- /dev/null +++ b/localtests/drop-null-add-not-null/create.sql @@ -0,0 +1,30 @@ +drop table if exists gh_ost_test; +create table gh_ost_test ( + id int auto_increment, + c1 int null, + c2 int not null, + primary key (id) +) auto_increment=1; + +insert into gh_ost_test values (null, null, 17); +insert into gh_ost_test values (null, null, 19); + +drop event if exists gh_ost_test; +delimiter ;; +create event gh_ost_test + on schedule every 1 second + starts current_timestamp + ends current_timestamp + interval 60 second + on completion not preserve + enable + do +begin + insert ignore into gh_ost_test values (101, 11, 23); + insert ignore into gh_ost_test values (102, 13, 23); + insert into gh_ost_test values (null, 17, 23); + insert into gh_ost_test values (null, null, 29); + set @last_insert_id := last_insert_id(); + -- update gh_ost_test set c2=c2+@last_insert_id where id=@last_insert_id order by id desc limit 1; + delete from gh_ost_test where id=1; + delete from gh_ost_test where c1=13; -- id=2 +end ;; diff --git a/localtests/drop-null-add-not-null/extra_args b/localtests/drop-null-add-not-null/extra_args new file mode 100644 index 0000000..948e978 --- /dev/null +++ b/localtests/drop-null-add-not-null/extra_args @@ -0,0 +1 @@ +--alter="drop column c1, add column c1 int not null after id" From 85c498511e4132c61b9188e650ce43b45b33f1e5 Mon Sep 17 00:00:00 2001 From: Shlomi Noach Date: Sun, 23 Apr 2017 08:23:56 +0300 Subject: [PATCH 2/5] parser recognizes DROP COLUMN tokens --- go/sql/parser.go | 30 +++++++++++++++++++++++++----- go/sql/parser_test.go | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 5 deletions(-) diff --git a/go/sql/parser.go b/go/sql/parser.go index b81b31c..7875fcc 100644 --- a/go/sql/parser.go +++ b/go/sql/parser.go @@ -14,15 +14,18 @@ import ( var ( sanitizeQuotesRegexp = regexp.MustCompile("('[^']*')") renameColumnRegexp = regexp.MustCompile(`(?i)\bchange\s+(column\s+|)([\S]+)\s+([\S]+)\s+`) + dropColumnRegexp = regexp.MustCompile(`(?i)\bdrop\s+(column\s+|)([\S]+)$`) ) type Parser struct { columnRenameMap map[string]string + droppedColumns map[string]bool } func NewParser() *Parser { return &Parser{ columnRenameMap: make(map[string]string), + droppedColumns: make(map[string]bool), } } @@ -59,10 +62,9 @@ func (this *Parser) sanitizeQuotesFromAlterStatement(alterStatement string) (str return strippedStatement } -func (this *Parser) ParseAlterStatement(alterStatement string) (err error) { - alterTokens, _ := this.tokenizeAlterStatement(alterStatement) - for _, alterToken := range alterTokens { - alterToken = this.sanitizeQuotesFromAlterStatement(alterToken) +func (this *Parser) parseAlterToken(alterToken string) (err error) { + { + // rename allStringSubmatch := renameColumnRegexp.FindAllStringSubmatch(alterToken, -1) for _, submatch := range allStringSubmatch { if unquoted, err := strconv.Unquote(submatch[2]); err == nil { @@ -71,10 +73,28 @@ func (this *Parser) ParseAlterStatement(alterStatement string) (err error) { if unquoted, err := strconv.Unquote(submatch[3]); err == nil { submatch[3] = unquoted } - this.columnRenameMap[submatch[2]] = submatch[3] } } + { + // drop + allStringSubmatch := dropColumnRegexp.FindAllStringSubmatch(alterToken, -1) + for _, submatch := range allStringSubmatch { + if unquoted, err := strconv.Unquote(submatch[2]); err == nil { + submatch[2] = unquoted + } + this.droppedColumns[submatch[2]] = true + } + } + return nil +} + +func (this *Parser) ParseAlterStatement(alterStatement string) (err error) { + alterTokens, _ := this.tokenizeAlterStatement(alterStatement) + for _, alterToken := range alterTokens { + alterToken = this.sanitizeQuotesFromAlterStatement(alterToken) + this.parseAlterToken(alterToken) + } return nil } diff --git a/go/sql/parser_test.go b/go/sql/parser_test.go index 8039f5f..3e1d845 100644 --- a/go/sql/parser_test.go +++ b/go/sql/parser_test.go @@ -120,3 +120,42 @@ func TestSanitizeQuotesFromAlterStatement(t *testing.T) { test.S(t).ExpectEquals(strippedStatement, "change column i int ''") } } + +func TestParseAlterStatementDroppedColumns(t *testing.T) { + + { + parser := NewParser() + statement := "drop column b" + err := parser.ParseAlterStatement(statement) + test.S(t).ExpectNil(err) + test.S(t).ExpectEquals(len(parser.droppedColumns), 1) + test.S(t).ExpectTrue(parser.droppedColumns["b"]) + } + { + parser := NewParser() + statement := "drop column b, drop key c_idx, drop column `d`" + err := parser.ParseAlterStatement(statement) + test.S(t).ExpectNil(err) + test.S(t).ExpectEquals(len(parser.droppedColumns), 2) + test.S(t).ExpectTrue(parser.droppedColumns["b"]) + test.S(t).ExpectTrue(parser.droppedColumns["d"]) + } + { + parser := NewParser() + statement := "drop column b, drop key c_idx, drop column `d`, drop `e`, drop primary key, drop foreign key fk_1" + err := parser.ParseAlterStatement(statement) + test.S(t).ExpectNil(err) + test.S(t).ExpectEquals(len(parser.droppedColumns), 3) + test.S(t).ExpectTrue(parser.droppedColumns["b"]) + test.S(t).ExpectTrue(parser.droppedColumns["d"]) + test.S(t).ExpectTrue(parser.droppedColumns["e"]) + } + { + parser := NewParser() + statement := "drop column b, drop bad statement, add column i int" + err := parser.ParseAlterStatement(statement) + test.S(t).ExpectNil(err) + test.S(t).ExpectEquals(len(parser.droppedColumns), 1) + test.S(t).ExpectTrue(parser.droppedColumns["b"]) + } +} From b0469b95b58983d444b7a9966844417e6a6e1199 Mon Sep 17 00:00:00 2001 From: Shlomi Noach Date: Sun, 23 Apr 2017 08:37:48 +0300 Subject: [PATCH 3/5] further fixes to the test case --- localtests/drop-null-add-not-null/extra_args | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/localtests/drop-null-add-not-null/extra_args b/localtests/drop-null-add-not-null/extra_args index 948e978..8219c7d 100644 --- a/localtests/drop-null-add-not-null/extra_args +++ b/localtests/drop-null-add-not-null/extra_args @@ -1 +1 @@ ---alter="drop column c1, add column c1 int not null after id" +--alter="drop column c1, add column c1 int not null default 47" From 8a0f1413eb06d2d08c4c2f55fa302a1c06c05ee7 Mon Sep 17 00:00:00 2001 From: Shlomi Noach Date: Sun, 23 Apr 2017 08:38:35 +0300 Subject: [PATCH 4/5] dropped columns are not 'shared' and no data copy attempted for such columns --- go/base/context.go | 1 + go/logic/inspect.go | 7 +++++++ go/logic/migrator.go | 1 + go/sql/parser.go | 4 ++++ 4 files changed, 13 insertions(+) diff --git a/go/base/context.go b/go/base/context.go index 5ab6d2a..f5a7bca 100644 --- a/go/base/context.go +++ b/go/base/context.go @@ -181,6 +181,7 @@ type MigrationContext struct { UniqueKey *sql.UniqueKey SharedColumns *sql.ColumnList ColumnRenameMap map[string]string + DroppedColumnsMap map[string]bool MappedSharedColumns *sql.ColumnList MigrationRangeMinValues *sql.ColumnValues MigrationRangeMaxValues *sql.ColumnValues diff --git a/go/logic/inspect.go b/go/logic/inspect.go index 1d30cb5..181ed0b 100644 --- a/go/logic/inspect.go +++ b/go/logic/inspect.go @@ -662,7 +662,14 @@ func (this *Inspector) getSharedColumns(originalColumns, ghostColumns *sql.Colum } sharedColumnNames := []string{} for _, originalColumn := range originalColumns.Names() { + isSharedColumn := false if columnsInGhost[originalColumn] || columnsInGhost[columnRenameMap[originalColumn]] { + isSharedColumn = true + } + if this.migrationContext.DroppedColumnsMap[originalColumn] { + isSharedColumn = false + } + if isSharedColumn { sharedColumnNames = append(sharedColumnNames, originalColumn) } } diff --git a/go/logic/migrator.go b/go/logic/migrator.go index 59432a3..092039e 100644 --- a/go/logic/migrator.go +++ b/go/logic/migrator.go @@ -248,6 +248,7 @@ func (this *Migrator) validateStatement() (err error) { } log.Infof("Alter statement has column(s) renamed. gh-ost finds the following renames: %v; --approve-renamed-columns is given and so migration proceeds.", this.parser.GetNonTrivialRenames()) } + this.migrationContext.DroppedColumnsMap = this.parser.DroppedColumnsMap() return nil } diff --git a/go/sql/parser.go b/go/sql/parser.go index 7875fcc..7114e10 100644 --- a/go/sql/parser.go +++ b/go/sql/parser.go @@ -111,3 +111,7 @@ func (this *Parser) GetNonTrivialRenames() map[string]string { func (this *Parser) HasNonTrivialRenames() bool { return len(this.GetNonTrivialRenames()) > 0 } + +func (this *Parser) DroppedColumnsMap() map[string]bool { + return this.droppedColumns +} From 7f62efba910b082331a2915da4aafc67737c0aad Mon Sep 17 00:00:00 2001 From: Shlomi Noach Date: Sun, 23 Apr 2017 08:48:06 +0300 Subject: [PATCH 5/5] tests to only check non dropped columns --- localtests/drop-null-add-not-null/ghost_columns | 1 + localtests/drop-null-add-not-null/orig_columns | 1 + 2 files changed, 2 insertions(+) create mode 100644 localtests/drop-null-add-not-null/ghost_columns create mode 100644 localtests/drop-null-add-not-null/orig_columns diff --git a/localtests/drop-null-add-not-null/ghost_columns b/localtests/drop-null-add-not-null/ghost_columns new file mode 100644 index 0000000..16f9ec0 --- /dev/null +++ b/localtests/drop-null-add-not-null/ghost_columns @@ -0,0 +1 @@ +c2 diff --git a/localtests/drop-null-add-not-null/orig_columns b/localtests/drop-null-add-not-null/orig_columns new file mode 100644 index 0000000..16f9ec0 --- /dev/null +++ b/localtests/drop-null-add-not-null/orig_columns @@ -0,0 +1 @@ +c2