[Maria-developers] Comments/thoughts on patch
Hi Kristian, I wrote attached patch (so that i would understand what code actually does) and found myself while refactoring also doing a change, namely that i removed the check for server_id in a couple of places. it still passes all tests... could you comment on this, or write(outline) a testcase that fails with my patch. i think functionality that i might have removed would be if you write have 2 mysqld M1 & M2 and your replicate both way...and you *don't* use domain_id...and you write concurrently on both mysqld (but maybe disjoint data ??). feedback/comments appreciated. /Jonas ps. patch applies to f4f37533a09b9776e8d5ac3f3a27957f553c9043
Jonas Oreland <jonaso@google.com> writes:
I wrote attached patch (so that i would understand what code actually does) and found myself while refactoring also doing a change, namely that i removed the check for server_id in a couple of places.
it still passes all tests... could you comment on this, or write(outline) a testcase that fails with my patch.
I think the check for server_id is needed in case of out-of-order binlogs. The easiest / most common way to do this is to do rouge transactions on the slave: Master: CREATE TABLE t1 # GTID 0-1-1 INSERT INTO t1 # GTID 0-1-2 Slave1: CREATE TABLE t_local # GTID 0-2-3 INSERT INTO t_local # GTID 0-2-4 Master: INSERT INTO t1 # GTID 0-1-3 UPDATE t1 # GTID 0-1-4 DELETE FROM t1 # GTID 0-1-5 Now the binlog on Slave1 contains: GTID 0-1-1 GTID 0-1-2 GTID 0-2-3 GTID 0-2-4 GTID 0-1-3 GTID 0-1-4 GTID 0-1-5 Suppose now that Slave2 asks to connect to Slave1 at GTID position 0-1-3. I think with your changes, the Slave2 will actually start from the earlier position 0-2-3. So you will get duplicate events 0-2-4 and 0-1-3 on Slave2.
i think functionality that i might have removed would be if you write have 2 mysqld M1 & M2 and your replicate both way...and you *don't* use domain_id...and you write concurrently on both mysqld (but maybe disjoint data ??).
That would probably trigger it as well I think. However, just a rouge slave transaction should trigger it, and this seem to be quite common practice by many users... In gtid_strict_mode, of course, all of this complexity goes away. But we cannot in general know if gtid_strict_mode was enabled when a given piece of binlog was written. - Kristian.
hmm...i'm not sure I get it... is it a bug or a feature that the "rouge" transactions is skipped by Slave2 in statement based replication, skipping 0-2-3 and 0-2-4 can cause arbitrary data drift, right ? thx in advance for help/patience /Jonas On Wed, Mar 4, 2015 at 2:37 PM, Kristian Nielsen <knielsen@knielsen-hq.org> wrote:
Jonas Oreland <jonaso@google.com> writes:
I wrote attached patch (so that i would understand what code actually does) and found myself while refactoring also doing a change, namely that i removed the check for server_id in a couple of places.
it still passes all tests... could you comment on this, or write(outline) a testcase that fails with my patch.
I think the check for server_id is needed in case of out-of-order binlogs.
The easiest / most common way to do this is to do rouge transactions on the slave:
Master:
CREATE TABLE t1 # GTID 0-1-1 INSERT INTO t1 # GTID 0-1-2
Slave1:
CREATE TABLE t_local # GTID 0-2-3 INSERT INTO t_local # GTID 0-2-4
Master:
INSERT INTO t1 # GTID 0-1-3 UPDATE t1 # GTID 0-1-4 DELETE FROM t1 # GTID 0-1-5
Now the binlog on Slave1 contains:
GTID 0-1-1 GTID 0-1-2 GTID 0-2-3 GTID 0-2-4 GTID 0-1-3 GTID 0-1-4 GTID 0-1-5
Suppose now that Slave2 asks to connect to Slave1 at GTID position 0-1-3. I think with your changes, the Slave2 will actually start from the earlier position 0-2-3. So you will get duplicate events 0-2-4 and 0-1-3 on Slave2.
i think functionality that i might have removed would be if you write have 2 mysqld M1 & M2 and your replicate both way...and you *don't* use domain_id...and you write concurrently on both mysqld (but maybe disjoint data ??).
That would probably trigger it as well I think. However, just a rouge slave transaction should trigger it, and this seem to be quite common practice by many users...
In gtid_strict_mode, of course, all of this complexity goes away. But we cannot in general know if gtid_strict_mode was enabled when a given piece of binlog was written.
- Kristian.
Jonas Oreland <jonaso@google.com> writes:
hmm...i'm not sure I get it...
is it a bug or a feature that the "rouge" transactions is skipped by Slave2 in statement based replication, skipping 0-2-3 and 0-2-4 can cause arbitrary data drift, right ?
They are not skipped. The bug is in your patch (I think, I did not test it); those two transactions can be duplicated (executed twice by Slave2). Let me give the example in more detail: Let's say Slave2 first connects to Slave1 from the start. Slave2 executes GTIDs 0-1-1, 0-1-2, 0-2-3, 0-2-4, 0-1-3. Now we run STOP SLAVE on Slave2, @@gtid_slave_pos=0-1-3. Later we do START SLAVE on Slave2. Then Slave2 has to resume from the correct position, which is just after 0-1-3. But with your patch, I think Slave2 will receive and execute 0-2-4 and 0-1-3 again. This results in duplicate events and possible data drift on Slave2. Because in your code, you will reach GTID 0-2-3 in the binlog, and compare against the 0-1-3 requested by Slave2. And since 3==3, you will run info->gtid_state.remove(gtid). And then the next GTID 0-2-4 will be sent (incorrectly) to Slave2. The correct behaviour is to compare 0-2-3 to 0-1-3, see that the server_ids are different, and skip and _not_ remove from the gtid_state. Then GTID 0-2-4 will be skipped, and only after the correct position 0-1-3 will Server2 start receiving events. More generally, if GTIDS D-S1-N1 comes before D-S2-N2 in the binlogs, there is no guarantee that N1 < N2. Only if S1=S2 can we be sure that N1 < N2. That is why the server_id checks are needed. Hope this helps, - Kristian.
Now the binlog on Slave1 contains:
GTID 0-1-1 GTID 0-1-2 GTID 0-2-3 GTID 0-2-4 GTID 0-1-3 GTID 0-1-4 GTID 0-1-5
Jonas Oreland <jonaso@google.com> writes:
I wrote attached patch (so that i would understand what code actually does) and found myself while refactoring also doing a change, namely that i removed the check for server_id in a couple of places.
it still passes all tests...
It is somewhat surprising (and bad) that there were no test cases that caught this. So I pushed a suitable test case: http://lists.askmonty.org/pipermail/commits/2015-March/007520.html Maybe it can help you... - Kristian.
great thx! (was on my TODO...but got stuck on other things today). I'll write a new patch, that passes this test too /Jonas On Thu, Mar 5, 2015 at 9:54 AM, Kristian Nielsen <knielsen@knielsen-hq.org> wrote:
Jonas Oreland <jonaso@google.com> writes:
I wrote attached patch (so that i would understand what code actually does) and found myself while refactoring also doing a change, namely that i removed the check for server_id in a couple of places.
it still passes all tests...
It is somewhat surprising (and bad) that there were no test cases that caught this.
So I pushed a suitable test case:
http://lists.askmonty.org/pipermail/commits/2015-March/007520.html
Maybe it can help you...
- Kristian.
update: just tested your new testcase with my patch...and it passes... i.e the test does not expose any problem in my patch /Jonas On Thu, Mar 5, 2015 at 1:22 PM, Jonas Oreland <jonaso@google.com> wrote:
great thx! (was on my TODO...but got stuck on other things today). I'll write a new patch, that passes this test too
/Jonas
On Thu, Mar 5, 2015 at 9:54 AM, Kristian Nielsen <knielsen@knielsen-hq.org
wrote:
Jonas Oreland <jonaso@google.com> writes:
I wrote attached patch (so that i would understand what code actually does) and found myself while refactoring also doing a change, namely that i removed the check for server_id in a couple of places.
it still passes all tests...
It is somewhat surprising (and bad) that there were no test cases that caught this.
So I pushed a suitable test case:
http://lists.askmonty.org/pipermail/commits/2015-March/007520.html
Maybe it can help you...
- Kristian.
Jonas Oreland <jonaso@google.com> writes:
update: just tested your new testcase with my patch...and it passes... i.e the test does not expose any problem in my patch
Strange. It failed for me with duplicate key error, when I tested it with the patch you attached applied to f4f37533a09b9776e8d5ac3f3a27957f553c9043. - Kristian.
strange indeed! i'll test the specific git revision... /Jonas On Thu, Mar 5, 2015 at 2:42 PM, Kristian Nielsen <knielsen@knielsen-hq.org> wrote:
Jonas Oreland <jonaso@google.com> writes:
update: just tested your new testcase with my patch...and it passes... i.e the test does not expose any problem in my patch
Strange. It failed for me with duplicate key error, when I tested it with the patch you attached applied to f4f37533a09b9776e8d5ac3f3a27957f553c9043.
- Kristian.
UPDATE 2: Found it! 1) I reproduced the problem in revision that you referred to 2) It passed on Google MariaDB as...ta ta... our (yet unpublished) sparse gtid index (that maps between gtid and offset) made it skip over the out of order transaction. this was pure "luck", if I disable it...it will scan the entire binlog and get the duplicate key... puh...that took a while to track down. --- I'll now revise my patch so that it works... /Jonas On Thu, Mar 5, 2015 at 2:55 PM, Jonas Oreland <jonaso@google.com> wrote:
strange indeed! i'll test the specific git revision...
/Jonas
On Thu, Mar 5, 2015 at 2:42 PM, Kristian Nielsen <knielsen@knielsen-hq.org
wrote:
Jonas Oreland <jonaso@google.com> writes:
update: just tested your new testcase with my patch...and it passes... i.e the test does not expose any problem in my patch
Strange. It failed for me with duplicate key error, when I tested it with the patch you attached applied to f4f37533a09b9776e8d5ac3f3a27957f553c9043.
- Kristian.
participants (2)
-
Jonas Oreland -
Kristian Nielsen