-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
VReplication: Support binlog_row_value_options=PARTIAL_JSON #17345
base: main
Are you sure you want to change the base?
Conversation
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
1859876
to
a44e7e5
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17345 +/- ##
==========================================
- Coverage 67.64% 67.61% -0.04%
==========================================
Files 1582 1582
Lines 254000 254241 +241
==========================================
+ Hits 171826 171900 +74
- Misses 82174 82341 +167 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
a44e7e5
to
c9cf71c
Compare
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
181d05e
to
7b75ed6
Compare
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
49ad080
to
30ce7d4
Compare
Signed-off-by: Matt Lord <[email protected]>
30ce7d4
to
bd3aa67
Compare
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
7fe38ad
to
1980b89
Compare
Signed-off-by: Matt Lord <[email protected]>
734d56a
to
7b25395
Compare
I'm thinking that this is worth a note in the 22.0 summary. I'm also thinking that it's worth adding a blurb somewhere on the website about our support for more efficient replication:
I'd like to know what reviewers think. If others disagree, then they or I can remove the labels for docs and release notes (I just added them now). Thanks! |
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
40c884a
to
8ad1fc0
Compare
@@ -111,7 +111,7 @@ jobs: | |||
|
|||
- name: Upload coverage reports to codecov.io | |||
if: steps.changes.outputs.changed_files == 'true' | |||
uses: codecov/codecov-action@v4 | |||
uses: codecov/codecov-action@015f24e6818733317a2da2edd6290ab26238649a # https://github.com/codecov/codecov-action/releases/tag/v5.0.7 |
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.
This didn't help with the periodic failures, but good to upgrade anyway as the latest GA is 5.1.
if !isBitSet(rowChange.DataColumns.Cols, i) { | ||
return nil, vterrors.Errorf(vtrpcpb.Code_INTERNAL, | ||
"binary log event missing a needed value for %s.%s due to not using binlog-row-image=FULL; you will need to re-run the workflow with binlog-row-image=FULL", | ||
tp.TargetName, field.Name) | ||
} |
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.
This fixes a bug we had with NOBLOB support. Without this, the blob/text field value would be silently lost (detected by vdiff). Since NOBLOB support is experimental (even in v22), I do not think we need to backport it.
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
if field.Type == querypb.Type_JSON { | ||
var jsVal *sqltypes.Value | ||
if vals[n].IsNull() { // An SQL NULL and not an actual JSON value | ||
jsVal = &sqltypes.NULL | ||
} else { // A JSON value (which may be a JSON null literal value) | ||
jsVal, err = vjson.MarshalSQLValue(vals[n].Raw()) | ||
if err != nil { | ||
return nil, err | ||
} | ||
} | ||
bindVar, err = tp.bindFieldVal(field, jsVal) | ||
} else { | ||
bindVar, err = tp.bindFieldVal(field, &vals[n]) | ||
} |
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.
This was a bug exposed in the VPlayerBatching, which was enabled by default in v22. Prior to v22 it was experimental and thus I don't think we need to backport this fix.
1. Use iota for op type 2. Optimize binary diff parsing when 0 bytes (don't allocate) Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
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.
Nice work!
Now we're covering all JSON types: - string - number - object (JSON object) - array - boolean - null Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Description
This PR adds support for
--binlog-row-value-options=PARTIAL_JSON
. You can read more about this here.This can offer big wins in specific cases. For example...
$.price
from 99.99 to 102.99 (inflation) and the original JSON document is 5MiB.binlog_row_value_options=PARTIAL_JSON
the binlog event will have the full 5MiB document in the BEFORE (original) and AFTER (updated) images. So 10MiB now.JSON_REPLACE
in this case, along with the path of$.price
and the value of 102.99. So we've cut the bytes stored on disk, sent over the wire, and processed in memory by ~ 1/2.Now if you imagine that instead of updating a single row, you updated 10,000 rows... let's say to remove the
$.on_sale
flag on the documents that had it set (Black Friday is over), you can see the big savings that comes with it.Important
With this option enabled, JSON columns that are NOT updated when a row is modified are elided from the AFTER images (similar to the
binlog_row_image=NOBLOB
behavior withBLOB
/TEXT
columns). So this also offers a big potential savings as you may be updating 10,000 rows in a table that happens to have a JSON column, but you're only changing some other column value(s).Manual test:
With the final result:
Related Issue(s)
Checklist