MDEV-20749: Improve error reporting of mysqlbinlog when used with --flashback#4767
MDEV-20749: Improve error reporting of mysqlbinlog when used with --flashback#4767monthdev wants to merge 1 commit intoMariaDB:mainfrom
Conversation
gkodinov
added
the
External Contribution
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
Thank you for your contribution! This is a preliminary review.
I am afraid I need to insist on a regression test. If you've debugged this anyhow you should have a working test case. Even if it's some manually generated binary sequence.
Last resort this could be short binlog file from MySQL.
Also, please squash your commits into a single commit and add a commit message to it that complies with CODING_STANDARDS.md
sql/log_event.h
Outdated
| ERR_RBR_TO_SBR = 4 /**< daisy-chanining RBR to SBR not allowed */ | ||
| }; | ||
|
|
||
| enum enum_row_decode_error |
There was a problem hiding this comment.
I'd just print the error right into the decoder function itself. In this way I'd be able to print things like offsets etc.
Or at least pass a const char ** to return the error into instead of returning a code that then needs to be translated.
Note also that the current implementation doesn't even try to satisfy Simon's wish list:
e.g.
which event type was not liked
what were you expecting to see, what did you see?
adding the "binlog pos" when reporting such an error would also be good as the other tools can be used to look at the event and maybe determine what's wrong.
There was a problem hiding this comment.
Okay, that makes sense. I will do it that way!
|
Hi! I got some help on Zulip and made some MTR tests that grep for the improved error message. |
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
The tests are looking good. I was hoping for a more functional test: i.e. an actual sample of bad binlog. But simulating it is OK too. Please also consider addressing the other points in my review.
|
@gkodinov I could look into an actual bad binlog, but I don't know what could trigger this failure path. I have already tried the first suggestion in this thread #general > MDEV-20749, but it did not hit the failure path. I resorted to the second suggestion to use the And yes, I will come back with changes for richer debug output! |
|
@gkodinov I tried to check off most of what was asked for regarding the debug output. Let me know if this is closer to how you wanted it! |
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
LGTM after squashing all commits into a single one with a compliant commit message.
Please stand by for the final review.
Print row decode failures directly in calc_row_event_length() so the diagnostic can include decoder-local context such as image kind, offsets, and field/type metadata. Pass event context from the flashback caller so the message can report the row event type and end_log_pos. Add MTR coverage for truncated row image and bad field length failures using DBUG fault injection in mariadb-binlog.
|
My understanding of the squashed commit is we need the matching issue title and paragraph description underneath. Please let me know if I did it wrong! |
3 participants
Address MDEV-20749
Improves the flashback row-decode error message by distinguishing truncated row images from bad field-length decoding.
This patch is not accompanied by an automated regression test yet. If there is a minimal sample binlog or reduced reproduction method, I can add an MTR regression test.