Skip to content

Commit aadbb79

Browse files
jorendorffjorendorff
authored
Fix problems when altering a column from binary to varbinary (#1628)
* Fix binary column trailing zero stripping for non-key columns MySQL's binlog strips trailing 0x00 bytes from binary(N) columns. PR #915 fixed this for unique key columns only, but the same issue affects all binary columns in INSERT/UPDATE operations. Remove the isUniqueKeyColumn condition so all binary(N) columns are padded to their declared length. Fixes a variation of #909 where the affected column is not a primary key. * Simplify by removing isUniqueKeyColumn now that it's no longer used. * In convertArg, don't convert binary data to strings. In this case, the input is binary, and the column type is `binary`. So the output should be binary, not text. * fix a lint
1 parent a1e9c9d commit aadbb79

File tree

7 files changed

+120
-41
lines changed

7 files changed

+120
-41
lines changed

‎go/logic/applier.go‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1470,10 +1470,9 @@ func (this *Applier) buildDMLEventQuery(dmlEvent *binlog.BinlogDMLEvent) []*dmlB
14701470
results = append(results, this.buildDMLEventQuery(dmlEvent)...)
14711471
return results
14721472
}
1473-
query, sharedArgs, uniqueKeyArgs, err := this.dmlUpdateQueryBuilder.BuildQuery(dmlEvent.NewColumnValues.AbstractValues(), dmlEvent.WhereColumnValues.AbstractValues())
1473+
query, updateArgs, err := this.dmlUpdateQueryBuilder.BuildQuery(dmlEvent.NewColumnValues.AbstractValues(), dmlEvent.WhereColumnValues.AbstractValues())
14741474
args := sqlutils.Args()
1475-
args = append(args, sharedArgs...)
1476-
args = append(args, uniqueKeyArgs...)
1475+
args = append(args, updateArgs...)
14771476
return []*dmlBuildResult{newDmlBuildResult(query, args, 0, err)}
14781477
}
14791478
}

‎go/sql/builder.go‎

Lines changed: 11 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -169,11 +169,11 @@ func (b *CheckpointInsertQueryBuilder) BuildQuery(uniqueKeyArgs []interface{}) (
169169
}
170170
convertedArgs := make([]interface{}, 0, 2*b.uniqueKeyColumns.Len())
171171
for i, column := range b.uniqueKeyColumns.Columns() {
172-
minArg := column.convertArg(uniqueKeyArgs[i], true)
172+
minArg := column.convertArg(uniqueKeyArgs[i])
173173
convertedArgs = append(convertedArgs, minArg)
174174
}
175175
for i, column := range b.uniqueKeyColumns.Columns() {
176-
minArg := column.convertArg(uniqueKeyArgs[i+b.uniqueKeyColumns.Len()], true)
176+
minArg := column.convertArg(uniqueKeyArgs[i+b.uniqueKeyColumns.Len()])
177177
convertedArgs = append(convertedArgs, minArg)
178178
}
179179
return b.preparedStatement, convertedArgs, nil
@@ -533,7 +533,7 @@ func (b *DMLDeleteQueryBuilder) BuildQuery(args []interface{}) (string, []interf
533533
uniqueKeyArgs := make([]interface{}, 0, b.uniqueKeyColumns.Len())
534534
for _, column := range b.uniqueKeyColumns.Columns() {
535535
tableOrdinal := b.tableColumns.Ordinals[column.Name]
536-
arg := column.convertArg(args[tableOrdinal], true)
536+
arg := column.convertArg(args[tableOrdinal])
537537
uniqueKeyArgs = append(uniqueKeyArgs, arg)
538538
}
539539
return b.preparedStatement, uniqueKeyArgs, nil
@@ -595,7 +595,7 @@ func (b *DMLInsertQueryBuilder) BuildQuery(args []interface{}) (string, []interf
595595
sharedArgs := make([]interface{}, 0, b.sharedColumns.Len())
596596
for _, column := range b.sharedColumns.Columns() {
597597
tableOrdinal := b.tableColumns.Ordinals[column.Name]
598-
arg := column.convertArg(args[tableOrdinal], false)
598+
arg := column.convertArg(args[tableOrdinal])
599599
sharedArgs = append(sharedArgs, arg)
600600
}
601601
return b.preparedStatement, sharedArgs, nil
@@ -661,20 +661,18 @@ func NewDMLUpdateQueryBuilder(databaseName, tableName string, tableColumns, shar
661661

662662
// BuildQuery builds the arguments array for a DML event UPDATE query.
663663
// It returns the query string, the shared arguments array, and the unique key arguments array.
664-
func (b *DMLUpdateQueryBuilder) BuildQuery(valueArgs, whereArgs []interface{}) (string, []interface{}, []interface{}, error) {
665-
sharedArgs := make([]interface{}, 0, b.sharedColumns.Len())
664+
func (b *DMLUpdateQueryBuilder) BuildQuery(valueArgs, whereArgs []interface{}) (string, []interface{}, error) {
665+
args := make([]interface{}, 0, b.sharedColumns.Len()+b.uniqueKeyColumns.Len())
666666
for _, column := range b.sharedColumns.Columns() {
667667
tableOrdinal := b.tableColumns.Ordinals[column.Name]
668-
arg := column.convertArg(valueArgs[tableOrdinal], false)
669-
sharedArgs = append(sharedArgs, arg)
668+
arg := column.convertArg(valueArgs[tableOrdinal])
669+
args = append(args, arg)
670670
}
671-
672-
uniqueKeyArgs := make([]interface{}, 0, b.uniqueKeyColumns.Len())
673671
for _, column := range b.uniqueKeyColumns.Columns() {
674672
tableOrdinal := b.tableColumns.Ordinals[column.Name]
675-
arg := column.convertArg(whereArgs[tableOrdinal], true)
676-
uniqueKeyArgs = append(uniqueKeyArgs, arg)
673+
arg := column.convertArg(whereArgs[tableOrdinal])
674+
args = append(args, arg)
677675
}
678676

679-
return b.preparedStatement, sharedArgs, uniqueKeyArgs, nil
677+
return b.preparedStatement, args, nil
680678
}

‎go/sql/builder_test.go‎

Lines changed: 14 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -647,7 +647,7 @@ func TestBuildDMLUpdateQuery(t *testing.T) {
647647
uniqueKeyColumns := NewColumnList([]string{"position"})
648648
builder, err := NewDMLUpdateQueryBuilder(databaseName, tableName, tableColumns, sharedColumns, sharedColumns, uniqueKeyColumns)
649649
require.NoError(t, err)
650-
query, sharedArgs, uniqueKeyArgs, err := builder.BuildQuery(valueArgs, whereArgs)
650+
query, updateArgs, err := builder.BuildQuery(valueArgs, whereArgs)
651651
require.NoError(t, err)
652652
expected := `
653653
update /* gh-ost mydb.tbl */
@@ -657,15 +657,14 @@ func TestBuildDMLUpdateQuery(t *testing.T) {
657657
((position = ?))
658658
`
659659
require.Equal(t, normalizeQuery(expected), normalizeQuery(query))
660-
require.Equal(t, []interface{}{3, "testname", 17, 23}, sharedArgs)
661-
require.Equal(t, []interface{}{17}, uniqueKeyArgs)
660+
require.Equal(t, []interface{}{3, "testname", 17, 23, 17}, updateArgs)
662661
}
663662
{
664663
sharedColumns := NewColumnList([]string{"id", "name", "position", "age"})
665664
uniqueKeyColumns := NewColumnList([]string{"position", "name"})
666665
builder, err := NewDMLUpdateQueryBuilder(databaseName, tableName, tableColumns, sharedColumns, sharedColumns, uniqueKeyColumns)
667666
require.NoError(t, err)
668-
query, sharedArgs, uniqueKeyArgs, err := builder.BuildQuery(valueArgs, whereArgs)
667+
query, updateArgs, err := builder.BuildQuery(valueArgs, whereArgs)
669668
require.NoError(t, err)
670669
expected := `
671670
update /* gh-ost mydb.tbl */
@@ -675,15 +674,14 @@ func TestBuildDMLUpdateQuery(t *testing.T) {
675674
((position = ?) and (name = ?))
676675
`
677676
require.Equal(t, normalizeQuery(expected), normalizeQuery(query))
678-
require.Equal(t, []interface{}{3, "testname", 17, 23}, sharedArgs)
679-
require.Equal(t, []interface{}{17, "testname"}, uniqueKeyArgs)
677+
require.Equal(t, []interface{}{3, "testname", 17, 23, 17, "testname"}, updateArgs)
680678
}
681679
{
682680
sharedColumns := NewColumnList([]string{"id", "name", "position", "age"})
683681
uniqueKeyColumns := NewColumnList([]string{"age"})
684682
builder, err := NewDMLUpdateQueryBuilder(databaseName, tableName, tableColumns, sharedColumns, sharedColumns, uniqueKeyColumns)
685683
require.NoError(t, err)
686-
query, sharedArgs, uniqueKeyArgs, err := builder.BuildQuery(valueArgs, whereArgs)
684+
query, updateArgs, err := builder.BuildQuery(valueArgs, whereArgs)
687685
require.NoError(t, err)
688686
expected := `
689687
update /* gh-ost mydb.tbl */
@@ -693,15 +691,14 @@ func TestBuildDMLUpdateQuery(t *testing.T) {
693691
((age = ?))
694692
`
695693
require.Equal(t, normalizeQuery(expected), normalizeQuery(query))
696-
require.Equal(t, []interface{}{3, "testname", 17, 23}, sharedArgs)
697-
require.Equal(t, []interface{}{56}, uniqueKeyArgs)
694+
require.Equal(t, []interface{}{3, "testname", 17, 23, 56}, updateArgs)
698695
}
699696
{
700697
sharedColumns := NewColumnList([]string{"id", "name", "position", "age"})
701698
uniqueKeyColumns := NewColumnList([]string{"age", "position", "id", "name"})
702699
builder, err := NewDMLUpdateQueryBuilder(databaseName, tableName, tableColumns, sharedColumns, sharedColumns, uniqueKeyColumns)
703700
require.NoError(t, err)
704-
query, sharedArgs, uniqueKeyArgs, err := builder.BuildQuery(valueArgs, whereArgs)
701+
query, updateArgs, err := builder.BuildQuery(valueArgs, whereArgs)
705702
require.NoError(t, err)
706703
expected := `
707704
update /* gh-ost mydb.tbl */
@@ -711,8 +708,7 @@ func TestBuildDMLUpdateQuery(t *testing.T) {
711708
((age = ?) and (position = ?) and (id = ?) and (name = ?))
712709
`
713710
require.Equal(t, normalizeQuery(expected), normalizeQuery(query))
714-
require.Equal(t, []interface{}{3, "testname", 17, 23}, sharedArgs)
715-
require.Equal(t, []interface{}{56, 17, 3, "testname"}, uniqueKeyArgs)
711+
require.Equal(t, []interface{}{3, "testname", 17, 23, 56, 17, 3, "testname"}, updateArgs)
716712
}
717713
{
718714
sharedColumns := NewColumnList([]string{"id", "name", "position", "age"})
@@ -732,7 +728,7 @@ func TestBuildDMLUpdateQuery(t *testing.T) {
732728
uniqueKeyColumns := NewColumnList([]string{"id"})
733729
builder, err := NewDMLUpdateQueryBuilder(databaseName, tableName, tableColumns, sharedColumns, mappedColumns, uniqueKeyColumns)
734730
require.NoError(t, err)
735-
query, sharedArgs, uniqueKeyArgs, err := builder.BuildQuery(valueArgs, whereArgs)
731+
query, updateArgs, err := builder.BuildQuery(valueArgs, whereArgs)
736732
require.NoError(t, err)
737733
expected := `
738734
update /* gh-ost mydb.tbl */
@@ -742,8 +738,7 @@ func TestBuildDMLUpdateQuery(t *testing.T) {
742738
((id = ?))
743739
`
744740
require.Equal(t, normalizeQuery(expected), normalizeQuery(query))
745-
require.Equal(t, []interface{}{3, "testname", 17, 23}, sharedArgs)
746-
require.Equal(t, []interface{}{3}, uniqueKeyArgs)
741+
require.Equal(t, []interface{}{3, "testname", 17, 23, 3}, updateArgs)
747742
}
748743
}
749744

@@ -759,7 +754,7 @@ func TestBuildDMLUpdateQuerySignedUnsigned(t *testing.T) {
759754
require.NoError(t, err)
760755
{
761756
// test signed
762-
query, sharedArgs, uniqueKeyArgs, err := builder.BuildQuery(valueArgs, whereArgs)
757+
query, updateArgs, err := builder.BuildQuery(valueArgs, whereArgs)
763758
require.NoError(t, err)
764759
expected := `
765760
update /* gh-ost mydb.tbl */
@@ -769,14 +764,13 @@ func TestBuildDMLUpdateQuerySignedUnsigned(t *testing.T) {
769764
((position = ?))
770765
`
771766
require.Equal(t, normalizeQuery(expected), normalizeQuery(query))
772-
require.Equal(t, []interface{}{3, "testname", int8(-17), int8(-2)}, sharedArgs)
773-
require.Equal(t, []interface{}{int8(-3)}, uniqueKeyArgs)
767+
require.Equal(t, []interface{}{3, "testname", int8(-17), int8(-2), int8(-3)}, updateArgs)
774768
}
775769
{
776770
// test unsigned
777771
sharedColumns.SetUnsigned("age")
778772
uniqueKeyColumns.SetUnsigned("position")
779-
query, sharedArgs, uniqueKeyArgs, err := builder.BuildQuery(valueArgs, whereArgs)
773+
query, updateArgs, err := builder.BuildQuery(valueArgs, whereArgs)
780774
require.NoError(t, err)
781775
expected := `
782776
update /* gh-ost mydb.tbl */
@@ -786,8 +780,7 @@ func TestBuildDMLUpdateQuerySignedUnsigned(t *testing.T) {
786780
((position = ?))
787781
`
788782
require.Equal(t, normalizeQuery(expected), normalizeQuery(query))
789-
require.Equal(t, []interface{}{3, "testname", int8(-17), uint8(254)}, sharedArgs)
790-
require.Equal(t, []interface{}{uint8(253)}, uniqueKeyArgs)
783+
require.Equal(t, []interface{}{3, "testname", int8(-17), uint8(254), uint8(253)}, updateArgs)
791784
}
792785
}
793786

‎go/sql/types.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ type Column struct {
5757
MySQLType string
5858
}
5959

60-
func (this *Column) convertArg(arg interface{}, isUniqueKeyColumn bool) interface{} {
60+
func (this *Column) convertArg(arg interface{}) interface{} {
6161
var arg2Bytes []byte
6262
if s, ok := arg.(string); ok {
6363
arg2Bytes = []byte(s)
@@ -77,14 +77,14 @@ func (this *Column) convertArg(arg interface{}, isUniqueKeyColumn bool) interfac
7777
}
7878
}
7979

80-
if this.Type == BinaryColumnType && isUniqueKeyColumn {
80+
if this.Type == BinaryColumnType {
8181
size := len(arg2Bytes)
8282
if uint(size) < this.BinaryOctetLength {
8383
buf := bytes.NewBuffer(arg2Bytes)
8484
for i := uint(0); i < (this.BinaryOctetLength - uint(size)); i++ {
8585
buf.Write([]byte{0})
8686
}
87-
arg = buf.String()
87+
arg = buf.Bytes()
8888
}
8989
}
9090

‎go/sql/types_test.go‎

Lines changed: 51 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,56 @@ func TestConvertArgCharsetDecoding(t *testing.T) {
6262
}
6363

6464
// Should decode []uint8
65-
str := col.convertArg(latin1Bytes, false)
65+
str := col.convertArg(latin1Bytes)
6666
require.Equal(t, "Garçon !", str)
6767
}
68+
69+
func TestConvertArgBinaryColumnPadding(t *testing.T) {
70+
// Test that binary columns are padded with trailing zeros to their declared length.
71+
// This is needed because MySQL's binlog strips trailing 0x00 bytes from binary values.
72+
// See https://github.com/github/gh-ost/issues/909
73+
74+
// Simulates a binary(20) column where binlog delivered only 18 bytes
75+
// (trailing zeros were stripped)
76+
truncatedValue := []uint8{
77+
0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08,
78+
0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10,
79+
0x11, 0x12, // 18 bytes, missing 2 trailing zeros
80+
}
81+
82+
col := Column{
83+
Name: "bin_col",
84+
Type: BinaryColumnType,
85+
BinaryOctetLength: 20,
86+
}
87+
88+
result := col.convertArg(truncatedValue)
89+
resultBytes := result.([]byte)
90+
91+
require.Equal(t, 20, len(resultBytes), "binary column should be padded to declared length")
92+
// First 18 bytes should be unchanged
93+
require.Equal(t, truncatedValue, resultBytes[:18])
94+
// Last 2 bytes should be zeros
95+
require.Equal(t, []byte{0x00, 0x00}, resultBytes[18:])
96+
}
97+
98+
func TestConvertArgBinaryColumnNoPaddingWhenFull(t *testing.T) {
99+
// When binary value is already at full length, no padding should occur
100+
fullValue := []uint8{
101+
0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08,
102+
0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10,
103+
0x11, 0x12, 0x13, 0x14, // 20 bytes
104+
}
105+
106+
col := Column{
107+
Name: "bin_col",
108+
Type: BinaryColumnType,
109+
BinaryOctetLength: 20,
110+
}
111+
112+
result := col.convertArg(fullValue)
113+
resultBytes := result.([]byte)
114+
115+
require.Equal(t, 20, len(resultBytes))
116+
require.Equal(t, fullValue, resultBytes)
117+
}

‎localtests/binary-to-varbinary/create.sql‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
-- Test for https://github.com/github/gh-ost/issues/909 variant:
2+
-- Binary columns with trailing zeros should preserve their values
3+
-- when migrating from binary(N) to varbinary(M), even for rows
4+
-- modified during migration via binlog events.
5+
6+
drop table if exists gh_ost_test;
7+
create table gh_ost_test (
8+
id int NOT NULL AUTO_INCREMENT,
9+
info varchar(255) NOT NULL,
10+
data binary(20) NOT NULL,
11+
PRIMARY KEY (id)
12+
) auto_increment=1;
13+
14+
drop event if exists gh_ost_test;
15+
delimiter ;;
16+
create event gh_ost_test
17+
on schedule every 1 second
18+
starts current_timestamp
19+
ends current_timestamp + interval 60 second
20+
on completion not preserve
21+
enable
22+
do
23+
begin
24+
-- Insert rows where data has trailing zeros (will be stripped by binlog)
25+
INSERT INTO gh_ost_test (info, data) VALUES ('insert-during-1', X'aabbccdd00000000000000000000000000000000');
26+
INSERT INTO gh_ost_test (info, data) VALUES ('insert-during-2', X'11223344556677889900000000000000000000ee');
27+
28+
-- Update existing rows to values with trailing zeros
29+
UPDATE gh_ost_test SET data = X'ffeeddcc00000000000000000000000000000000' WHERE info = 'update-target-1';
30+
UPDATE gh_ost_test SET data = X'aabbccdd11111111111111111100000000000000' WHERE info = 'update-target-2';
31+
end ;;
32+
33+
-- Pre-existing rows (copied via rowcopy, not binlog - these should work fine)
34+
INSERT INTO gh_ost_test (info, data) VALUES
35+
('pre-existing-1', X'01020304050607080910111213141516171819ff'),
36+
('pre-existing-2', X'0102030405060708091011121314151617181900'),
37+
('update-target-1', X'ffffffffffffffffffffffffffffffffffffffff'),
38+
('update-target-2', X'eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee');

‎localtests/binary-to-varbinary/extra_args‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
--alter="MODIFY data varbinary(32)"

0 commit comments

Comments
 (0)