Handle encoding binary data separately #16988

dbussink · 2024-10-17T11:43:58Z

Binary data should be encoded as such when interpolated, for example in bind variables. Otherwise valuable type information that the value is actually binary is lost.

Also means that we should not mark literals in the parser as binary either. They are strings and there's other introducers like _binary that end up marking things as proper binary.

Related Issue(s)

Fixes #16989

Checklist

"Backport to:" labels have been added if this change should be back-ported to release branches
If this change is to be back-ported to previous releases, a justification is included in the PR description
Tests were added or are not required
Did the new or modified tests pass consistently locally and on CI?
Documentation was added or is not required

vitess-bot · 2024-10-17T11:44:01Z

codecov · 2024-10-18T10:12:44Z

Codecov Report

Attention: Patch coverage is 71.25000% with 23 lines in your changes missing coverage. Please review.

Project coverage is 67.15%. Comparing base (03b93a1) to head (5e82912).
Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
go/mysql/query.go	52.08%	23 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16988      +/-   ##
==========================================
- Coverage   67.15%   67.15%   -0.01%     
==========================================
  Files        1571     1571              
  Lines      251868   251895      +27     
==========================================
+ Hits       169148   169150       +2     
- Misses      82720    82745      +25

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

The fix in vitessio#16988 makes a change here that's not really a breaking change, but it does break the test as the test matches the exact string output which changes here due to a binary value. Signed-off-by: Dirkjan Bussink <[email protected]>

Binary data should be encoded as such when interpolated, for example in bind variables. Otherwise valuable type information that the value is actually binary is lost. Also means that we should not mark literals in the parser as binary either. They are strings and there's other introducers like `_binary` that end up marking things as proper binary. Signed-off-by: Dirkjan Bussink <[email protected]>

This has been using all binary everywhere since day one this was added, but we are a lot more type aware and handle types correctly now. This means we also should convert prepared statement values to the proper types as well when we receive them as inputs for queries. This way we know we can handle them correctly further down as well. Signed-off-by: Dirkjan Bussink <[email protected]>

Since we now have proper binary data encoding and these are binary columsn, we see the data as binary now sent down so we have to match on that. WIP, many more tests like this that need to be updated. Signed-off-by: Dirkjan Bussink <[email protected]>

Signed-off-by: Dirkjan Bussink <[email protected]>

This tracks the explicit type for the id column so that we can use that properly when binding variables. If we don't bind the right type, we end up depending on implicit MySQL type casting behavior which isn't something we should do. Making it explicit here is much more robust. Signed-off-by: Dirkjan Bussink <[email protected]>

This is an integer column, so we should those as bind variables and not a string since then we depend on string to integer conversion logic inside MySQL itself. Signed-off-by: Dirkjan Bussink <[email protected]>

Using the X'' syntax makes things behave differently, since it makes MySQL interpret it as a hex number in contexts where it casts it to an integer, for example in a `where id = 'binary string'` where `id` is a numerical type. `_binary` as a prefix more behaves like no prefix at all, so it better matches how things behave today so we're better backwards compatible. Signed-off-by: Dirkjan Bussink <[email protected]>

These are not allowed in MySQL as well, so don't allow them in Vitess either. Signed-off-by: Dirkjan Bussink <[email protected]>

Signed-off-by: Dirkjan Bussink <[email protected]>

dbussink · 2024-10-22T12:45:22Z

go/mysql/query.go

@@ -713,7 +713,11 @@ func (c *Conn) parseStmtArgs(data []byte, typ querypb.Type, pos int) (sqltypes.V
 		}
 		switch size {
 		case 0x00:
-			return sqltypes.NewVarChar(" "), pos, ok


This is a broken value, the protocol definition specifies that a zero size means the zero date / date time and that is represented in a string with all the 0 values as below.

dbussink · 2024-10-22T12:45:42Z

go/mysql/query.go

+			val = strconv.AppendInt(val, int64(month), 10)
+			val = append(val, '-')
+			val = strconv.AppendInt(val, int64(day), 10)
+			if typ != sqltypes.Date {


This is so that we don't add a time value if we're a date type.

dbussink · 2024-10-22T12:46:23Z

go/mysql/query.go

 		default:
 			return sqltypes.NULL, 0, false
 		}
 	case sqltypes.Decimal, sqltypes.Text, sqltypes.Blob, sqltypes.VarChar, sqltypes.VarBinary, sqltypes.Year, sqltypes.Char,
 		sqltypes.Bit, sqltypes.Enum, sqltypes.Set, sqltypes.Geometry, sqltypes.Binary, sqltypes.TypeJSON, sqltypes.Vector:
 		val, pos, ok := readLenEncStringAsBytesCopy(data, pos)
-		return sqltypes.MakeTrusted(sqltypes.VarBinary, val), pos, ok
+		return sqltypes.MakeTrusted(typ, val), pos, ok


This now returns the actual type instead of marking everything as binary which can break things. That was exposed by the other changes in this PR here where serialization of binary values was changed.

dbussink · 2024-10-22T12:53:12Z

go/sqltypes/value.go

+}
+
+func encodeBinarySQLBytes2(val []byte, buf *bytes2.Buffer) {
+	buf.Write([]byte("_binary"))


So there are basically 2 ways to do binary strings. One is with the _binary introducer as done here, another approach is using the X'cafe' syntax.

Initially I tried the second approach, so no introducer is needed. There's a problem with that approach though, which is how it is handled in the case of a match on a column of a different type.

In case there's an id column of type int and you query it with select * from t where id = '1' really a binary string (and not a varchar). When using hex style syntax, it would turn into:

select * from t where id = X'31'

This changes behavior then, because in this context MySQL interprets it as a hex numerical, so it really executes select * from t where id = 49 (with 49 being the numerical value for that binary input).

When using _binary it doesn't change behavior. So the final choice is to use the introducer then.

dbussink · 2024-10-22T12:54:28Z

go/vt/sqlparser/sql.y

-| underscore_charsets column_name_or_offset %prec UNARY
-  {
-    $$ = &IntroducerExpr{CharacterSet: $1, Expr: $2}
-  }


Using the introducer for binary values exposed another problem. We were accepting introducers with a column, which is not something that MySQL does.

There was no dependency on this anywhere, except in a test for vplayer filtering where we were using this invalid syntax.

dbussink · 2024-10-22T12:56:19Z

go/vt/vttablet/tabletserver/messager/message_manager.go

@@ -236,6 +236,9 @@ type messageManager struct {
 	ackQuery                  *sqlparser.ParsedQuery
 	postponeQuery             *sqlparser.ParsedQuery
 	purgeQuery                *sqlparser.ParsedQuery
+
+	// idType is the type of the id column in the message table.
+	idType sqltypes.Type


Before the messager always bound as binary, which is not correct. This is what also exposed originally the problem with the hex syntax, as it would not find rows.

The proper fix here though is to actually look at the real type of the column and use that in the bind variable so we always correctly handle it, independent from whether by accident passing in a binary value to this random type works or not.

dbussink · 2024-10-22T12:58:32Z

go/vt/vttablet/tabletmanager/vreplication/vplayer_flaky_test.go

@@ -748,7 +748,7 @@ func TestPlayerFilters(t *testing.T) {
 			Filter: "select id1, val from src5 where val = 'abc'",
 		}, {
 			Match:  "dst_charset",
-			Filter: "select id1, concat(substr(_utf8mb4 val collate utf8mb4_bin,1,1),'abcxyz') val, concat(substr(_utf8mb4 val collate utf8mb4_bin,1,1),'abcxyz') val2 from src_charset",
+			Filter: "select id1, concat(substr(CONVERT(val USING utf8mb4) COLLATE utf8mb4_bin,1,1),'abcxyz') val, concat(substr(CONVERT(val USING utf8mb4) COLLATE utf8mb4_bin,1,1),'abcxyz') val2 from src_charset",


_utf8mb4 val here is the actual invalid syntax, since you can't use an introducer on a column. Instead, this now properly uses a convert function to force the correct collation for this test.

vmg

These fixes all look great 🙇

harshit-gangal · 2024-10-23T08:41:32Z

go/vt/vttablet/tabletserver/tx_engine_test.go

-		expQuery:    `update _vt.redo_state set state = 1, message = 'deadline exceeded' where dtid = 'aa'`,
+		expQuery:    `update _vt.redo_state set state = 1, message = 'deadline exceeded' where dtid = _binary'aa'`,
 	}, {
 		receivedErr: vterrors.New(vtrpcpb.Code_INVALID_ARGUMENT, "invalid argument"),
 		retryable:   false,
-		expQuery:    `update _vt.redo_state set state = 0, message = 'invalid argument' where dtid = 'aa'`,
+		expQuery:    `update _vt.redo_state set state = 0, message = 'invalid argument' where dtid = _binary'aa'`,
 	}, {
 		receivedErr: sqlerror.NewSQLError(sqlerror.ERLockDeadlock, sqlerror.SSLockDeadlock, "Deadlock found when trying to get lock; try restarting transaction"),
 		retryable:   false,
-		expQuery:    `update _vt.redo_state set state = 0, message = 'Deadlock found when trying to get lock; try restarting transaction (errno 1213) (sqlstate 40001)' where dtid = 'aa'`,
+		expQuery:    `update _vt.redo_state set state = 0, message = 'Deadlock found when trying to get lock; try restarting transaction (errno 1213) (sqlstate 40001)' where dtid = _binary'aa'`,


Is this the side effect or it is necessary to have _binary for all varbinary column types?

This query is generated with a bind var which now always uses _binary for a binary value.

Doing this is required to fix the original bug where variables weren't correctly typed and resulted in broken behavior.

harshit-gangal

Looks great. Just left 1 comment for understanding.

github-actions bot added this to the v22.0.0 milestone Oct 17, 2024

dbussink force-pushed the dbussink/fix-binary-encoding branch from ec1a3a9 to 2c56bcf Compare October 17, 2024 11:59

dbussink force-pushed the dbussink/fix-binary-encoding branch 7 times, most recently from bc7c8f1 to f656471 Compare October 22, 2024 07:48

dbussink mentioned this pull request Oct 22, 2024

Relax vexplain test for upcoming changes #17035

Merged

5 tasks

dbussink added 12 commits October 22, 2024 22:29

Fix vexplain query tests

f3501eb

Signed-off-by: Dirkjan Bussink <[email protected]>

Fix a bunch more tests

1f03f7b

Signed-off-by: Dirkjan Bussink <[email protected]>

Update more tests

e32fc23

Signed-off-by: Dirkjan Bussink <[email protected]>

Fix another test

a7a4a50

Signed-off-by: Dirkjan Bussink <[email protected]>

Another test fix

379d4d3

Signed-off-by: Dirkjan Bussink <[email protected]>

Use correct type for value

ef410f9

This is an integer column, so we should those as bind variables and not a string since then we depend on string to integer conversion logic inside MySQL itself. Signed-off-by: Dirkjan Bussink <[email protected]>

Disallow introducers for column expressions

e810914

These are not allowed in MySQL as well, so don't allow them in Vitess either. Signed-off-by: Dirkjan Bussink <[email protected]>

dbussink added 2 commits October 22, 2024 22:30

Fix tests

94019cf

Signed-off-by: Dirkjan Bussink <[email protected]>

Handle upgrade / downgrade in test

5e82912

Signed-off-by: Dirkjan Bussink <[email protected]>

dbussink force-pushed the dbussink/fix-binary-encoding branch from f656471 to 5e82912 Compare October 22, 2024 20:30

dbussink commented Oct 22, 2024

View reviewed changes

dbussink marked this pull request as ready for review October 22, 2024 20:35

dbussink requested review from mattlord, rohit-nayak-ps, derekperkins, harshit-gangal, systay, shlomi-noach, timvaillancourt, frouioui, GuptaManan100, arthurschreiber and vmg as code owners October 22, 2024 20:35

vmg approved these changes Oct 23, 2024

View reviewed changes

harshit-gangal reviewed Oct 23, 2024

View reviewed changes

harshit-gangal approved these changes Oct 23, 2024

View reviewed changes

dbussink merged commit b0b7981 into vitessio:main Oct 23, 2024
103 of 104 checks passed

dbussink deleted the dbussink/fix-binary-encoding branch October 23, 2024 12:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle encoding binary data separately #16988

Handle encoding binary data separately #16988

dbussink commented Oct 17, 2024 •

edited

Loading

vitess-bot bot commented Oct 17, 2024

codecov bot commented Oct 18, 2024 •

edited

Loading

dbussink Oct 22, 2024

dbussink Oct 22, 2024

dbussink Oct 22, 2024

dbussink Oct 22, 2024

dbussink Oct 22, 2024

dbussink Oct 22, 2024

dbussink Oct 22, 2024

vmg left a comment

harshit-gangal Oct 23, 2024

dbussink Oct 23, 2024

harshit-gangal left a comment

Handle encoding binary data separately #16988

Handle encoding binary data separately #16988

Conversation

dbussink commented Oct 17, 2024 • edited Loading

Related Issue(s)

Checklist

vitess-bot bot commented Oct 17, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

codecov bot commented Oct 18, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harshit-gangal left a comment

Choose a reason for hiding this comment

dbussink commented Oct 17, 2024 •

edited

Loading

codecov bot commented Oct 18, 2024 •

edited

Loading