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
=== added file 'Percona-Server/mysql-test/suite/rpl/r/rpl_trigger_bug67504.result'
--- Percona-Server/mysql-test/suite/rpl/r/rpl_trigger_bug67504.result 1970-01-01 00:00:00 +0000
+++ Percona-Server/mysql-test/suite/rpl/r/rpl_trigger_bug67504.result 2013-02-18 21:23:23 +0000
@@ -0,0 +1,28 @@
1include/master-slave.inc
2[connection master]
3DROP TRIGGER IF EXISTS t1_insert;
4DROP TABLE IF EXISTS slave_only;
5DROP TABLE IF EXISTS t1;
6DROP TABLE IF EXISTS t2;
7CREATE TABLE t1(id INT AUTO_INCREMENT PRIMARY KEY) ENGINE=INNODB;
8CREATE TABLE t2(id INT AUTO_INCREMENT PRIMARY KEY) ENGINE=INNODB;
9CREATE TABLE slave_only(id INT AUTO_INCREMENT PRIMARY KEY) ENGINE=INNODB;
10CREATE TRIGGER t1_insert AFTER INSERT ON t1 FOR EACH ROW INSERT INTO slave_only VALUES(NULL);
11INSERT INTO slave_only VALUES(NULL);
12INSERT INTO slave_only VALUES(NULL);
13INSERT INTO t2 VALUES(NULL);
14INSERT INTO t2 VALUES(NULL);
15INSERT INTO t1 VALUES(NULL);
16SELECT id FROM t1;
17id
181
19SELECT id FROM slave_only;
20id
211
222
233
24DROP TRIGGER t1_insert;
25DROP TABLE slave_only;
26DROP TABLE t1;
27DROP TABLE t2;
28include/rpl_end.inc
029
=== added file 'Percona-Server/mysql-test/suite/rpl/t/rpl_trigger_bug67504-slave.opt'
--- Percona-Server/mysql-test/suite/rpl/t/rpl_trigger_bug67504-slave.opt 1970-01-01 00:00:00 +0000
+++ Percona-Server/mysql-test/suite/rpl/t/rpl_trigger_bug67504-slave.opt 2013-02-18 21:23:23 +0000
@@ -0,0 +1,1 @@
1--replicate_ignore_table=test.t2
02
=== added file 'Percona-Server/mysql-test/suite/rpl/t/rpl_trigger_bug67504.test'
--- Percona-Server/mysql-test/suite/rpl/t/rpl_trigger_bug67504.test 1970-01-01 00:00:00 +0000
+++ Percona-Server/mysql-test/suite/rpl/t/rpl_trigger_bug67504.test 2013-02-18 21:23:23 +0000
@@ -0,0 +1,49 @@
1########################################################################
2# Bug 67504: Duplicate error in replication with slave triggers and
3# auto increment
4########################################################################
5--source include/have_innodb.inc
6--source include/have_binlog_format_mixed_or_statement.inc
7--source include/master-slave.inc
8
9disable_query_log;
10call mtr.add_suppression("Unsafe statement written to the binary log using statement format since BINLOG_FORMAT = STATEMENT");
11enable_query_log;
12
13--disable_warnings
14connection slave;
15DROP TRIGGER IF EXISTS t1_insert;
16DROP TABLE IF EXISTS slave_only;
17connection master;
18DROP TABLE IF EXISTS t1;
19DROP TABLE IF EXISTS t2;
20sync_slave_with_master;
21--enable_warnings
22
23connection master;
24CREATE TABLE t1(id INT AUTO_INCREMENT PRIMARY KEY) ENGINE=INNODB;
25CREATE TABLE t2(id INT AUTO_INCREMENT PRIMARY KEY) ENGINE=INNODB;
26
27sync_slave_with_master;
28CREATE TABLE slave_only(id INT AUTO_INCREMENT PRIMARY KEY) ENGINE=INNODB;
29CREATE TRIGGER t1_insert AFTER INSERT ON t1 FOR EACH ROW INSERT INTO slave_only VALUES(NULL);
30INSERT INTO slave_only VALUES(NULL);
31INSERT INTO slave_only VALUES(NULL);
32
33connection master;
34INSERT INTO t2 VALUES(NULL);
35INSERT INTO t2 VALUES(NULL);
36INSERT INTO t1 VALUES(NULL);
37
38sync_slave_with_master;
39SELECT id FROM t1;
40SELECT id FROM slave_only;
41
42DROP TRIGGER t1_insert;
43DROP TABLE slave_only;
44
45connection master;
46DROP TABLE t1;
47DROP TABLE t2;
48
49--source include/rpl_end.inc
050
=== modified file 'Percona-Server/sql/sql_parse.cc'
--- Percona-Server/sql/sql_parse.cc 2013-01-30 09:55:26 +0000
+++ Percona-Server/sql/sql_parse.cc 2013-02-18 21:23:23 +0000
@@ -2171,9 +2171,14 @@
2171 }2171 }
2172 /* 2172 /*
2173 Execute deferred events first2173 Execute deferred events first
2174 Bug lp1068210 or upstream 67504: Test first to see if we are executing
2175 within a sub statement. If so, DO NOT execute any deferred events, they
2176 have already been executed by the parent statement and have no bearing
2177 on the sub statement and can cause faulty behavior.
2174 */2178 */
2175 if (slave_execute_deferred_events(thd))2179 if (thd->in_sub_stmt == 0)
2176 DBUG_RETURN(-1);2180 if (slave_execute_deferred_events(thd))
2181 DBUG_RETURN(-1);
2177 }2182 }
2178 else2183 else
2179 {2184 {

Subscribers

People subscribed via source and target branches