VReplication Workflows: support noblob binlog_row_image format for MoveTables and Reshard #12905

rohit-nayak-ps · 2023-04-16T19:02:31Z

Description

Motivation

Currently, the full binlog_row_image is a hard requirement for using VReplication within Vitess. This is because
there are certain Online DDL and Materialize workflows which will not work without full.

For example, if the target table has a different primary key and doesn't have the existing primary key column, a partial
row image for a delete may not have the new primary key column to identify the target column.

However, if a row has blobs which are huge but rarely updated, we end up with a huge waste of resources (storage,
network bandwidth, cpu), in the replication (vplayer) phase, as compared to a noblob binlog_row_image.

Many common workflows like MoveTables and Reshard and indeed many Online DDL migrations will work fine. Hence, we should look at relaxing the requirement for full for such workflows.

Approach

For phase 1, to minimize impacted code and risk, we will support noblob for MoveTables and Reshard.
minimal will provide fewer benefits and requires more intrusive code changes.

VStreamer

We continue to send the same number of columns in VStreamer, but missing columns are sent as null using length:-1
for those columns. This implies that the contract on the vstreamer's consumers stays the same. An additional attribute
in the proto sends the null column bitmap. To optimize, we do this only if the flag is set and if it is not a
complete image.

VPlayer

The copy phase will do a full copy, using direct select queries against the source, so there is no impact of the binlog
row image mode.

For the VPlayer we need to generate different Insert and Update queries for different combinations of columns that are
used.

For partial queries, we compute the required ParsedQuery at runtime when we encounter one. We store it in a cache, one
each for Insert and Update, using the bitmap as the key. So we only compute it once and we then bind the variables to
this query based on the received bitmap.

Other design options and trade-offs are discussed below.

Other design options

Some other options that were considered before choosing the current approach.

Use single parsed query for update statements

We use the single Update parsed query and remove the columns by parsing the generated sql string, dropping the missing
columns and serialize the ast again.

Benefit
We don't have multiple partial update parsed queries. We cannot predict how many of such queries can exist:
theoretically we could have an combinatorial explosion depending on the number of update templates used by the app.
ORMs, for example, could generate many such depending on which column is modified.

Problem
Reparsing and regenerating the query dynamically will have a significant impact especially with a large update qps.

Status
We will export a metric to keep track of the cache size and reevaluate this later on. In general it is not expected to
require a huge number of cache entries.

Use `default` in insert statements

For insert statements we can continue to use the single parsed Insert query, but use the default keyword for columns
which are not present.

Benefit
Only single parsed query for insert statements

Problem
We will need to rewrite the already created query, for missing columns, in the TrackedBuffer, to replace the existing
bind variables format strings to (say) %q and then use default as the bind variable value.This needs to be done for every partial query and consequently result in non-trivial performance penalties.

Status
Same as above

Related Issue(s)

Fixes: Support "minimal" binlog_row_image for vreplication #12653

Checklist

"Backport to:" labels have been added if this change should be back-ported
Tests were added or are not required
Did the new or modified tests pass consistently locally and on the CI
Documentation was added or is not required

…e2e test Signed-off-by: Rohit Nayak <rohit@planetscale.com>

…e bitmap in RowChanges. Optimize if there are no missing columns. Add vstreamer test. Signed-off-by: Rohit Nayak <rohit@planetscale.com>

…so test for noblob Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

…nit tests. Dynamically cache partial update queries. Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

… noblob binlog row image Signed-off-by: Rohit Nayak <rohit@planetscale.com>

…d flaky binlog_row_image in tests: need to diag/fix Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

…al tests for new changes. Set binlog mode dynamically instead of in test-suite.cnf Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

vitess-bot · 2023-04-21T20:25:00Z

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

shlomi-noach · 2023-04-24T05:14:43Z

Related and notable: #12921 ; VReplication dos not have full support for column prefix indexes. As such, #12921 rejects them. The relevance is that a MySQL index cannot cover a blob column without a prefix -- meaning VReplication does not support blob PRIMARY KEY in the first place.

This means it is safe to use binlog_row_image=noblob for vitess Online DDL; either the migration is accepted and runs correctly, or is rejected.

shlomi-noach

Just incredible.

proto/binlogdata.proto

go/vt/vttablet/tabletserver/vstreamer/engine.go

shlomi-noach · 2023-04-24T05:51:40Z

go/vt/vtgate/tabletgateway_flaky_test.go

@@ -209,7 +209,7 @@ func TestGatewayBufferingWhileReparenting(t *testing.T) {
 	hc.SetTabletType(replicaTablet, topodatapb.TabletType_PRIMARY)
 	hc.SetServing(replicaTablet, true)
 	hc.Broadcast(replicaTablet)
-
+	time.Sleep(10 * time.Second) // temp. hack to see if CI failures are due to delays here


TODO: remove

go/vt/vttablet/tabletmanager/vreplication/replicator_plan.go

… Improve comments Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

mattlord

LGTM. Nice work on this, Rohit! I only had minor comments that you can address as you feel is best.

changelog/17.0/17.0.0/summary.md

mattlord · 2023-04-25T18:38:47Z

go/test/endtoend/vreplication/helper_test.go

@@ -533,6 +534,17 @@ func getDebugVar(t *testing.T, port int, varPath []string) (string, error) {
 	return string(val), nil
 }

+func getDebugVars(t *testing.T, port int) map[string]any {


I think I had added this or similar elsewhere too. Might be worth moving it to the cluster package (if it's not already there). In this case, we could move it to go/test/endtoend/cluster/vttablet_process.go or make it more generic and just take a host:port like you are here.

Thanks, changed it to use GetVars() from VTTabletProcess .

go/test/endtoend/vreplication/helper_test.go

mattlord · 2023-04-25T18:51:21Z

proto/binlogdata.proto

@@ -306,13 +306,20 @@ enum VEventType {
  COPY_COMPLETED = 20;
 }

+message Bitmap {


This message name feels too generic to me. Can we call it RowImageBitmap or something? In fact, it might be worth just embedding it directly in RowChange as it probably has no value/usage outside of that right? Then the name would be perfectly fine as it becomes a child. For example: https://github.com/vitessio/vitess/pull/12622/files#diff-13fb22ef91d4bfe00ede706cafee2757699abdb1358c14519b747e4eaf9e34b4R1314-R1320

mattlord · 2023-04-25T20:22:58Z

go/vt/vttablet/tabletmanager/vreplication/table_plan_partial.go

+func (tpb *tablePlanBuilder) createPartialUpdateQuery(dataColumns *binlogdatapb.Bitmap) *sqlparser.ParsedQuery {
+	bvf := &bindvarFormatter{}
+	buf := sqlparser.NewTrackedBuffer(bvf.formatter)
+	buf.Myprintf("update %v set ", tpb.name)


If it's not already, I think we should make it an IdentifierCS for the potential escaping.

colexpr already uses IndentifierCIs

go/vt/vttablet/tabletmanager/vreplication/table_plan_partial.go

mattlord · 2023-04-25T20:28:37Z

go/vt/vttablet/tabletmanager/vreplication/table_plan_partial.go

+		if !isBitSet(dataColumns.Cols, ind) {
+			continue
+		}
+		buf.Myprintf("%s%v", separator, cexpr.colName)


We should probably escape the column names (if needed) too, no?

go/vt/vttablet/tabletmanager/vreplication/table_plan_partial.go

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

vitess-bot · 2023-04-26T10:19:28Z

I was unable to backport this Pull Request to the following branches: release-16.0.

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Backport: VReplication Workflows: support noblob binlog_row_image format for MoveTables and Reshard vitessio#12905

…veTables and Reshard (vitessio#12905) (vitessio#2022) * Initial POC of binlog_row_image=noblob. Working player unit test and e2e test * Add vttablet flag to allow non-full binlog row image. Send only single bitmap in RowChanges. Optimize if there are no missing columns. Add vstreamer test. * Remove noblob settings from cnf files and modify test framework to also test for noblob * Set binlog row image dynamically for e2e tests too * Validate that noblob is only used with MoveTables/Reshard. More e2e/unit tests. Dynamically cache partial update queries. * Minor refactor * Unit tests: fake workflow type as MoveTables so that we can test with noblob binlog row image * Also support partial inserts. Needed to update test-suite.cnf to avoid flaky binlog_row_image in tests: need to diag/fix * Don't throw errors for noblob, for unit tests * Fix flags help * Add metrics for cache size and number of partial queries * Refactor flags * Fix tests * Fail vstreamer if binlog mode is minimal. Fix UpdateVSchema and Minimal tests for new changes. Set binlog mode dynamically instead of in test-suite.cnf * Fix flags help * Extra cnf was not being set correctly in tests * More tests * We were testing for partial queries all the time instead of only noblob * More comments and minor refactor * make proto * Minor refactor, more comments * Update release summary * Fix cleanup after vplayer tests * Cleanup tests * Address review comments. Add vstreamer/vplayer tests for unique keys. Improve comments * Make proto * Address review comments --------- Signed-off-by: Rohit Nayak <rohit@planetscale.com> Co-authored-by: Rohit Nayak <57520317+rohit-nayak-ps@users.noreply.github.com>

github-actions bot added this to the v17.0.0 milestone Apr 16, 2023

rohit-nayak-ps added Type: Feature Request Component: VReplication Do Not Merge release notes (needs details) This PR needs to be listed in the release notes in a dedicated section (deprecation notice, etc...) NeedsWebsiteDocsUpdate What it says labels Apr 16, 2023

rohit-nayak-ps added 14 commits April 20, 2023 14:15

Initial POC of binlog_row_image=noblob. Working player unit test and …

5bcf4e6

…e2e test Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Add vttablet flag to allow non-full binlog row image. Send only singl…

174dafd

…e bitmap in RowChanges. Optimize if there are no missing columns. Add vstreamer test. Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Remove noblob settings from cnf files and modify test framework to al…

8ff9c7a

…so test for noblob Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Set binlog row image dynamically for e2e tests too

1aaf708

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Validate that noblob is only used with MoveTables/Reshard. More e2e/u…

bc33fe4

…nit tests. Dynamically cache partial update queries. Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Minor refactor

95da3d0

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Unit tests: fake workflow type as MoveTables so that we can test with…

9618d46

… noblob binlog row image Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Also support partial inserts. Needed to update test-suite.cnf to avoi…

4a759c2

…d flaky binlog_row_image in tests: need to diag/fix Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Don't throw errors for noblob, for unit tests

f4c117d

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Fix flags help

97eea18

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Add metrics for cache size and number of partial queries

3e4af81

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Refactor flags

3a84213

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Fix tests

dde8fbe

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Fail vstreamer if binlog mode is minimal. Fix UpdateVSchema and Minim…

2c8f3c5

…al tests for new changes. Set binlog mode dynamically instead of in test-suite.cnf Signed-off-by: Rohit Nayak <rohit@planetscale.com>

rohit-nayak-ps force-pushed the rn-noblob branch from aafa559 to 6c40c5d Compare April 20, 2023 12:23

rohit-nayak-ps added 6 commits April 20, 2023 14:23

Fix flags help

f3af840

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Extra cnf was not being set correctly in tests

59a23ef

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

More tests

6c40c5d

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

We were testing for partial queries all the time instead of only noblob

929cb8d

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

More comments and minor refactor

c101118

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

make proto

cd3aef1

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

rohit-nayak-ps changed the title ~~Experimental: support noblob binlog_row_image format for MoveTables and Reshard~~ VReplication Workflows: support noblob binlog_row_image format for MoveTables and Reshard Apr 21, 2023

Minor refactor, more comments

c2906a3

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

rohit-nayak-ps removed the Do Not Merge label Apr 21, 2023

rohit-nayak-ps requested a review from GuptaManan100 as a code owner April 21, 2023 20:24

rohit-nayak-ps removed the request for review from systay April 21, 2023 20:25

rohit-nayak-ps added the Backport to: release-16.0 label Apr 21, 2023

rohit-nayak-ps added 3 commits April 23, 2023 20:28

Update release summary

a0aef4e

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Fix cleanup after vplayer tests

3ce7274

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Cleanup tests

75c1221

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

rohit-nayak-ps removed release notes (needs details) This PR needs to be listed in the release notes in a dedicated section (deprecation notice, etc...) NeedsWebsiteDocsUpdate What it says labels Apr 23, 2023

shlomi-noach reviewed Apr 24, 2023

View reviewed changes

rohit-nayak-ps added Backport to: release-16.0 and removed Backport to: release-16.0 labels Apr 24, 2023

rohit-nayak-ps added 2 commits April 24, 2023 10:37

Address review comments. Add vstreamer/vplayer tests for unique keys.…

6bf172d

… Improve comments Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Make proto

84cc6d5

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

shlomi-noach approved these changes Apr 25, 2023

View reviewed changes

mattlord approved these changes Apr 25, 2023

View reviewed changes

Address review comments

6ed670f

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

rohit-nayak-ps merged commit 11a5c03 into vitessio:main Apr 26, 2023

rohit-nayak-ps deleted the rn-noblob branch April 26, 2023 10:19

rohit-nayak-ps removed the Backport to: release-16.0 label Apr 26, 2023

rohit-nayak-ps added a commit to planetscale/vitess that referenced this pull request May 9, 2023

Backport vitessio#12905 to v15. e2e tests are pending

1d3c59c

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

rohit-nayak-ps mentioned this pull request May 9, 2023

Backport: VReplication Workflows: support noblob binlog_row_image format for MoveTables and Reshard #12905 planetscale/vitess#78

Merged

4 tasks

rohit-nayak-ps added a commit to planetscale/vitess that referenced this pull request May 10, 2023

Merge pull request #78 from planetscale/noblob

1802428

Backport: VReplication Workflows: support noblob binlog_row_image format for MoveTables and Reshard vitessio#12905

This was referenced May 16, 2023

Flaky vreplication tests due to race in resetting the global query channel. #13092

Closed

Flaky vplayer tests: temporarily disable noblob variant #13100

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VReplication Workflows: support noblob binlog_row_image format for MoveTables and Reshard #12905

VReplication Workflows: support noblob binlog_row_image format for MoveTables and Reshard #12905

rohit-nayak-ps commented Apr 16, 2023 •

edited

Loading

vitess-bot bot commented Apr 21, 2023

shlomi-noach commented Apr 24, 2023 •

edited

Loading

shlomi-noach left a comment

shlomi-noach Apr 24, 2023

rohit-nayak-ps Apr 24, 2023

mattlord left a comment

mattlord Apr 25, 2023

rohit-nayak-ps Apr 26, 2023

mattlord Apr 25, 2023

mattlord Apr 25, 2023

rohit-nayak-ps Apr 26, 2023

mattlord Apr 25, 2023

vitess-bot bot commented Apr 26, 2023

VReplication Workflows: support noblob binlog_row_image format for MoveTables and Reshard #12905

VReplication Workflows: support noblob binlog_row_image format for MoveTables and Reshard #12905

Conversation

rohit-nayak-ps commented Apr 16, 2023 • edited Loading

Description

Motivation

Approach

VStreamer

VPlayer

Other design options

Use single parsed query for update statements

Use default in insert statements

Related Issue(s)

Checklist

vitess-bot bot commented Apr 21, 2023

Review Checklist

General

If a new flag is being introduced:

If a workflow is added or modified:

Bug fixes

Non-trivial changes

New/Existing features

Backward compatibility

shlomi-noach commented Apr 24, 2023 • edited Loading

shlomi-noach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattlord left a 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

Choose a reason for hiding this comment

vitess-bot bot commented Apr 26, 2023

rohit-nayak-ps commented Apr 16, 2023 •

edited

Loading

Use `default` in insert statements

shlomi-noach commented Apr 24, 2023 •

edited

Loading