VReplication: Fixes for generated column handling #17107
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We were using the lowercase column name when building the list of columns to ignore but then using the case we got back from MySQL when checking the list. Signed-off-by: Matt Lord <[email protected]>
mattlord
added
Type: Bug
Component: VReplication
Backport to: release-18.0
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
|
vitess-bot
bot
added
NeedsBackportReason
mattlord
removed
NeedsDescriptionUpdate
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17107 +/- ##
==========================================
+ Coverage 67.15% 67.18% +0.03%
==========================================
Files 1571 1571
Lines 252250 252236 -14
==========================================
+ Hits 169409 169477 +68
+ Misses 82841 82759 -82 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
mattlord
changed the title
mattlord
removed
Backport to: release-18.0
mattlord
commented
Oct 30, 2024
Comment on lines
+596
to
+599
| resetCompression := mainClusterConfig.enableGTIDCompression() | ||
| defer resetCompression() | ||
| resetExperimentalFlags := setAllVTTabletExperimentalFlags() | ||
| defer resetExperimentalFlags() |
There was a problem hiding this comment.
This is unrelated to the PR issues, but I noticed that it was incorrect as I walked through this test.
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
mattlord
requested review from
harshit-gangal,
systay,
shlomi-noach and
timvaillancourt
as code owners
rohit-nayak-ps
approved these changes
Oct 31, 2024
| @@ -142,6 +142,11 @@ type TestQuery struct { | |||
| type TestRowChange struct { | |||
| before []string | |||
| after []string | |||
|
|
|||
| // If you need to customize the image you can use the raw types. | |||
|
Awesome, thanks for implementing this! |
dbussink
approved these changes
Oct 31, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
We were using the lowercase column name when building the map of columns to ignore in the copy phase — where we copy the filtered rows from a SELECT statement on the source — but then using the case we got back from MySQL when checking the map. h/t to @aluque01 for doing all of the hard work around this in the issue.
Without the code fix, the tests such as
TestBasicV2Workflowsfail with:The test changes added for that issue, then highlighted another issue with how we handled generated columns. Specifically when using the
binlog_row_image=noblobMySQL setting (we added experimental support for that in #12905). The problem was that we were removing the generated column from the table's list of columns entirely in the replication plan. This then threw off the indexes used in the bitmap contained in the non-FULL binlog images which denote which columns/fields have data in the event. Without the code fixes for this issue, the tests such asTestCellAliasVreplicationWorkflowwhich usebinlog_row_image=noblobwould fail when doing the vdiff after sharding tables in the product keyspace (examples here):This was because, in this specific case, we incorrectly thought that the binlog event that was generated here where only the
dec0column was updated in all existing rows, thus resulting in MySQL eliding theblbblob column from the ROW images in the binlog event (as it did not change), but VReplication incorrectly thinking it was there and thus assigning aNULLvalue (there was in fact no value in the event for it) to it in the resultingUPDATEstatement generated.Related Issue(s)
Checklist