From 7b25395999f51bcd3dd5ddada39befe969b6430e Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Fri, 13 Dec 2024 23:04:51 -0500 Subject: [PATCH] Minor changes from self review Signed-off-by: Matt Lord --- go/mysql/binlog/binlog_json.go | 33 +++++++++---------- go/test/utils/binlog.go | 7 ++-- go/test/utils/binlog_test.go | 2 ++ .../vreplication/replicator_plan.go | 10 +++--- .../vreplication/vplayer_flaky_test.go | 32 ++++++++++++------ 5 files changed, 47 insertions(+), 37 deletions(-) diff --git a/go/mysql/binlog/binlog_json.go b/go/mysql/binlog/binlog_json.go index 33a9c474bd8..2b83780df72 100644 --- a/go/mysql/binlog/binlog_json.go +++ b/go/mysql/binlog/binlog_json.go @@ -113,6 +113,7 @@ func ParseBinaryJSONDiff(data []byte) (sqltypes.Value, error) { } else { // Only the inner most function has the field name diff.WriteString("%s, ") // This will later be replaced by the field name } + outer = true pathLen, readTo := readVariableLength(data, pos) pos = readTo @@ -125,27 +126,25 @@ func ParseBinaryJSONDiff(data []byte) (sqltypes.Value, error) { diff.WriteByte('\'') diff.Write(path) diff.WriteByte('\'') - if opType == jsonDiffOpRemove { // No value for remove diff.WriteByte(')') - } else { - diff.WriteString(", ") - valueLen, readTo := readVariableLength(data, pos) - pos = readTo - value, err := ParseBinaryJSON(data[pos : pos+valueLen]) - if err != nil { - return sqltypes.Value{}, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, - "cannot read JSON diff value for path %s: %v", path, err) - } - pos += valueLen - if value.Type() == json.TypeString { - diff.WriteString(sqlparser.Utf8mb4Str) - } - diff.Write(value.MarshalTo(nil)) - diff.WriteByte(')') + continue } - outer = true + diff.WriteString(", ") + valueLen, readTo := readVariableLength(data, pos) + pos = readTo + value, err := ParseBinaryJSON(data[pos : pos+valueLen]) + if err != nil { + return sqltypes.Value{}, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, + "cannot read JSON diff value for path %s: %v", path, err) + } + pos += valueLen + if value.Type() == json.TypeString { + diff.WriteString(sqlparser.Utf8mb4Str) + } + diff.Write(value.MarshalTo(nil)) + diff.WriteByte(')') } return sqltypes.MakeTrusted(sqltypes.Expression, diff.Bytes()), nil diff --git a/go/test/utils/binlog.go b/go/test/utils/binlog.go index af24e3b1b57..7287bfee44c 100644 --- a/go/test/utils/binlog.go +++ b/go/test/utils/binlog.go @@ -88,10 +88,9 @@ func SetBinlogRowImageMode(mode string, cnfDir string, includePartialJSON bool) // variable is empty -- meaning we're not running in the CI and we assume // MySQL8.0 or later is used, and you can understand the failures and make // adjustments as necessary -- or it's set to reflect usage of MySQL 8.0 or -// later. This relies on the current standard values used such as mysql5.7, -// mysql8.0, mysql8.4, mariadb10.7, etc. This can be used when the CI test -// behavior needs to be altered based on the specific database platform -// we're testing against. +// later. This relies on the current standard values used such as mysql57, +// mysql80, mysql84, etc. This can be used when the CI test behavior needs +// to be altered based on the specific database platform we're testing against. func CIDBPlatformIsMySQL8orLater() bool { dbPlatform := strings.ToLower(os.Getenv("CI_DB_PLATFORM")) if dbPlatform == "" { diff --git a/go/test/utils/binlog_test.go b/go/test/utils/binlog_test.go index 45f787bc85a..5b307595107 100644 --- a/go/test/utils/binlog_test.go +++ b/go/test/utils/binlog_test.go @@ -45,6 +45,8 @@ func TestUtils(t *testing.T) { require.Contains(t, string(data), "binlog_row_image=noblob") require.Contains(t, string(data), "binlog_row_value_options=PARTIAL_JSON") require.Contains(t, os.Getenv(ExtraCnf), BinlogRowImageCnf) + } else { + require.Error(t, SetBinlogRowImageMode("noblob", tmpDir, true)) } // Test that clearing the mode will remove the cnf file and the cnf from the EXTRA_MY_CNF env var. diff --git a/go/vt/vttablet/tabletmanager/vreplication/replicator_plan.go b/go/vt/vttablet/tabletmanager/vreplication/replicator_plan.go index fa3a225b18b..6320ee38d55 100644 --- a/go/vt/vttablet/tabletmanager/vreplication/replicator_plan.go +++ b/go/vt/vttablet/tabletmanager/vreplication/replicator_plan.go @@ -480,12 +480,10 @@ func (tp *TablePlan) applyChange(rowChange *binlogdatapb.RowChange, executor fun jsonIndex := 0 for i, field := range tp.Fields { if field.Type == querypb.Type_JSON && rowChange.JsonPartialValues != nil { - if !isBitSet(rowChange.JsonPartialValues.Cols, jsonIndex) { + switch { + case !isBitSet(rowChange.JsonPartialValues.Cols, jsonIndex): // We use the full AFTER value which we already have. - jsonIndex++ - continue - } - if len(afterVals[i].Raw()) == 0 { + case len(afterVals[i].Raw()) == 0: // When using BOTH binlog_row_image=NOBLOB AND // binlog_row_value_options=PARTIAL_JSON then the JSON column has the data bit // set and the diff is empty when it's not present. So we want to use the @@ -500,7 +498,7 @@ func (tp *TablePlan) applyChange(rowChange *binlogdatapb.RowChange, executor fun return nil, vterrors.Wrapf(err, "failed to bind field value for %s.%s when building insert query", tp.TargetName, field.Name) } - } else { + default: // For JSON columns when binlog-row-value-options=PARTIAL_JSON is used, we // need to wrap the JSON diff function(s) around the BEFORE value. diff := bindvars["a_"+field.Name].Value diff --git a/go/vt/vttablet/tabletmanager/vreplication/vplayer_flaky_test.go b/go/vt/vttablet/tabletmanager/vreplication/vplayer_flaky_test.go index 8248dcf6476..b4a50ab529f 100644 --- a/go/vt/vttablet/tabletmanager/vreplication/vplayer_flaky_test.go +++ b/go/vt/vttablet/tabletmanager/vreplication/vplayer_flaky_test.go @@ -1519,14 +1519,15 @@ func TestPlayerRowMove(t *testing.T) { validateQueryCountStat(t, "replicate", 3) } -// TestPlayerPartialImagesUpdatePK tests the behavior of the vplayer when we -// have partial binlog images, meaning that binlog-row-image=NOBLOB and -// binlog-row-value-options=PARTIAL_JSON. These are both set when running the -// unit tests with runNoBlobTest=true and runPartialJSONTest=true. +// TestPlayerPartialImagesUpdatePK tests the behavior of the vplayer when +// updating the Primary Key for a row when we have partial binlog +// images, meaning that binlog-row-image=NOBLOB and +// binlog-row-value-options=PARTIAL_JSON. These are both set when running +// the unit tests with runNoBlobTest=true and runPartialJSONTest=true. // If the test is running in the CI and the database platform is not MySQL -// 8.0 or later (which you can control using the CI_DB_PLATFORM env variable), -// then runPartialJSONTest will be false and the test will be skipped. -// the test if it's not set. +// 8.0 or later (which you can control using the CI_DB_PLATFORM env +// variable), then runPartialJSONTest will be false and the test will be +// skipped. func TestPlayerPartialImagesUpdatePK(t *testing.T) { if !runNoBlobTest { t.Skip("Skipping test as binlog_row_image=NOBLOB is not set") @@ -1586,15 +1587,26 @@ func TestPlayerPartialImagesUpdatePK(t *testing.T) { }, }, { - input: `update src set id = id+10, bd = 'new blob data' where id = 2`, + input: `update src set bd = 'new blob data' where id = 2`, + output: []string{ + "update dst set bd=_binary'new blob data' where id=2", + }, + data: [][]string{ + {"1", "{\"key1\": \"val1\", \"color\": \"red\"}", "blob data"}, + {"2", "{\"key2\": \"val2\"}", "new blob data"}, + {"3", "{\"key3\": \"val3\"}", "blob data3"}, + }, + }, + { + input: `update src set id = id+10, bd = 'newest blob data' where id = 2`, output: []string{ "delete from dst where id=2", - "insert into dst(id,jd,bd) values (12,JSON_OBJECT(_utf8mb4'key2', _utf8mb4'val2'),_binary'new blob data')", + "insert into dst(id,jd,bd) values (12,JSON_OBJECT(_utf8mb4'key2', _utf8mb4'val2'),_binary'newest blob data')", }, data: [][]string{ {"1", "{\"key1\": \"val1\", \"color\": \"red\"}", "blob data"}, {"3", "{\"key3\": \"val3\"}", "blob data3"}, - {"12", "{\"key2\": \"val2\"}", "new blob data"}, + {"12", "{\"key2\": \"val2\"}", "newest blob data"}, }, }, {