From 40c884a17b6b8101df15c07edafd723048acd673 Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Thu, 19 Dec 2024 09:07:59 -0500 Subject: [PATCH] Nitting while waiting for reviews Signed-off-by: Matt Lord --- go/test/utils/binlog.go | 6 +-- go/test/utils/binlog_test.go | 10 ++-- .../vreplication/framework_test.go | 8 +-- .../vreplication/replicator_plan.go | 18 +++---- .../vreplication/vplayer_flaky_test.go | 53 +++++++++++++------ 5 files changed, 57 insertions(+), 38 deletions(-) diff --git a/go/test/utils/binlog.go b/go/test/utils/binlog.go index 7287bfee44c..0db6929054c 100644 --- a/go/test/utils/binlog.go +++ b/go/test/utils/binlog.go @@ -28,12 +28,12 @@ const ( BinlogRowImageCnf = "binlog-row-image.cnf" ) -// SetBinlogRowImageMode creates a temp cnf file to set binlog_row_image=NOBLOB and +// SetBinlogRowImageOptions creates a temp cnf file to set binlog_row_image=NOBLOB and // optionally binlog_row_value_options=PARTIAL_JSON (since it does not exist in 5.7) // for vreplication unit tests. // It adds it to the EXTRA_MY_CNF environment variable which appends text from them // into my.cnf. -func SetBinlogRowImageMode(mode string, cnfDir string, includePartialJSON bool) error { +func SetBinlogRowImageOptions(mode string, partialJSON bool, cnfDir string) error { var newCnfs []string // remove any existing extra cnfs for binlog row image @@ -59,7 +59,7 @@ func SetBinlogRowImageMode(mode string, cnfDir string, includePartialJSON bool) if err != nil { return err } - if includePartialJSON { + if partialJSON { if !CIDBPlatformIsMySQL8orLater() { return fmt.Errorf("partial JSON values are only supported in MySQL 8.0 or later") } diff --git a/go/test/utils/binlog_test.go b/go/test/utils/binlog_test.go index 5b307595107..d8a0d6d4222 100644 --- a/go/test/utils/binlog_test.go +++ b/go/test/utils/binlog_test.go @@ -24,13 +24,13 @@ import ( "github.com/stretchr/testify/require" ) -// TestSetBinlogRowImageMode tests the SetBinlogRowImageMode function. +// TestSetBinlogRowImageOptions tests the SetBinlogRowImageOptions function. func TestUtils(t *testing.T) { tmpDir := "/tmp" cnfFile := fmt.Sprintf("%s/%s", tmpDir, BinlogRowImageCnf) // Test that setting the mode will create the cnf file and add it to the EXTRA_MY_CNF env var. - require.NoError(t, SetBinlogRowImageMode("noblob", tmpDir, false)) + require.NoError(t, SetBinlogRowImageOptions("noblob", false, tmpDir)) data, err := os.ReadFile(cnfFile) require.NoError(t, err) require.Contains(t, string(data), "binlog_row_image=noblob") @@ -39,18 +39,18 @@ func TestUtils(t *testing.T) { // Test that setting the mode and passing true for includePartialJSON will set both options // as expected. if CIDBPlatformIsMySQL8orLater() { - require.NoError(t, SetBinlogRowImageMode("noblob", tmpDir, true)) + require.NoError(t, SetBinlogRowImageOptions("noblob", true, tmpDir)) data, err = os.ReadFile(cnfFile) require.NoError(t, err) 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)) + require.Error(t, SetBinlogRowImageOptions("noblob", true, tmpDir)) } // Test that clearing the mode will remove the cnf file and the cnf from the EXTRA_MY_CNF env var. - require.NoError(t, SetBinlogRowImageMode("", tmpDir, false)) + require.NoError(t, SetBinlogRowImageOptions("", false, tmpDir)) require.NotContains(t, os.Getenv(ExtraCnf), BinlogRowImageCnf) _, err = os.Stat(cnfFile) require.True(t, os.IsNotExist(err)) diff --git a/go/vt/vttablet/tabletmanager/vreplication/framework_test.go b/go/vt/vttablet/tabletmanager/vreplication/framework_test.go index c7eee1e3092..c9c6451bbeb 100644 --- a/go/vt/vttablet/tabletmanager/vreplication/framework_test.go +++ b/go/vt/vttablet/tabletmanager/vreplication/framework_test.go @@ -183,10 +183,10 @@ func TestMain(m *testing.M) { exitCode := func() int { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - if err := utils.SetBinlogRowImageMode("full", tempDir, false); err != nil { + if err := utils.SetBinlogRowImageOptions("full", false, tempDir); err != nil { panic(err) } - defer utils.SetBinlogRowImageMode("", tempDir, false) + defer utils.SetBinlogRowImageOptions("", false, tempDir) cancel, ret := setup(ctx) if ret > 0 { return ret @@ -202,10 +202,10 @@ func TestMain(m *testing.M) { // We still run unit tests with MySQL 5.7, so we cannot add it to the cnf file // when using 5.7 or mysqld will fail to start. runPartialJSONTest = utils.CIDBPlatformIsMySQL8orLater() - if err := utils.SetBinlogRowImageMode("noblob", tempDir, runPartialJSONTest); err != nil { + if err := utils.SetBinlogRowImageOptions("noblob", runPartialJSONTest, tempDir); err != nil { panic(err) } - defer utils.SetBinlogRowImageMode("", tempDir, false) + defer utils.SetBinlogRowImageOptions("", false, tempDir) cancel, ret = setup(ctx) if ret > 0 { return ret diff --git a/go/vt/vttablet/tabletmanager/vreplication/replicator_plan.go b/go/vt/vttablet/tabletmanager/vreplication/replicator_plan.go index 952dde672d1..ec9de26d7e6 100644 --- a/go/vt/vttablet/tabletmanager/vreplication/replicator_plan.go +++ b/go/vt/vttablet/tabletmanager/vreplication/replicator_plan.go @@ -403,12 +403,11 @@ func (tp *TablePlan) applyChange(rowChange *binlogdatapb.RowChange, executor fun // This occurs when using partial JSON values as a result of mysqld using // binlog-row-value-options=PARTIAL_JSON. if len(afterVals[i].Raw()) == 0 { - // When using BOTH binlog_row_image=NOBLOB AND - // binlog_row_value_options=PARTIAL_JSON, if the JSON column was NOT updated - // then the JSON column is marked as partial and the diff is empty as a way - // to exclude it from the AFTER image. It still has the data bit set, however, - // even though it's not really present. So we have to account for this by - // unsetting the data bit so that the current JSON value is not lost. + // If the JSON column was NOT updated then the JSON column is marked as + // partial and the diff is empty as a way to exclude it from the AFTER image. + // It still has the data bit set, however, even though it's not really + // present. So we have to account for this by unsetting the data bit so + // that the column's current JSON value is not lost. setBit(rowChange.DataColumns.Cols, i, false) newVal = ptr.Of(sqltypes.MakeTrusted(querypb.Type_EXPRESSION, nil)) } else { @@ -486,10 +485,9 @@ func (tp *TablePlan) applyChange(rowChange *binlogdatapb.RowChange, executor fun case !isBitSet(rowChange.JsonPartialValues.Cols, jsonIndex): // We use the full AFTER value which we already have. case len(afterVals[i].Raw()) == 0: - // When using BOTH binlog_row_image=NOBLOB AND - // binlog_row_value_options=PARTIAL_JSON, if the JSON column was NOT updated - // then the JSON column is marked as partial and the diff is empty as a way - // to exclude it from the AFTER image. So we want to use the BEFORE image value. + // If the JSON column was NOT updated then the JSON column is marked as partial + // and the diff is empty as a way to exclude it from the AFTER image. So we + // want to use the BEFORE image value. beforeVal, err := vjson.MarshalSQLValue(bindvars["b_"+field.Name].Value) if err != nil { return nil, vterrors.Wrapf(err, "failed to convert JSON to SQL field value for %s.%s when building insert query", diff --git a/go/vt/vttablet/tabletmanager/vreplication/vplayer_flaky_test.go b/go/vt/vttablet/tabletmanager/vreplication/vplayer_flaky_test.go index b4a50ab529f..d6efcb5d5d4 100644 --- a/go/vt/vttablet/tabletmanager/vreplication/vplayer_flaky_test.go +++ b/go/vt/vttablet/tabletmanager/vreplication/vplayer_flaky_test.go @@ -1521,19 +1521,11 @@ func TestPlayerRowMove(t *testing.T) { // 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. +// images, meaning that binlog-row-image=NOBLOB and/or +// binlog-row-value-options=PARTIAL_JSON. func TestPlayerPartialImagesUpdatePK(t *testing.T) { - if !runNoBlobTest { - t.Skip("Skipping test as binlog_row_image=NOBLOB is not set") - } if !runPartialJSONTest { - t.Skip("Skipping test as binlog-row-value-options=PARTIAL_JSON is not set (most likely because the database type is not MySQL 8.0 or later)") + t.Skip("Skipping test as binlog_row_value_options=PARTIAL_JSON is not enabled") } defer deleteTablet(addTablet(100)) @@ -1577,7 +1569,9 @@ func TestPlayerPartialImagesUpdatePK(t *testing.T) { {"3", "{\"key3\": \"val3\"}", "blob data3"}, }, }, - { + } + if runNoBlobTest { + testCases = append(testCases, testCase{ input: `update src set jd=JSON_SET(jd, '$.color', 'red') where id = 1`, output: []string{"update dst set jd=JSON_INSERT(`jd`, _utf8mb4'$.color', _utf8mb4\"red\") where id=1"}, data: [][]string{ @@ -1585,7 +1579,19 @@ func TestPlayerPartialImagesUpdatePK(t *testing.T) { {"2", "{\"key2\": \"val2\"}", "blob data2"}, {"3", "{\"key3\": \"val3\"}", "blob data3"}, }, - }, + }) + } else { + testCases = append(testCases, testCase{ + input: `update src set jd=JSON_SET(jd, '$.color', 'red') where id = 1`, + output: []string{"update dst set jd=JSON_INSERT(`jd`, _utf8mb4'$.color', _utf8mb4\"red\"), bd=_binary'blob data' where id=1"}, + data: [][]string{ + {"1", "{\"key1\": \"val1\", \"color\": \"red\"}", "blob data"}, + {"2", "{\"key2\": \"val2\"}", "blob data2"}, + {"3", "{\"key3\": \"val3\"}", "blob data3"}, + }, + }) + } + testCases = append(testCases, []testCase{ { input: `update src set bd = 'new blob data' where id = 2`, output: []string{ @@ -1609,10 +1615,25 @@ func TestPlayerPartialImagesUpdatePK(t *testing.T) { {"12", "{\"key2\": \"val2\"}", "newest blob data"}, }, }, - { + }...) + if runNoBlobTest { + testCases = append(testCases, testCase{ input: `update src set id = id+10 where id = 3`, error: "binary log event missing a needed value for dst.bd due to not using binlog-row-image=FULL", - }, + }) + } else { + testCases = append(testCases, testCase{ + input: `update src set id = id+10 where id = 3`, + output: []string{ + "delete from dst where id=3", + "insert into dst(id,jd,bd) values (13,JSON_OBJECT(_utf8mb4'key3', _utf8mb4'val3'),_binary'blob data3')", + }, + data: [][]string{ + {"1", "{\"key1\": \"val1\", \"color\": \"red\"}", "blob data"}, + {"12", "{\"key2\": \"val2\"}", "newest blob data"}, + {"13", "{\"key3\": \"val3\"}", "blob data3"}, + }, + }) } for _, tc := range testCases { @@ -1776,7 +1797,7 @@ func TestPlayerTypes(t *testing.T) { {"2", "null", `{"name": null}`, "123", `{"a": [42, 100]}`, `{"foo": "bar"}`}, }, }} - if runNoBlobTest && runPartialJSONTest { + if runPartialJSONTest { // With partial JSON values we don't replicate the JSON columns that aren't // actually updated. testcases = append(testcases, testcase{