-
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: use proper column collations in vstreamer #15313
Conversation
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
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
|
6d6e605
to
3aa6bbf
Compare
Signed-off-by: Matt Lord <[email protected]>
3aa6bbf
to
cc08784
Compare
Signed-off-by: Matt Lord <[email protected]>
bab31e3
to
6992ba1
Compare
6992ba1
to
99da069
Compare
Signed-off-by: Matt Lord <[email protected]>
99da069
to
c3a2b42
Compare
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.
Looks correct to me, although as I pointed out, storing only textual collations makes this needlessly complex.
// override for text based columns ONLY. This means that the | ||
// array position needs to be mapped to the ordered list of | ||
// text based columns in the table. | ||
ColumnCollationIDs []collations.ID |
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.
I think a lot of the code is going to be easier to write and easier to follow if this slice has one entry for each column and you simply fill the non-text columns with a Binary collation.
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.
Soooo... I agree! But unfortunately I'd have to perform the same logic to fill in those gaps as the binlog row metadata does not include the column's index or name. I'll think about it some more though.
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.
I explored this path, but we don't have access to the collation ENV in order to properly fill in the gaps here. So leaving it as-is for now, where users of this (vstreamer) have access to both the collation ENV and mysqld.
Signed-off-by: Matt Lord <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15313 +/- ##
==========================================
- Coverage 67.41% 65.43% -1.99%
==========================================
Files 1560 1561 +1
Lines 192752 193585 +833
==========================================
- Hits 129952 126678 -3274
- Misses 62800 66907 +4107 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
go/mysql/binlog_event_rbr.go
Outdated
pos = read | ||
|
||
// Read any text based column collation values provided in the optional metadata. | ||
//log.Errorf("DEBUG: Remaining optional metadata bytes for %s: %v", result.Name, data[pos:]) |
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.
All of these DEBUG logging statements will be removed before merging.
0be3a59
to
f1c7840
Compare
Signed-off-by: Matt Lord <[email protected]>
f1c7840
to
8f6f957
Compare
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 ❤️
Description
In order to address the problem described in #15338, this PR does the following:
TableMapEvent
s when presentRelated Issue(s)
Checklist