-
Notifications
You must be signed in to change notification settings - Fork 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
Support for PARTIAL_UPDATE_ROWS_EVENT binlog event and PARTIAL_JSON mode #774
Conversation
Hi, is this still a WIP? |
I checked it in our prod and now it looks working and correct :) |
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.
rest lgtm 👍
@@ -856,6 +877,39 @@ type RowsEvent struct { | |||
ignoreJSONDecodeErr bool | |||
} | |||
|
|||
// enum class enum_row_image_type { WRITE_AI, UPDATE_BI, UPDATE_AI, DELETE_BI }; |
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.
Does AI and BI represent "after image" and "before image"? A bit unfamiliar
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.
Yes, I preserve naming from mysql sources. I can rename it for more clarity if you think so.
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 can see their meanings in the linked MySQL code now 😄
for i := 0; i < int(e.ColumnCount); i++ { | ||
isPartial := isPartialJsonUpdate && |
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.
seems isPartialIndex
is same as i
? and we can move creating isPartial
closer to e.decodeValue(..., isPartial)
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.
seems isPartialIndex is same as i?
No, it's an index inside a partial bitmap and we increment it only for partial fields.
can move creating isPartial closer to e.decodeValue(..., isPartial)
Partial bitmap checking must prepend to null bitmap checking.
I'll update this code for more clarity.
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 just forget a false table.ColumnType[i] == MYSQL_TYPE_JSON
will short circuit isBitSetIncr(partialBitmap, &partialBitmapIndex)
,
replication/row_event.go
Outdated
diff, err = e.decodeJsonPartialBinary(data[meta:n]) | ||
if err == nil { | ||
if diff == nil { | ||
v = []byte{} |
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.
Can we add a "JsonDiffOperationNoOp" JsonDiff, or tell the caller nil is no-op for JsonDiff? It's a bit ambiguous that we return a []byte{}
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.
MySQL doesn't allow any other variants. So I just removed a nil JsonDiff :)
for i := 0; i < int(e.ColumnCount); i++ { | ||
isPartial := isPartialJsonUpdate && |
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 just forget a false table.ColumnType[i] == MYSQL_TYPE_JSON
will short circuit isBitSetIncr(partialBitmap, &partialBitmapIndex)
,
@@ -856,6 +877,39 @@ type RowsEvent struct { | |||
ignoreJSONDecodeErr bool | |||
} | |||
|
|||
// enum class enum_row_image_type { WRITE_AI, UPDATE_BI, UPDATE_AI, DELETE_BI }; |
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 can see their meanings in the linked MySQL code now 😄
OK. I'll make last checks and merge it. |
ping @atercattus looking forward to this commit |
@lance6716 I'm done with this PR |
82c1282
to
f594b7e
Compare
f594b7e
to
b096301
Compare
cdd955d
to
3d2be20
Compare
I accidentally messed up the branch, but fixed it through force push :) |
Implementation for
PARTIAL_UPDATE_ROWS_EVENT
binlog event.PARTIAL_UPDATE_ROWS_EVENT
is an extension ofUPDATE_ROWS_EVENT
, allowing partial values according to binlog_row_value_options in mysql8. Valuebinlog_row_value_options=PARTIAL_JSON
allows for mysql store only minimal diff from the previous column value.Tested for mysql 5.7 and 8,
binlog_row_image
FULL
andMINIMAL
, withbinlog_row_value_options
and without.Known limitations:
BinlogParser
returnsRowsEvent
where a fieldRows
contains instances of a new type:And a user can apply a patch in any way. Why? In
binlog_row_image=MINIMAL
mode a mysql doesn't write the previous value (BEFORE_IMAGE) to binlog. So the code cannot apply the path to an unknown value.For example:
After this
PARTIAL_UPDATE_ROWS_EVENT
event will contain onlyenum_json_diff_operation::REPLACE
,$.ab
, and["ab_updatedccc"]'
. And nothing all.