diff --git a/go/logic/applier.go b/go/logic/applier.go index 68d11171b..e77cd4c17 100644 --- a/go/logic/applier.go +++ b/go/logic/applier.go @@ -1470,10 +1470,9 @@ func (this *Applier) buildDMLEventQuery(dmlEvent *binlog.BinlogDMLEvent) []*dmlB results = append(results, this.buildDMLEventQuery(dmlEvent)...) return results } - query, sharedArgs, uniqueKeyArgs, err := this.dmlUpdateQueryBuilder.BuildQuery(dmlEvent.NewColumnValues.AbstractValues(), dmlEvent.WhereColumnValues.AbstractValues()) + query, updateArgs, err := this.dmlUpdateQueryBuilder.BuildQuery(dmlEvent.NewColumnValues.AbstractValues(), dmlEvent.WhereColumnValues.AbstractValues()) args := sqlutils.Args() - args = append(args, sharedArgs...) - args = append(args, uniqueKeyArgs...) + args = append(args, updateArgs...) return []*dmlBuildResult{newDmlBuildResult(query, args, 0, err)} } } diff --git a/go/sql/builder.go b/go/sql/builder.go index 61dd9706f..0bb2d5e6a 100644 --- a/go/sql/builder.go +++ b/go/sql/builder.go @@ -169,11 +169,11 @@ func (b *CheckpointInsertQueryBuilder) BuildQuery(uniqueKeyArgs []interface{}) ( } convertedArgs := make([]interface{}, 0, 2*b.uniqueKeyColumns.Len()) for i, column := range b.uniqueKeyColumns.Columns() { - minArg := column.convertArg(uniqueKeyArgs[i], true) + minArg := column.convertArg(uniqueKeyArgs[i]) convertedArgs = append(convertedArgs, minArg) } for i, column := range b.uniqueKeyColumns.Columns() { - minArg := column.convertArg(uniqueKeyArgs[i+b.uniqueKeyColumns.Len()], true) + minArg := column.convertArg(uniqueKeyArgs[i+b.uniqueKeyColumns.Len()]) convertedArgs = append(convertedArgs, minArg) } return b.preparedStatement, convertedArgs, nil @@ -533,7 +533,7 @@ func (b *DMLDeleteQueryBuilder) BuildQuery(args []interface{}) (string, []interf uniqueKeyArgs := make([]interface{}, 0, b.uniqueKeyColumns.Len()) for _, column := range b.uniqueKeyColumns.Columns() { tableOrdinal := b.tableColumns.Ordinals[column.Name] - arg := column.convertArg(args[tableOrdinal], true) + arg := column.convertArg(args[tableOrdinal]) uniqueKeyArgs = append(uniqueKeyArgs, arg) } return b.preparedStatement, uniqueKeyArgs, nil @@ -595,7 +595,7 @@ func (b *DMLInsertQueryBuilder) BuildQuery(args []interface{}) (string, []interf sharedArgs := make([]interface{}, 0, b.sharedColumns.Len()) for _, column := range b.sharedColumns.Columns() { tableOrdinal := b.tableColumns.Ordinals[column.Name] - arg := column.convertArg(args[tableOrdinal], false) + arg := column.convertArg(args[tableOrdinal]) sharedArgs = append(sharedArgs, arg) } return b.preparedStatement, sharedArgs, nil @@ -661,20 +661,18 @@ func NewDMLUpdateQueryBuilder(databaseName, tableName string, tableColumns, shar // BuildQuery builds the arguments array for a DML event UPDATE query. // It returns the query string, the shared arguments array, and the unique key arguments array. -func (b *DMLUpdateQueryBuilder) BuildQuery(valueArgs, whereArgs []interface{}) (string, []interface{}, []interface{}, error) { - sharedArgs := make([]interface{}, 0, b.sharedColumns.Len()) +func (b *DMLUpdateQueryBuilder) BuildQuery(valueArgs, whereArgs []interface{}) (string, []interface{}, error) { + args := make([]interface{}, 0, b.sharedColumns.Len()+b.uniqueKeyColumns.Len()) for _, column := range b.sharedColumns.Columns() { tableOrdinal := b.tableColumns.Ordinals[column.Name] - arg := column.convertArg(valueArgs[tableOrdinal], false) - sharedArgs = append(sharedArgs, arg) + arg := column.convertArg(valueArgs[tableOrdinal]) + args = append(args, arg) } - - uniqueKeyArgs := make([]interface{}, 0, b.uniqueKeyColumns.Len()) for _, column := range b.uniqueKeyColumns.Columns() { tableOrdinal := b.tableColumns.Ordinals[column.Name] - arg := column.convertArg(whereArgs[tableOrdinal], true) - uniqueKeyArgs = append(uniqueKeyArgs, arg) + arg := column.convertArg(whereArgs[tableOrdinal]) + args = append(args, arg) } - return b.preparedStatement, sharedArgs, uniqueKeyArgs, nil + return b.preparedStatement, args, nil } diff --git a/go/sql/builder_test.go b/go/sql/builder_test.go index 840d85c96..15c21d5fa 100644 --- a/go/sql/builder_test.go +++ b/go/sql/builder_test.go @@ -647,7 +647,7 @@ func TestBuildDMLUpdateQuery(t *testing.T) { uniqueKeyColumns := NewColumnList([]string{"position"}) builder, err := NewDMLUpdateQueryBuilder(databaseName, tableName, tableColumns, sharedColumns, sharedColumns, uniqueKeyColumns) require.NoError(t, err) - query, sharedArgs, uniqueKeyArgs, err := builder.BuildQuery(valueArgs, whereArgs) + query, updateArgs, err := builder.BuildQuery(valueArgs, whereArgs) require.NoError(t, err) expected := ` update /* gh-ost mydb.tbl */ @@ -657,15 +657,14 @@ func TestBuildDMLUpdateQuery(t *testing.T) { ((position = ?)) ` require.Equal(t, normalizeQuery(expected), normalizeQuery(query)) - require.Equal(t, []interface{}{3, "testname", 17, 23}, sharedArgs) - require.Equal(t, []interface{}{17}, uniqueKeyArgs) + require.Equal(t, []interface{}{3, "testname", 17, 23, 17}, updateArgs) } { sharedColumns := NewColumnList([]string{"id", "name", "position", "age"}) uniqueKeyColumns := NewColumnList([]string{"position", "name"}) builder, err := NewDMLUpdateQueryBuilder(databaseName, tableName, tableColumns, sharedColumns, sharedColumns, uniqueKeyColumns) require.NoError(t, err) - query, sharedArgs, uniqueKeyArgs, err := builder.BuildQuery(valueArgs, whereArgs) + query, updateArgs, err := builder.BuildQuery(valueArgs, whereArgs) require.NoError(t, err) expected := ` update /* gh-ost mydb.tbl */ @@ -675,15 +674,14 @@ func TestBuildDMLUpdateQuery(t *testing.T) { ((position = ?) and (name = ?)) ` require.Equal(t, normalizeQuery(expected), normalizeQuery(query)) - require.Equal(t, []interface{}{3, "testname", 17, 23}, sharedArgs) - require.Equal(t, []interface{}{17, "testname"}, uniqueKeyArgs) + require.Equal(t, []interface{}{3, "testname", 17, 23, 17, "testname"}, updateArgs) } { sharedColumns := NewColumnList([]string{"id", "name", "position", "age"}) uniqueKeyColumns := NewColumnList([]string{"age"}) builder, err := NewDMLUpdateQueryBuilder(databaseName, tableName, tableColumns, sharedColumns, sharedColumns, uniqueKeyColumns) require.NoError(t, err) - query, sharedArgs, uniqueKeyArgs, err := builder.BuildQuery(valueArgs, whereArgs) + query, updateArgs, err := builder.BuildQuery(valueArgs, whereArgs) require.NoError(t, err) expected := ` update /* gh-ost mydb.tbl */ @@ -693,15 +691,14 @@ func TestBuildDMLUpdateQuery(t *testing.T) { ((age = ?)) ` require.Equal(t, normalizeQuery(expected), normalizeQuery(query)) - require.Equal(t, []interface{}{3, "testname", 17, 23}, sharedArgs) - require.Equal(t, []interface{}{56}, uniqueKeyArgs) + require.Equal(t, []interface{}{3, "testname", 17, 23, 56}, updateArgs) } { sharedColumns := NewColumnList([]string{"id", "name", "position", "age"}) uniqueKeyColumns := NewColumnList([]string{"age", "position", "id", "name"}) builder, err := NewDMLUpdateQueryBuilder(databaseName, tableName, tableColumns, sharedColumns, sharedColumns, uniqueKeyColumns) require.NoError(t, err) - query, sharedArgs, uniqueKeyArgs, err := builder.BuildQuery(valueArgs, whereArgs) + query, updateArgs, err := builder.BuildQuery(valueArgs, whereArgs) require.NoError(t, err) expected := ` update /* gh-ost mydb.tbl */ @@ -711,8 +708,7 @@ func TestBuildDMLUpdateQuery(t *testing.T) { ((age = ?) and (position = ?) and (id = ?) and (name = ?)) ` require.Equal(t, normalizeQuery(expected), normalizeQuery(query)) - require.Equal(t, []interface{}{3, "testname", 17, 23}, sharedArgs) - require.Equal(t, []interface{}{56, 17, 3, "testname"}, uniqueKeyArgs) + require.Equal(t, []interface{}{3, "testname", 17, 23, 56, 17, 3, "testname"}, updateArgs) } { sharedColumns := NewColumnList([]string{"id", "name", "position", "age"}) @@ -732,7 +728,7 @@ func TestBuildDMLUpdateQuery(t *testing.T) { uniqueKeyColumns := NewColumnList([]string{"id"}) builder, err := NewDMLUpdateQueryBuilder(databaseName, tableName, tableColumns, sharedColumns, mappedColumns, uniqueKeyColumns) require.NoError(t, err) - query, sharedArgs, uniqueKeyArgs, err := builder.BuildQuery(valueArgs, whereArgs) + query, updateArgs, err := builder.BuildQuery(valueArgs, whereArgs) require.NoError(t, err) expected := ` update /* gh-ost mydb.tbl */ @@ -742,8 +738,7 @@ func TestBuildDMLUpdateQuery(t *testing.T) { ((id = ?)) ` require.Equal(t, normalizeQuery(expected), normalizeQuery(query)) - require.Equal(t, []interface{}{3, "testname", 17, 23}, sharedArgs) - require.Equal(t, []interface{}{3}, uniqueKeyArgs) + require.Equal(t, []interface{}{3, "testname", 17, 23, 3}, updateArgs) } } @@ -759,7 +754,7 @@ func TestBuildDMLUpdateQuerySignedUnsigned(t *testing.T) { require.NoError(t, err) { // test signed - query, sharedArgs, uniqueKeyArgs, err := builder.BuildQuery(valueArgs, whereArgs) + query, updateArgs, err := builder.BuildQuery(valueArgs, whereArgs) require.NoError(t, err) expected := ` update /* gh-ost mydb.tbl */ @@ -769,14 +764,13 @@ func TestBuildDMLUpdateQuerySignedUnsigned(t *testing.T) { ((position = ?)) ` require.Equal(t, normalizeQuery(expected), normalizeQuery(query)) - require.Equal(t, []interface{}{3, "testname", int8(-17), int8(-2)}, sharedArgs) - require.Equal(t, []interface{}{int8(-3)}, uniqueKeyArgs) + require.Equal(t, []interface{}{3, "testname", int8(-17), int8(-2), int8(-3)}, updateArgs) } { // test unsigned sharedColumns.SetUnsigned("age") uniqueKeyColumns.SetUnsigned("position") - query, sharedArgs, uniqueKeyArgs, err := builder.BuildQuery(valueArgs, whereArgs) + query, updateArgs, err := builder.BuildQuery(valueArgs, whereArgs) require.NoError(t, err) expected := ` update /* gh-ost mydb.tbl */ @@ -786,8 +780,7 @@ func TestBuildDMLUpdateQuerySignedUnsigned(t *testing.T) { ((position = ?)) ` require.Equal(t, normalizeQuery(expected), normalizeQuery(query)) - require.Equal(t, []interface{}{3, "testname", int8(-17), uint8(254)}, sharedArgs) - require.Equal(t, []interface{}{uint8(253)}, uniqueKeyArgs) + require.Equal(t, []interface{}{3, "testname", int8(-17), uint8(254), uint8(253)}, updateArgs) } } diff --git a/go/sql/types.go b/go/sql/types.go index a01fb8bff..9a50bc620 100644 --- a/go/sql/types.go +++ b/go/sql/types.go @@ -57,7 +57,7 @@ type Column struct { MySQLType string } -func (this *Column) convertArg(arg interface{}, isUniqueKeyColumn bool) interface{} { +func (this *Column) convertArg(arg interface{}) interface{} { var arg2Bytes []byte if s, ok := arg.(string); ok { arg2Bytes = []byte(s) @@ -77,14 +77,14 @@ func (this *Column) convertArg(arg interface{}, isUniqueKeyColumn bool) interfac } } - if this.Type == BinaryColumnType && isUniqueKeyColumn { + if this.Type == BinaryColumnType { size := len(arg2Bytes) if uint(size) < this.BinaryOctetLength { buf := bytes.NewBuffer(arg2Bytes) for i := uint(0); i < (this.BinaryOctetLength - uint(size)); i++ { buf.Write([]byte{0}) } - arg = buf.String() + arg = buf.Bytes() } } diff --git a/go/sql/types_test.go b/go/sql/types_test.go index 7b808e64f..83c74073a 100644 --- a/go/sql/types_test.go +++ b/go/sql/types_test.go @@ -62,6 +62,56 @@ func TestConvertArgCharsetDecoding(t *testing.T) { } // Should decode []uint8 - str := col.convertArg(latin1Bytes, false) + str := col.convertArg(latin1Bytes) require.Equal(t, "Garçon !", str) } + +func TestConvertArgBinaryColumnPadding(t *testing.T) { + // Test that binary columns are padded with trailing zeros to their declared length. + // This is needed because MySQL's binlog strips trailing 0x00 bytes from binary values. + // See https://github.com/github/gh-ost/issues/909 + + // Simulates a binary(20) column where binlog delivered only 18 bytes + // (trailing zeros were stripped) + truncatedValue := []uint8{ + 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, + 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10, + 0x11, 0x12, // 18 bytes, missing 2 trailing zeros + } + + col := Column{ + Name: "bin_col", + Type: BinaryColumnType, + BinaryOctetLength: 20, + } + + result := col.convertArg(truncatedValue) + resultBytes := result.([]byte) + + require.Equal(t, 20, len(resultBytes), "binary column should be padded to declared length") + // First 18 bytes should be unchanged + require.Equal(t, truncatedValue, resultBytes[:18]) + // Last 2 bytes should be zeros + require.Equal(t, []byte{0x00, 0x00}, resultBytes[18:]) +} + +func TestConvertArgBinaryColumnNoPaddingWhenFull(t *testing.T) { + // When binary value is already at full length, no padding should occur + fullValue := []uint8{ + 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, + 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10, + 0x11, 0x12, 0x13, 0x14, // 20 bytes + } + + col := Column{ + Name: "bin_col", + Type: BinaryColumnType, + BinaryOctetLength: 20, + } + + result := col.convertArg(fullValue) + resultBytes := result.([]byte) + + require.Equal(t, 20, len(resultBytes)) + require.Equal(t, fullValue, resultBytes) +} diff --git a/localtests/binary-to-varbinary/create.sql b/localtests/binary-to-varbinary/create.sql new file mode 100644 index 000000000..c2867f9ea --- /dev/null +++ b/localtests/binary-to-varbinary/create.sql @@ -0,0 +1,38 @@ +-- Test for https://github.com/github/gh-ost/issues/909 variant: +-- Binary columns with trailing zeros should preserve their values +-- when migrating from binary(N) to varbinary(M), even for rows +-- modified during migration via binlog events. + +drop table if exists gh_ost_test; +create table gh_ost_test ( + id int NOT NULL AUTO_INCREMENT, + info varchar(255) NOT NULL, + data binary(20) NOT NULL, + PRIMARY KEY (id) +) auto_increment=1; + +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 rows where data has trailing zeros (will be stripped by binlog) + INSERT INTO gh_ost_test (info, data) VALUES ('insert-during-1', X'aabbccdd00000000000000000000000000000000'); + INSERT INTO gh_ost_test (info, data) VALUES ('insert-during-2', X'11223344556677889900000000000000000000ee'); + + -- Update existing rows to values with trailing zeros + UPDATE gh_ost_test SET data = X'ffeeddcc00000000000000000000000000000000' WHERE info = 'update-target-1'; + UPDATE gh_ost_test SET data = X'aabbccdd11111111111111111100000000000000' WHERE info = 'update-target-2'; +end ;; + +-- Pre-existing rows (copied via rowcopy, not binlog - these should work fine) +INSERT INTO gh_ost_test (info, data) VALUES + ('pre-existing-1', X'01020304050607080910111213141516171819ff'), + ('pre-existing-2', X'0102030405060708091011121314151617181900'), + ('update-target-1', X'ffffffffffffffffffffffffffffffffffffffff'), + ('update-target-2', X'eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee'); diff --git a/localtests/binary-to-varbinary/extra_args b/localtests/binary-to-varbinary/extra_args new file mode 100644 index 000000000..7ebdacf6c --- /dev/null +++ b/localtests/binary-to-varbinary/extra_args @@ -0,0 +1 @@ +--alter="MODIFY data varbinary(32)"