Merge lp:~gl-az/percona-server/BT-26980-5.5-bug1068210 into lp:percona-server/5.5

Proposed by George Ormond Lorch III
Status: Merged
Approved by: Laurynas Biveinis
Approved revision: 447
Merged at revision: 447
Proposed branch: lp:~gl-az/percona-server/BT-26980-5.5-bug1068210
Merge into: lp:percona-server/5.5
Diff against target: 113 lines (+85/-2)
4 files modified
Percona-Server/mysql-test/suite/rpl/r/rpl_trigger_bug67504.result (+28/-0)
Percona-Server/mysql-test/suite/rpl/t/rpl_trigger_bug67504-slave.opt (+1/-0)
Percona-Server/mysql-test/suite/rpl/t/rpl_trigger_bug67504.test (+49/-0)
Percona-Server/sql/sql_parse.cc (+7/-2)
To merge this branch: bzr merge lp:~gl-az/percona-server/BT-26980-5.5-bug1068210
Reviewer Review Type Date Requested Status
Laurynas Biveinis (community) Approve
Review via email: mp+149159@code.launchpad.net

This proposal supersedes a proposal from 2013-02-11.

Description of the change

Fix for lp1068210 which is also reported as upstream 67504.

If when processing slave replication events, a trigger is fired, any slave 'deferred' events such as LAST_INSERT_ID are replayed within the trigger sub statement. Simple test added to check to see if execution is within a sub statement before attempting to execute any deferred events.

Since only test case was modified, re-ran jenkins for just that case.

http://jenkins.percona.com/view/PS%205.5/job/percona-server-5.5-param/680/

To post a comment you must log in.
Revision history for this message
George Ormond Lorch III (gl-az) wrote : Posted in a previous version of this proposal
Revision history for this message
Laurynas Biveinis (laurynas-biveinis) wrote : Posted in a previous version of this proposal

    - The code looks OK. I am not sure why there is no
      deferred_events::rewind() call immediatelly after or in
      d_e::execute() method, which would seem to resolve this issue,
      but oh well.
    - The testcase:
    - Any reason to mark this test for SBR/MBR and not RBR? If I
      understand correctly, it is only the SBR that triggers the
      bug. MBR would use RBR for all t1 events. So, let's just test
      SBR.
    - Please replace connection slave; with sync_slave_with_master at
      diff line 72, so that the presence of t1 is guaranteed at slave
      at the trigger create time.
    - Redundant "conection slave;" at line 85.
    - Lines 89 and 90 can be replaced by include/stop_slave.inc, but
      this for future reference, as in this instance the testcase
      cleanup should be done in another way:
    - Tables t1 and t2 would remain on the slave at the end of the
      testcase end, because their DROPs are not replicated. Thus,
      omit the slave stop, perform the slave-only cleanup, perform the
      (replicated) cleanup on master, and then include rpl_end.inc,
      which will sync, stop slave, and cleanup.

review: Needs Fixing
Revision history for this message
Laurynas Biveinis (laurynas-biveinis) wrote : Posted in a previous version of this proposal

On the second thought, let's test all three SBR/RBR/MBR replication formats in the testcase. Even though the bug concerns SBR only, the testcase has triggers and slave-only tables, that's interesting for RBR too.

Revision history for this message
George Ormond Lorch III (gl-az) wrote : Posted in a previous version of this proposal

> - Any reason to mark this test for SBR/MBR and not RBR? If I
> understand correctly, it is only the SBR that triggers the
> bug. MBR would use RBR for all t1 events. So, let's just test
> SBR.

This test will not work with RBR. It seems that if RBR is used, the slave side trigger is not fired and thus the rows replicated into t1 never create the corresponding rows in slave_only, causing the test case to fail on SELECT id FROM slave_only; IT _does_ seem to work fine for SBR and MBR though.

> - Please replace connection slave; with sync_slave_with_master at
> diff line 72, so that the presence of t1 is guaranteed at slave
> at the trigger create time.

Good catch.

> - Redundant "conection slave;" at line 85.

Another good catch.

> - Lines 89 and 90 can be replaced by include/stop_slave.inc, but
> this for future reference, as in this instance the testcase
> cleanup should be done in another way:
> - Tables t1 and t2 would remain on the slave at the end of the
> testcase end, because their DROPs are not replicated. Thus,
> omit the slave stop, perform the slave-only cleanup, perform the
> (replicated) cleanup on master, and then include rpl_end.inc,
> which will sync, stop slave, and cleanup.

Ahh, very cool. I was a bit uncertain as to the 'correct' way to clean up the slave in this situation and didn't know about rpl_end.inc. Much cleaner now, Thanks!

Revision history for this message
Laurynas Biveinis (laurynas-biveinis) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added file 'Percona-Server/mysql-test/suite/rpl/r/rpl_trigger_bug67504.result'
2--- Percona-Server/mysql-test/suite/rpl/r/rpl_trigger_bug67504.result 1970-01-01 00:00:00 +0000
3+++ Percona-Server/mysql-test/suite/rpl/r/rpl_trigger_bug67504.result 2013-02-18 21:23:23 +0000
4@@ -0,0 +1,28 @@
5+include/master-slave.inc
6+[connection master]
7+DROP TRIGGER IF EXISTS t1_insert;
8+DROP TABLE IF EXISTS slave_only;
9+DROP TABLE IF EXISTS t1;
10+DROP TABLE IF EXISTS t2;
11+CREATE TABLE t1(id INT AUTO_INCREMENT PRIMARY KEY) ENGINE=INNODB;
12+CREATE TABLE t2(id INT AUTO_INCREMENT PRIMARY KEY) ENGINE=INNODB;
13+CREATE TABLE slave_only(id INT AUTO_INCREMENT PRIMARY KEY) ENGINE=INNODB;
14+CREATE TRIGGER t1_insert AFTER INSERT ON t1 FOR EACH ROW INSERT INTO slave_only VALUES(NULL);
15+INSERT INTO slave_only VALUES(NULL);
16+INSERT INTO slave_only VALUES(NULL);
17+INSERT INTO t2 VALUES(NULL);
18+INSERT INTO t2 VALUES(NULL);
19+INSERT INTO t1 VALUES(NULL);
20+SELECT id FROM t1;
21+id
22+1
23+SELECT id FROM slave_only;
24+id
25+1
26+2
27+3
28+DROP TRIGGER t1_insert;
29+DROP TABLE slave_only;
30+DROP TABLE t1;
31+DROP TABLE t2;
32+include/rpl_end.inc
33
34=== added file 'Percona-Server/mysql-test/suite/rpl/t/rpl_trigger_bug67504-slave.opt'
35--- Percona-Server/mysql-test/suite/rpl/t/rpl_trigger_bug67504-slave.opt 1970-01-01 00:00:00 +0000
36+++ Percona-Server/mysql-test/suite/rpl/t/rpl_trigger_bug67504-slave.opt 2013-02-18 21:23:23 +0000
37@@ -0,0 +1,1 @@
38+--replicate_ignore_table=test.t2
39
40=== added file 'Percona-Server/mysql-test/suite/rpl/t/rpl_trigger_bug67504.test'
41--- Percona-Server/mysql-test/suite/rpl/t/rpl_trigger_bug67504.test 1970-01-01 00:00:00 +0000
42+++ Percona-Server/mysql-test/suite/rpl/t/rpl_trigger_bug67504.test 2013-02-18 21:23:23 +0000
43@@ -0,0 +1,49 @@
44+########################################################################
45+# Bug 67504: Duplicate error in replication with slave triggers and
46+# auto increment
47+########################################################################
48+--source include/have_innodb.inc
49+--source include/have_binlog_format_mixed_or_statement.inc
50+--source include/master-slave.inc
51+
52+disable_query_log;
53+call mtr.add_suppression("Unsafe statement written to the binary log using statement format since BINLOG_FORMAT = STATEMENT");
54+enable_query_log;
55+
56+--disable_warnings
57+connection slave;
58+DROP TRIGGER IF EXISTS t1_insert;
59+DROP TABLE IF EXISTS slave_only;
60+connection master;
61+DROP TABLE IF EXISTS t1;
62+DROP TABLE IF EXISTS t2;
63+sync_slave_with_master;
64+--enable_warnings
65+
66+connection master;
67+CREATE TABLE t1(id INT AUTO_INCREMENT PRIMARY KEY) ENGINE=INNODB;
68+CREATE TABLE t2(id INT AUTO_INCREMENT PRIMARY KEY) ENGINE=INNODB;
69+
70+sync_slave_with_master;
71+CREATE TABLE slave_only(id INT AUTO_INCREMENT PRIMARY KEY) ENGINE=INNODB;
72+CREATE TRIGGER t1_insert AFTER INSERT ON t1 FOR EACH ROW INSERT INTO slave_only VALUES(NULL);
73+INSERT INTO slave_only VALUES(NULL);
74+INSERT INTO slave_only VALUES(NULL);
75+
76+connection master;
77+INSERT INTO t2 VALUES(NULL);
78+INSERT INTO t2 VALUES(NULL);
79+INSERT INTO t1 VALUES(NULL);
80+
81+sync_slave_with_master;
82+SELECT id FROM t1;
83+SELECT id FROM slave_only;
84+
85+DROP TRIGGER t1_insert;
86+DROP TABLE slave_only;
87+
88+connection master;
89+DROP TABLE t1;
90+DROP TABLE t2;
91+
92+--source include/rpl_end.inc
93
94=== modified file 'Percona-Server/sql/sql_parse.cc'
95--- Percona-Server/sql/sql_parse.cc 2013-01-30 09:55:26 +0000
96+++ Percona-Server/sql/sql_parse.cc 2013-02-18 21:23:23 +0000
97@@ -2171,9 +2171,14 @@
98 }
99 /*
100 Execute deferred events first
101+ Bug lp1068210 or upstream 67504: Test first to see if we are executing
102+ within a sub statement. If so, DO NOT execute any deferred events, they
103+ have already been executed by the parent statement and have no bearing
104+ on the sub statement and can cause faulty behavior.
105 */
106- if (slave_execute_deferred_events(thd))
107- DBUG_RETURN(-1);
108+ if (thd->in_sub_stmt == 0)
109+ if (slave_execute_deferred_events(thd))
110+ DBUG_RETURN(-1);
111 }
112 else
113 {

Subscribers

People subscribed via source and target branches