Re: [Maria-developers] [Commits] Rev 3772: MDEV-4645: Incorrect reads of frozen binlog events; FDE corrupted in relay log in file:///home/bell/maria/bzr/work-maria-10.0-MDEV-4548/
Sanja, Re: https://mariadb.atlassian.net/browse/MDEV-4645 Re: http://lists.askmonty.org/pipermail/commits/2013-July/005093.html I saw your commit on the commits list (sorry, wasn't on the list before the mail went out, so I can't reply directly to that message). A couple of points related to the commit: - You seem to have an error in the get_header_len, as it's implemented only in the base class, and you've left functions with the correct signature for it but the wrong names in the two frozen classes (they were left named has_fixed_minimal_header). This would mean the change is completely broken. :) - The change to get_header_len I think is in general fine, although it does alter the point of the whole thing -- there is again no way to exactly identify which events have frozen headers, programmatically (i.e. you can't differentiate them from events who just "happen" to have header_len = LOG_EVENT_MINIMAL_HEADER_LEN). I think that doesn't matter for this patch, but does defeat the point of what I was trying to add. - I don't really understand why you've made the hex printing code so complex and completely unreadable (IMHO). This is not performance-sensitive code and in my opinion should favor readability over performance. All you seem to have saved in this change is to avoid printing some static formatting characters ("#", "|", spaces, etc.) inside the loop. However, I can hardly even figure out what it's supposed to be doing now. - Additionally, you've changed the output slightly -- normally the ASCII character section prints its ending "|" immediately after the last character; it seems now to be fixed position (leaving extra spaces before "|". This differs from the output of hexdump, and it somewhat ambiguous/confusing. - You've mixed tabs and spaces. :( Regards, Jeremy
18.07.2013 19:39, Jeremy Cole пишет:
Sanja,
Re: https://mariadb.atlassian.net/browse/MDEV-4645
Re: http://lists.askmonty.org/pipermail/commits/2013-July/005093.html
I saw your commit on the commits list (sorry, wasn't on the list before the mail went out, so I can't reply directly to that message).
A couple of points related to the commit:
- You seem to have an error in the get_header_len, as it's implemented only in the base class, and you've left functions with the correct signature for it but the wrong names in the two frozen classes (they were left named has_fixed_minimal_header). This would mean the change is completely broken. :) - The change to get_header_len I think is in general fine, although it does alter the point of the whole thing -- there is again no way to exactly identify which events have frozen headers, programmatically (i.e. you can't differentiate them from events who just "happen" to have header_len = LOG_EVENT_MINIMAL_HEADER_LEN). I think that doesn't matter for this patch, but does defeat the point of what I was trying to add. - I don't really understand why you've made the hex printing code so complex and completely unreadable (IMHO). This is not performance-sensitive code and in my opinion should favor readability over performance. All you seem to have saved in this change is to avoid printing some static formatting characters ("#", "|", spaces, etc.) inside the loop. However, I can hardly even figure out what it's supposed to be doing now. - Additionally, you've changed the output slightly -- normally the ASCII character section prints its ending "|" immediately after the last character; it seems now to be fixed position (leaving extra spaces before "|". This differs from the output of hexdump, and it somewhat ambiguous/confusing. - You've mixed tabs and spaces. :(
Thank you a lot for this bug-report, I saw that the test suite do not cover the bug so did not pushed the fix hoping for your response. Test suite still highly appreciated :) (if it is possible to make it at all, I doubts) The cause of changes was: 1) avoid double copying string (formatting characters are not really matter) and extra memory allocation on the stack. 2) avoid 'if' in case when method can return value which we can use directly (looks better IMHO) 3) avoid extra virtual method call (they are quite expensive) Yes 1 and 3 are about performance but I deal with server code mostly. I'll change the patch, thank you yet another time.
Sanja, I could provide some very simple binary logs (from Google MySQL 5.1) which are internally consistent and have extra headers (they would have 31-byte headers instead of the usual 19-byte ones). That can at least pretty easily be used to make sure mysqlbinlog can parse them properly. Regards, Jeremy On Thu, Jul 18, 2013 at 11:07 AM, Oleksandr Byelkin <sanja@montyprogram.com>wrote:
18.07.2013 19:39, Jeremy Cole пишет:
Sanja,
Re: https://mariadb.atlassian.net/**browse/MDEV-4645<https://mariadb.atlassian.net/browse/MDEV-4645>
I saw your commit on the commits list (sorry, wasn't on the list before the mail went out, so I can't reply directly to that message).
A couple of points related to the commit:
- You seem to have an error in the get_header_len, as it's implemented
only in the base class, and you've left functions with the correct signature for it but the wrong names in the two frozen classes (they were left named has_fixed_minimal_header). This would mean the change is completely broken. :) - The change to get_header_len I think is in general fine, although it
does alter the point of the whole thing -- there is again no way to exactly identify which events have frozen headers, programmatically (i.e. you can't differentiate them from events who just "happen" to have header_len = LOG_EVENT_MINIMAL_HEADER_LEN). I think that doesn't matter for this patch, but does defeat the point of what I was trying to add. - I don't really understand why you've made the hex printing code so
complex and completely unreadable (IMHO). This is not performance-sensitive code and in my opinion should favor readability over performance. All you seem to have saved in this change is to avoid printing some static formatting characters ("#", "|", spaces, etc.) inside the loop. However, I can hardly even figure out what it's supposed to be doing now. - Additionally, you've changed the output slightly -- normally the ASCII
character section prints its ending "|" immediately after the last character; it seems now to be fixed position (leaving extra spaces before "|". This differs from the output of hexdump, and it somewhat ambiguous/confusing. - You've mixed tabs and spaces. :(
Thank you a lot for this bug-report, I saw that the test suite do not cover the bug so did not pushed the fix hoping for your response.
Test suite still highly appreciated :) (if it is possible to make it at all, I doubts)
The cause of changes was: 1) avoid double copying string (formatting characters are not really matter) and extra memory allocation on the stack. 2) avoid 'if' in case when method can return value which we can use directly (looks better IMHO) 3) avoid extra virtual method call (they are quite expensive) Yes 1 and 3 are about performance but I deal with server code mostly.
I'll change the patch, thank you yet another time.
Hi, Jeremy! 18.07.2013 21:12, Jeremy Cole пишет:
Sanja,
I could provide some very simple binary logs (from Google MySQL 5.1) which are internally consistent and have extra headers (they would have 31-byte headers instead of the usual 19-byte ones). That can at least pretty easily be used to make sure mysqlbinlog can parse them properly.
I'd really appreciate it. (I have no such big experience with the logs to get some ones easy).
Sanja, (Resending with the mailing list since this previous conversation was there.) Please see the attached binlog files: 1. binlog_none.binlog -- No extra headers. You could generate this yourself easily, but I included it for completeness, since it otherwise matches exactly the contents of the other binlogs. 2. binlog_checksum.binlog -- With Google's checksum enabled. All non-frozen events should have an additional 4-byte header. 3. binlog_group_id.binlog -- With Google's group_id enabled. All non-frozen events should have an additional 8-byte header. 4. binlog_group_id_checksum.binlog -- With both checksum and group_id enabled. All non-frozen events should have a total of 12 bytes of additional header. Regards, Jeremy On Thu, Jul 18, 2013 at 11:19 AM, Oleksandr Byelkin <sanja@montyprogram.com>wrote:
Hi, Jeremy!
18.07.2013 21:12, Jeremy Cole пишет:
Sanja,
I could provide some very simple binary logs (from Google MySQL 5.1) which are internally consistent and have extra headers (they would have 31-byte headers instead of the usual 19-byte ones). That can at least pretty easily be used to make sure mysqlbinlog can parse them properly.
I'd really appreciate it. (I have no such big experience with the logs to get some ones easy).
07.08.2013 23:57, Jeremy Cole пишет:
Sanja,
(Resending with the mailing list since this previous conversation was there.)
Thank you a lot! [skip]
Hi, Jeremy! 18.07.2013 19:39, Jeremy Cole пишет: [skip]
- You've mixed tabs and spaces. :(
The problem is that I do not see any \t printed. so where is that tabs you are talking about? [skip]
I meant that the code itself is sometimes indented with spaces, and sometimes tabs, not that you have printed with a mix of " " and "\t". Regards, Jeremy On Fri, Jul 19, 2013 at 4:05 AM, Oleksandr Byelkin <sanja@montyprogram.com>wrote:
Hi, Jeremy!
18.07.2013 19:39, Jeremy Cole пишет: [skip]
- You've mixed tabs and spaces. :(
The problem is that I do not see any \t printed. so where is that tabs you are talking about?
[skip]
Hi, Jeremy! 07.08.2013 23:58, Jeremy Cole пишет:
I meant that the code itself is sometimes indented with spaces, and sometimes tabs, not that you have printed with a mix of " " and "\t".
That is because of setup of my editor according to MySQL codding style, it just do not use tabs at all (but lets tabs stay) [skip]
participants (2)
-
Jeremy Cole
-
Oleksandr Byelkin