Re: [Maria-developers] d9913834ceb: MDEV-14014 Multi-Slave Replication Fail: bogus data in log event
Hi, andrei.elkin! On May 30, andrei.elkin@pp.inet.fi wrote:
revision-id: d9913834cebe4ed8494b3d76762cd882788a8fd5 (mariadb-10.1.33-28-gd9913834ceb) parent(s): c1698e8dc50f0c342c1a6886426ba1d43396cd1e author: Andrei Elkin committer: Andrei Elkin timestamp: 2018-05-30 20:06:44 +0300 message:
MDEV-14014 Multi-Slave Replication Fail: bogus data in log event
MDEV-7257 made a dump thread to read from binlog concurrently with writers as long as the read bytes are below a water-mark (MYSQL_BIN_LOG::binlog_end_pos). However it appeared to be possible a dump thread reader reach out for bytes past the water mark through a feature of IO_CACHE that fills in the internal buffer and while doing so it could read what the reader is not supposed to see (the bytes above MYSQL_BIN_LOG::binlog_end_pos).
The issue is fixed with constraining the IO_CACHE buffer fill to respect the watermark. An added test simulates potentially unconstrained buffer fill and an assert guards this is not the case anymore.
What about a much simpler one-liner fix: --- a/sql/sql_repl.cc +++ b/sql/sql_repl.cc @@ -2549,6 +2549,7 @@ static int send_events(binlog_send_info *info, IO_CACHE* return 1; info->last_pos= linfo->pos; + log->end_of_file= end_pos; error= Log_event::read_log_event(log, packet, info->fdev, opt_master_verify_checksum ? info->current_checksum_alg : BINLOG_CHECKSUM_ALG_OFF); It doesn't change IO_CACHE structure and doesn't add any overhead to the IO_CACHE. Did you already test that your commit actually fixes the issue? Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Sergei, hello.
Hi, andrei.elkin!
On May 30, andrei.elkin@pp.inet.fi wrote:
revision-id: d9913834cebe4ed8494b3d76762cd882788a8fd5 (mariadb-10.1.33-28-gd9913834ceb) parent(s): c1698e8dc50f0c342c1a6886426ba1d43396cd1e author: Andrei Elkin committer: Andrei Elkin timestamp: 2018-05-30 20:06:44 +0300 message:
MDEV-14014 Multi-Slave Replication Fail: bogus data in log event
MDEV-7257 made a dump thread to read from binlog concurrently with writers as long as the read bytes are below a water-mark (MYSQL_BIN_LOG::binlog_end_pos). However it appeared to be possible a dump thread reader reach out for bytes past the water mark through a feature of IO_CACHE that fills in the internal buffer and while doing so it could read what the reader is not supposed to see (the bytes above MYSQL_BIN_LOG::binlog_end_pos).
The issue is fixed with constraining the IO_CACHE buffer fill to respect the watermark. An added test simulates potentially unconstrained buffer fill and an assert guards this is not the case anymore.
What about a much simpler one-liner fix:
--- a/sql/sql_repl.cc +++ b/sql/sql_repl.cc @@ -2549,6 +2549,7 @@ static int send_events(binlog_send_info *info, IO_CACHE* return 1;
info->last_pos= linfo->pos; + log->end_of_file= end_pos;
That's nice! I could not guess out that so need parameter was always at my disposal.
error= Log_event::read_log_event(log, packet, info->fdev, opt_master_verify_checksum ? info->current_checksum_alg : BINLOG_CHECKSUM_ALG_OFF);
It doesn't change IO_CACHE structure and doesn't add any overhead to the IO_CACHE.
Very true, and even more - 'end_of_file' of the read cache seems to have found usability beyond being a constant.
Did you already test that your commit actually fixes the issue?
Like I said, I did not try after we found it's NFS binlog. I've done it right now, but my attempt lasted minutes while in the user's case apart of anything specific they needed hours sometimes. I am committing a new patch which sustains the simulation test of the old one. Cheers, Andrei
Hi, Andrei! On Jun 12, andrei.elkin@pp.inet.fi wrote:
Did you already test that your commit actually fixes the issue?
Like I said, I did not try after we found it's NFS binlog. I've done it right now, but my attempt lasted minutes while in the user's case apart of anything specific they needed hours sometimes.
I don't understand, sorry. Were you able to repeat the user's error in your NFS tests?
I am committing a new patch which sustains the simulation test of the old one.
I don't quite like the test case. It may have nothing to do with the user's bug - we still don't know why it happens and we weren't able to repeat it, as far as I understand. If you just want to test that IO_CACHE doesn't read beyond end_of_file, it's a good thing to test, I agree. But it should be a unit test. So, please, remove this replication test and put a unit test for end_of_file into unittest/sql/mf_iocache-t.cc Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Sergei,
Hi, Andrei!
On Jun 12, andrei.elkin@pp.inet.fi wrote:
Did you already test that your commit actually fixes the issue?
Like I said, I did not try after we found it's NFS binlog. I've done it right now, but my attempt lasted minutes while in the user's case apart of anything specific they needed hours sometimes.
I don't understand, sorry. Were you able to repeat the user's error in your NFS tests?
Sorry, I wanted to say that I tried yesterday to reproduce the bug with few minutes testing against the base sources, and resultlessly. It could be that I needed to run for a lot more time, or only their NFS is prone to the bug.
I am committing a new patch which sustains the simulation test of the old one.
I don't quite like the test case. It may have nothing to do with the user's bug - we still don't know why it happens and we weren't able to repeat it, as far as I understand.
I could not reproduce. But let us see how the user will do. Personally I have 0% of doubt in that the fixes have made it.
If you just want to test that IO_CACHE doesn't read beyond end_of_file, it's a good thing to test, I agree. But it should be a unit test. So, please, remove this replication test and put a unit test for end_of_file into unittest/sql/mf_iocache-t.cc
I'm considering. Thanks! Andrei
participants (2)
-
andrei.elkin＠pp.inet.fi -
Sergei Golubchik