-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Online DDL: avoid SQL's CONVERT(...), convert programmatically if needed
#16597
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
Changes from 3 commits
63d4ae3
904df0e
e3b07ba
0d0d56f
3d8bc2a
6e15d3f
3693d69
fe7e73f
0548937
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| MODIFY `t1` varchar(128) CHARACTER SET utf8mb4 NOT NULL, MODIFY `t2` varchar(128) CHARACTER SET latin2 NOT NULL, MODIFY `tutf8` varchar(128) CHARACTER SET latin1 NOT NULL |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| drop table if exists onlineddl_test; | ||
| create table onlineddl_test ( | ||
| id int auto_increment, | ||
| t1 varchar(128) charset latin1 collate latin1_swedish_ci, | ||
| t2 varchar(128) charset latin1 collate latin1_swedish_ci, | ||
| tutf8 varchar(128) charset utf8, | ||
| tutf8mb4 varchar(128) charset utf8mb4, | ||
| tlatin1 varchar(128) charset latin1 collate latin1_swedish_ci, | ||
| primary key(id) | ||
| ) auto_increment=1; | ||
|
|
||
| insert into onlineddl_test values (null, md5(rand()), md5(rand()), md5(rand()), md5(rand()), md5(rand())); | ||
| insert into onlineddl_test values (null, 'átesting', 'átesting', 'átesting', 'átesting', 'átesting'); | ||
| insert into onlineddl_test values (null, 'testátest', 'testátest', 'testátest', '🍻😀', 'átesting'); | ||
| insert into onlineddl_test values (null, 'átesting-binlog', 'átesting-binlog', 'átesting-binlog', 'átesting-binlog', 'átesting-binlog'); | ||
| insert into onlineddl_test values (null, 'testátest-binlog', 'testátest-binlog', 'testátest-binlog', '🍻😀', 'átesting-binlog'); | ||
| insert into onlineddl_test values (null, 'átesting-bnull', 'átesting-bnull', 'átesting-bnull', null, null); | ||
|
|
||
| drop event if exists onlineddl_test; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| (5.5|5.6|5.7) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| drop table if exists onlineddl_test; | ||
| create table onlineddl_test ( | ||
| id varchar(128) charset latin1 collate latin1_swedish_ci, | ||
| t1 varchar(128) charset latin1 collate latin1_swedish_ci, | ||
| t2 varchar(128) charset latin1 collate latin1_swedish_ci, | ||
| tutf8 varchar(128) charset utf8, | ||
| tutf8mb4 varchar(128) charset utf8mb4, | ||
| tlatin1 varchar(128) charset latin1 collate latin1_swedish_ci, | ||
| primary key(id) | ||
| ) auto_increment=1; | ||
|
|
||
| insert into onlineddl_test values (concat('átesting-', md5(rand())), md5(rand()), md5(rand()), md5(rand()), md5(rand()), md5(rand())); | ||
| insert into onlineddl_test values (concat('átesting-', md5(rand())), 'átesting', 'átesting', 'átesting', 'átesting', 'átesting'); | ||
| insert into onlineddl_test values (concat('átesting-', md5(rand())), 'testátest', 'testátest', 'testátest', '🍻😀', 'átesting'); | ||
|
|
||
| drop event if exists onlineddl_test; | ||
| delimiter ;; | ||
| create event onlineddl_test | ||
| on schedule every 1 second | ||
| starts current_timestamp | ||
| ends current_timestamp + interval 60 second | ||
| on completion not preserve | ||
| enable | ||
| do | ||
| begin | ||
| insert into onlineddl_test values (concat('átesting-', md5(rand())), md5(rand()), md5(rand()), md5(rand()), md5(rand()), md5(rand())); | ||
| insert into onlineddl_test values (concat('átesting-', md5(rand())), 'átesting-binlog', 'átesting-binlog', 'átesting-binlog', 'átesting-binlog', 'átesting-binlog'); | ||
| insert into onlineddl_test values (concat('átesting-', md5(rand())), 'testátest-binlog', 'testátest-binlog', 'testátest-binlog', '🍻😀', 'átesting-binlog'); | ||
| insert into onlineddl_test values (concat('átesting-', md5(rand())), 'átesting-bnull', 'átesting-bnull', 'átesting-bnull', null, null); | ||
| end ;; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| (5.5|5.6|5.7) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -289,6 +289,9 @@ func (v *VRepl) generateFilterQuery() error { | |
| sb.WriteString(fmt.Sprintf("CONCAT(%s)", escapeName(name))) | ||
| case sourceCol.Type() == "json": | ||
| sb.WriteString(fmt.Sprintf("convert(%s using utf8mb4)", escapeName(name))) | ||
| case targetCol.Type() == "json" && sourceCol.Type() != "json": | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This moves up from below so as to eliminate a case before we compare charsets for JSONs, which is not required and not beneficial. |
||
| // Convert any type to JSON: encode the type as utf8mb4 text | ||
| sb.WriteString(fmt.Sprintf("convert(%s using utf8mb4)", escapeName(name))) | ||
| case sourceCol.IsTextual(): | ||
| // Check source and target charset/encoding. If needed, create | ||
| // a binlogdatapb.CharsetConversion entry (later written to vreplication) | ||
|
|
@@ -301,19 +304,19 @@ func (v *VRepl) generateFilterQuery() error { | |
| if targetCol.IsTextual() && toCollation == collations.Unknown { | ||
| return vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "Character set %s not supported for column %s", targetCol.Charset(), targetCol.Name()) | ||
| } | ||
|
|
||
| if trivialCharset(fromCollation) && trivialCharset(toCollation) && targetCol.Type() != "json" { | ||
| sb.WriteString(escapeName(name)) | ||
| } else if fromCollation == toCollation && targetCol.Type() != "json" { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't want
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In any event, I don't think this is a major issue as the primary issue we've seen on the target/
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
You're right! We changed the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we're still left with a few |
||
| // No need for charset conversions as both have the same collation. | ||
| sb.WriteString(escapeName(name)) | ||
| } else { | ||
| // Charset conversion required: | ||
| v.convertCharset[targetName] = &binlogdatapb.CharsetConversion{ | ||
| FromCharset: sourceCol.Charset(), | ||
| ToCharset: targetCol.Charset(), | ||
| } | ||
| sb.WriteString(fmt.Sprintf("convert(%s using utf8mb4)", escapeName(name))) | ||
| sb.WriteString(escapeName(name)) | ||
| } | ||
| case targetCol.Type() == "json" && sourceCol.Type() != "json": | ||
| // Convert any type to JSON: encode the type as utf8mb4 text | ||
| sb.WriteString(fmt.Sprintf("convert(%s using utf8mb4)", escapeName(name))) | ||
| default: | ||
| sb.WriteString(escapeName(name)) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,7 @@ import ( | |
| "vitess.io/vitess/go/mysql/collations/charset" | ||
| "vitess.io/vitess/go/mysql/collations/colldata" | ||
| vjson "vitess.io/vitess/go/mysql/json" | ||
| "vitess.io/vitess/go/mysql/sqlerror" | ||
| "vitess.io/vitess/go/sqltypes" | ||
| "vitess.io/vitess/go/vt/binlog/binlogplayer" | ||
| "vitess.io/vitess/go/vt/sqlparser" | ||
|
|
@@ -257,7 +258,7 @@ func (tp *TablePlan) applyBulkInsert(sqlbuffer *bytes2.Buffer, rows []*querypb.R | |
| if i > 0 { | ||
| sqlbuffer.WriteString(", ") | ||
| } | ||
| if err := appendFromRow(tp.BulkInsertValues, sqlbuffer, tp.Fields, row, tp.FieldsToSkip); err != nil { | ||
| if err := tp.appendFromRow(tp.BulkInsertValues, sqlbuffer, tp.Fields, row, tp.FieldsToSkip); err != nil { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we make this change, which I'm OK with, then we don't need to pass in the other
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch! Fixed. |
||
| return nil, err | ||
| } | ||
| } | ||
|
|
@@ -324,9 +325,14 @@ func (tp *TablePlan) bindFieldVal(field *querypb.Field, val *sqltypes.Value) (*q | |
| if fromCollation == collations.Unknown { | ||
| return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "Character set %s not supported for column %s", conversion.FromCharset, field.Name) | ||
| } | ||
| out, err := charset.Convert(nil, charset.Charset_utf8mb4{}, val.Raw(), colldata.Lookup(fromCollation).Charset()) | ||
| toCollation := tp.CollationEnv.DefaultCollationForCharset(conversion.ToCharset) | ||
| if toCollation == collations.Unknown { | ||
| return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "Character set %s not supported for column %s", conversion.ToCharset, field.Name) | ||
| } | ||
|
|
||
| out, err := charset.Convert(nil, colldata.Lookup(toCollation).Charset(), val.Raw(), colldata.Lookup(fromCollation).Charset()) | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, sqlerror.NewSQLError(sqlerror.ERTruncatedWrongValueForField, sqlerror.SSUnknownSQLState, "Incorrect string value: %s", err.Error()) | ||
| } | ||
| return sqltypes.StringBindVariable(string(out)), nil | ||
| } | ||
|
|
@@ -590,7 +596,7 @@ func valsEqual(v1, v2 sqltypes.Value) bool { | |
| // note: there can be more fields than bind locations since extra columns might be requested from the source if not all | ||
| // primary keys columns are present in the target table, for example. Also some values in the row may not correspond for | ||
| // values from the database on the source: sum/count for aggregation queries, for example | ||
| func appendFromRow(pq *sqlparser.ParsedQuery, buf *bytes2.Buffer, fields []*querypb.Field, row *querypb.Row, skipFields map[string]bool) error { | ||
| func (tp *TablePlan) appendFromRow(pq *sqlparser.ParsedQuery, buf *bytes2.Buffer, fields []*querypb.Field, row *querypb.Row, skipFields map[string]bool) error { | ||
| bindLocations := pq.BindLocations() | ||
| if len(fields) < len(bindLocations) { | ||
| return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "wrong number of fields: got %d fields for %d bind locations ", | ||
|
|
@@ -601,6 +607,7 @@ func appendFromRow(pq *sqlparser.ParsedQuery, buf *bytes2.Buffer, fields []*quer | |
| typ querypb.Type | ||
| length int64 | ||
| offset int64 | ||
| field *querypb.Field | ||
| } | ||
| rowInfo := make([]*colInfo, 0) | ||
|
|
||
|
|
@@ -612,6 +619,7 @@ func appendFromRow(pq *sqlparser.ParsedQuery, buf *bytes2.Buffer, fields []*quer | |
| typ: field.Type, | ||
| length: length, | ||
| offset: offset, | ||
| field: field, | ||
| }) | ||
| } | ||
| if length > 0 { | ||
|
|
@@ -646,6 +654,24 @@ func appendFromRow(pq *sqlparser.ParsedQuery, buf *bytes2.Buffer, fields []*quer | |
| buf.WriteString(sqltypes.NullStr) | ||
| } else { | ||
| vv := sqltypes.MakeTrusted(typ, row.Values[col.offset:col.offset+col.length]) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this also allocate and later on too? Is it worth avoiding creating this if we overwrite it later?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. No double allocation. Also, converged the two codepaths that do |
||
|
|
||
| if conversion, ok := tp.ConvertCharset[col.field.Name]; ok && col.length >= 0 { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't want
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Due to my bad English, I'm not sure if you mean we should use Just in case you mean the former, we do have
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dbussink pointed out that you meant to highlight |
||
| // Non-null string value, for which we have a charset conversion instruction | ||
| fromCollation := tp.CollationEnv.DefaultCollationForCharset(conversion.FromCharset) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have to rely on the default collation for the charset (on from and to side)? If we take
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you're up for squeezing another change in here... I think we might want to make it
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It's a bit moot. We only use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is worth digging into. If we do end up using collation rather than charset, then there's a few proto changes to make, so this will be outside the scope of this PR. |
||
| if fromCollation == collations.Unknown { | ||
| return vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "Character set %s not supported for column %s", conversion.FromCharset, col.field.Name) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit, but errors aren't supposed to be capitalized (due to wrapping). That applies throughout the new code in the PR.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed! One place where I did leave the message capitalized is in |
||
| } | ||
| toCollation := tp.CollationEnv.DefaultCollationForCharset(conversion.ToCharset) | ||
| if toCollation == collations.Unknown { | ||
| return vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "Character set %s not supported for column %s", conversion.ToCharset, col.field.Name) | ||
| } | ||
| out, err := charset.Convert(nil, colldata.Lookup(toCollation).Charset(), vv.Raw(), colldata.Lookup(fromCollation).Charset()) | ||
| if err != nil { | ||
| return sqlerror.NewSQLError(sqlerror.ERTruncatedWrongValueForField, sqlerror.SSUnknownSQLState, "Incorrect string value: %s", err.Error()) | ||
| } | ||
| vv = sqltypes.MakeTrusted(typ, out) | ||
| } | ||
|
shlomi-noach marked this conversation as resolved.
Outdated
|
||
|
|
||
| vv.EncodeSQLBytes2(buf) | ||
| } | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dbussink do you think this is still needed? I don't think so anymore, now that we have native
JSONtype support.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expect this to be bytes we pass on to MySQL "on the other side" and they are interpreted there as either a
JSONfield or serialized as autf8mb4string if some other type on the target.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either way, I don't think it's a major deal on the source/
vcopierside as the primary problems we've seen there are when theseCONVERTcalls then preclude us from using the desired index in therowstreamerquery and you can't add indexes directly onJSONcolumns anyway.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave it like so for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JSON is a bit special anyway, since we can't use the direct textual representation, but we turn it into a sql expression using
JSON_OBJECTso we lose as little type information as possible.