From 1376f0af23780a80885df49ea7d66ef1298f8fcb Mon Sep 17 00:00:00 2001 From: Shlomi Noach Date: Mon, 22 Aug 2016 08:49:27 +0200 Subject: [PATCH] fixed UPDATE dml on renamed column --- go/logic/applier.go | 7 +++---- go/logic/migrator.go | 2 +- go/sql/builder.go | 15 ++++++++------- go/sql/builder_test.go | 33 +++++++++++++++++++++++++-------- 4 files changed, 37 insertions(+), 20 deletions(-) diff --git a/go/logic/applier.go b/go/logic/applier.go index 10da785..d240abb 100644 --- a/go/logic/applier.go +++ b/go/logic/applier.go @@ -107,7 +107,7 @@ func (this *Applier) ValidateOrDropExistingTables() error { } } if this.tableExists(this.migrationContext.GetGhostTableName()) { - return fmt.Errorf("Table %s already exists. Panicking. Use --initially-drop-ghost-table to force dropping it", sql.EscapeName(this.migrationContext.GetGhostTableName())) + return fmt.Errorf("Table %s already exists. Panicking. Use --initially-drop-ghost-table to force dropping it, though I really prefer that you drop it or rename it away", sql.EscapeName(this.migrationContext.GetGhostTableName())) } if this.migrationContext.InitiallyDropOldTable { if err := this.DropOldTable(); err != nil { @@ -115,7 +115,7 @@ func (this *Applier) ValidateOrDropExistingTables() error { } } if this.tableExists(this.migrationContext.GetOldTableName()) { - return fmt.Errorf("Table %s already exists. Panicking. Use --initially-drop-old-table to force dropping it", sql.EscapeName(this.migrationContext.GetOldTableName())) + return fmt.Errorf("Table %s already exists. Panicking. Use --initially-drop-old-table to force dropping it, though I really prefer that you drop it or rename it away", sql.EscapeName(this.migrationContext.GetOldTableName())) } return nil @@ -837,7 +837,7 @@ func (this *Applier) buildDMLEventQuery(dmlEvent *binlog.BinlogDMLEvent) (query } case binlog.UpdateDML: { - query, sharedArgs, uniqueKeyArgs, err := sql.BuildDMLUpdateQuery(dmlEvent.DatabaseName, this.migrationContext.GetGhostTableName(), this.migrationContext.OriginalTableColumns, this.migrationContext.MappedSharedColumns, &this.migrationContext.UniqueKey.Columns, dmlEvent.NewColumnValues.AbstractValues(), dmlEvent.WhereColumnValues.AbstractValues()) + query, sharedArgs, uniqueKeyArgs, err := sql.BuildDMLUpdateQuery(dmlEvent.DatabaseName, this.migrationContext.GetGhostTableName(), this.migrationContext.OriginalTableColumns, this.migrationContext.SharedColumns, this.migrationContext.MappedSharedColumns, &this.migrationContext.UniqueKey.Columns, dmlEvent.NewColumnValues.AbstractValues(), dmlEvent.WhereColumnValues.AbstractValues()) args = append(args, sharedArgs...) args = append(args, uniqueKeyArgs...) return query, args, 0, err @@ -853,7 +853,6 @@ func (this *Applier) ApplyDMLEventQuery(dmlEvent *binlog.BinlogDMLEvent) error { if err != nil { return err } - // TODO The below is in preparation for transactional writes on the ghost tables. // Such writes would be, for example: // - prepended with sql_mode setup diff --git a/go/logic/migrator.go b/go/logic/migrator.go index 600381e..998f386 100644 --- a/go/logic/migrator.go +++ b/go/logic/migrator.go @@ -354,7 +354,7 @@ func (this *Migrator) validateStatement() (err error) { if this.parser.HasNonTrivialRenames() && !this.migrationContext.SkipRenamedColumns { this.migrationContext.ColumnRenameMap = this.parser.GetNonTrivialRenames() if !this.migrationContext.ApproveRenamedColumns { - return fmt.Errorf("Alter statement has column(s) renamed. gh-ost suspects the following renames: %v; but to proceed you must approve via `--approve-renamed-columns` (or you can skip renamed columns via `--skip-renamed-columns`)", this.parser.GetNonTrivialRenames()) + return fmt.Errorf("gh-ost believes the ALTER statement renames columns, as follows: %v; as precation, you are asked to confirm gh-ost is correct, and provide with `--approve-renamed-columns`, and we're all happy. Or you can skip renamed columns via `--skip-renamed-columns`, in which case column data may be lost", this.parser.GetNonTrivialRenames()) } 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()) } diff --git a/go/sql/builder.go b/go/sql/builder.go index 7c670a9..dedd323 100644 --- a/go/sql/builder.go +++ b/go/sql/builder.go @@ -393,7 +393,7 @@ func BuildDMLInsertQuery(databaseName, tableName string, tableColumns, sharedCol return result, sharedArgs, nil } -func BuildDMLUpdateQuery(databaseName, tableName string, tableColumns, sharedColumns, uniqueKeyColumns *ColumnList, valueArgs, whereArgs []interface{}) (result string, sharedArgs, uniqueKeyArgs []interface{}, err error) { +func BuildDMLUpdateQuery(databaseName, tableName string, tableColumns, sharedColumns, mappedSharedColumns, uniqueKeyColumns *ColumnList, valueArgs, whereArgs []interface{}) (result string, sharedArgs, uniqueKeyArgs []interface{}, err error) { if len(valueArgs) != tableColumns.Len() { return result, sharedArgs, uniqueKeyArgs, fmt.Errorf("value args count differs from table column count in BuildDMLUpdateQuery") } @@ -415,9 +415,10 @@ func BuildDMLUpdateQuery(databaseName, tableName string, tableColumns, sharedCol databaseName = EscapeName(databaseName) tableName = EscapeName(tableName) - for _, column := range sharedColumns.Names { + for i, column := range sharedColumns.Names { + mappedColumn := mappedSharedColumns.Names[i] tableOrdinal := tableColumns.Ordinals[column] - arg := fixArgType(valueArgs[tableOrdinal], sharedColumns.IsUnsigned(column)) + arg := fixArgType(valueArgs[tableOrdinal], mappedSharedColumns.IsUnsigned(mappedColumn)) sharedArgs = append(sharedArgs, arg) } @@ -427,11 +428,11 @@ func BuildDMLUpdateQuery(databaseName, tableName string, tableColumns, sharedCol uniqueKeyArgs = append(uniqueKeyArgs, arg) } - sharedColumnNames := duplicateNames(sharedColumns.Names) - for i := range sharedColumnNames { - sharedColumnNames[i] = EscapeName(sharedColumnNames[i]) + mappedSharedColumnNames := duplicateNames(mappedSharedColumns.Names) + for i := range mappedSharedColumnNames { + mappedSharedColumnNames[i] = EscapeName(mappedSharedColumnNames[i]) } - setClause, err := BuildSetPreparedClause(sharedColumnNames) + setClause, err := BuildSetPreparedClause(mappedSharedColumnNames) equalsComparison, err := BuildEqualsPreparedComparison(uniqueKeyColumns.Names) result = fmt.Sprintf(` diff --git a/go/sql/builder_test.go b/go/sql/builder_test.go index db2617e..565954e 100644 --- a/go/sql/builder_test.go +++ b/go/sql/builder_test.go @@ -544,7 +544,7 @@ func TestBuildDMLUpdateQuery(t *testing.T) { { sharedColumns := NewColumnList([]string{"id", "name", "position", "age"}) uniqueKeyColumns := NewColumnList([]string{"position"}) - query, sharedArgs, uniqueKeyArgs, err := BuildDMLUpdateQuery(databaseName, tableName, tableColumns, sharedColumns, uniqueKeyColumns, valueArgs, whereArgs) + query, sharedArgs, uniqueKeyArgs, err := BuildDMLUpdateQuery(databaseName, tableName, tableColumns, sharedColumns, sharedColumns, uniqueKeyColumns, valueArgs, whereArgs) test.S(t).ExpectNil(err) expected := ` update /* gh-ost mydb.tbl */ @@ -560,7 +560,7 @@ func TestBuildDMLUpdateQuery(t *testing.T) { { sharedColumns := NewColumnList([]string{"id", "name", "position", "age"}) uniqueKeyColumns := NewColumnList([]string{"position", "name"}) - query, sharedArgs, uniqueKeyArgs, err := BuildDMLUpdateQuery(databaseName, tableName, tableColumns, sharedColumns, uniqueKeyColumns, valueArgs, whereArgs) + query, sharedArgs, uniqueKeyArgs, err := BuildDMLUpdateQuery(databaseName, tableName, tableColumns, sharedColumns, sharedColumns, uniqueKeyColumns, valueArgs, whereArgs) test.S(t).ExpectNil(err) expected := ` update /* gh-ost mydb.tbl */ @@ -576,7 +576,7 @@ func TestBuildDMLUpdateQuery(t *testing.T) { { sharedColumns := NewColumnList([]string{"id", "name", "position", "age"}) uniqueKeyColumns := NewColumnList([]string{"age"}) - query, sharedArgs, uniqueKeyArgs, err := BuildDMLUpdateQuery(databaseName, tableName, tableColumns, sharedColumns, uniqueKeyColumns, valueArgs, whereArgs) + query, sharedArgs, uniqueKeyArgs, err := BuildDMLUpdateQuery(databaseName, tableName, tableColumns, sharedColumns, sharedColumns, uniqueKeyColumns, valueArgs, whereArgs) test.S(t).ExpectNil(err) expected := ` update /* gh-ost mydb.tbl */ @@ -592,7 +592,7 @@ func TestBuildDMLUpdateQuery(t *testing.T) { { sharedColumns := NewColumnList([]string{"id", "name", "position", "age"}) uniqueKeyColumns := NewColumnList([]string{"age", "position", "id", "name"}) - query, sharedArgs, uniqueKeyArgs, err := BuildDMLUpdateQuery(databaseName, tableName, tableColumns, sharedColumns, uniqueKeyColumns, valueArgs, whereArgs) + query, sharedArgs, uniqueKeyArgs, err := BuildDMLUpdateQuery(databaseName, tableName, tableColumns, sharedColumns, sharedColumns, uniqueKeyColumns, valueArgs, whereArgs) test.S(t).ExpectNil(err) expected := ` update /* gh-ost mydb.tbl */ @@ -608,15 +608,32 @@ func TestBuildDMLUpdateQuery(t *testing.T) { { sharedColumns := NewColumnList([]string{"id", "name", "position", "age"}) uniqueKeyColumns := NewColumnList([]string{"age", "surprise"}) - _, _, _, err := BuildDMLUpdateQuery(databaseName, tableName, tableColumns, sharedColumns, uniqueKeyColumns, valueArgs, whereArgs) + _, _, _, err := BuildDMLUpdateQuery(databaseName, tableName, tableColumns, sharedColumns, sharedColumns, uniqueKeyColumns, valueArgs, whereArgs) test.S(t).ExpectNotNil(err) } { sharedColumns := NewColumnList([]string{"id", "name", "position", "age"}) uniqueKeyColumns := NewColumnList([]string{}) - _, _, _, err := BuildDMLUpdateQuery(databaseName, tableName, tableColumns, sharedColumns, uniqueKeyColumns, valueArgs, whereArgs) + _, _, _, err := BuildDMLUpdateQuery(databaseName, tableName, tableColumns, sharedColumns, sharedColumns, uniqueKeyColumns, valueArgs, whereArgs) test.S(t).ExpectNotNil(err) } + { + sharedColumns := NewColumnList([]string{"id", "name", "position", "age"}) + mappedColumns := NewColumnList([]string{"id", "name", "role", "age"}) + uniqueKeyColumns := NewColumnList([]string{"id"}) + query, sharedArgs, uniqueKeyArgs, err := BuildDMLUpdateQuery(databaseName, tableName, tableColumns, sharedColumns, mappedColumns, uniqueKeyColumns, valueArgs, whereArgs) + test.S(t).ExpectNil(err) + expected := ` + update /* gh-ost mydb.tbl */ + mydb.tbl + set id=?, name=?, role=?, age=? + where + ((id = ?)) + ` + test.S(t).ExpectEquals(normalizeQuery(query), normalizeQuery(expected)) + test.S(t).ExpectTrue(reflect.DeepEqual(sharedArgs, []interface{}{3, "testname", 17, 23})) + test.S(t).ExpectTrue(reflect.DeepEqual(uniqueKeyArgs, []interface{}{3})) + } } func TestBuildDMLUpdateQuerySignedUnsigned(t *testing.T) { @@ -629,7 +646,7 @@ func TestBuildDMLUpdateQuerySignedUnsigned(t *testing.T) { uniqueKeyColumns := NewColumnList([]string{"position"}) { // test signed - query, sharedArgs, uniqueKeyArgs, err := BuildDMLUpdateQuery(databaseName, tableName, tableColumns, sharedColumns, uniqueKeyColumns, valueArgs, whereArgs) + query, sharedArgs, uniqueKeyArgs, err := BuildDMLUpdateQuery(databaseName, tableName, tableColumns, sharedColumns, sharedColumns, uniqueKeyColumns, valueArgs, whereArgs) test.S(t).ExpectNil(err) expected := ` update /* gh-ost mydb.tbl */ @@ -646,7 +663,7 @@ func TestBuildDMLUpdateQuerySignedUnsigned(t *testing.T) { // test unsigned sharedColumns.SetUnsigned("age") uniqueKeyColumns.SetUnsigned("position") - query, sharedArgs, uniqueKeyArgs, err := BuildDMLUpdateQuery(databaseName, tableName, tableColumns, sharedColumns, uniqueKeyColumns, valueArgs, whereArgs) + query, sharedArgs, uniqueKeyArgs, err := BuildDMLUpdateQuery(databaseName, tableName, tableColumns, sharedColumns, sharedColumns, uniqueKeyColumns, valueArgs, whereArgs) test.S(t).ExpectNil(err) expected := ` update /* gh-ost mydb.tbl */