Skip to content

Commit 8c817e2

Browse files
andrelkinemoonrain
andrelkin
authored and
emoonrain
committed
MDEV-35570 parallel slave ALTER-SEQUENCE attempted to binlog out-of-order
Since MDEV-31503 fixes ALTER-SEQUENCE might be able to complete its work including binlogging before a preceding in binlog-order transaction. There may not be data dependency between the two transactions, but there would be "an attempt was made to binlog GTID D-S-XYZ which would create an out-of-order sequence number" error in the gtid_strict_mode = ON. After the preceding transaction started committing, and does it rather slow, ALTER-SEQUNCE was able to find a time window to complete because it temporarily releases its link with the waitee parent transaction. And while having it released it also erroneously executes the binlogging part. Fixed with restoring the commit dependency on the parent before ALTER-SEQUNCE takes on binlogging.
1 parent e79aa9c commit 8c817e2

File tree

4 files changed

+177
-96
lines changed

4 files changed

+177
-96
lines changed

‎mysql-test/suite/rpl/r/rpl_parallel_seq.result‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,30 @@ SELECT @@global.gtid_binlog_state, @@global.gtid_slave_pos as "all through 101 h
8282
@@global.gtid_binlog_state all through 101 have been committed
8383
0-1-101 0-1-101
8484
connection slave;
85+
include/stop_slave.inc
86+
set @saved_mode= @@global.slave_parallel_mode;
87+
set @@global.slave_parallel_mode = conservative;
88+
include/start_slave.inc
89+
connection master;
90+
INSERT INTO ti SET a=2;
91+
include/save_master_gtid.inc
92+
connection slave;
93+
include/sync_with_master_gtid.inc
94+
lock table ti write;
95+
SET GLOBAL debug_dbug= "+d,halt_past_mark_start_commit";
96+
connection master;
97+
INSERT INTO ti SET a=35570;
98+
ALTER SEQUENCE s2 restart with 1;
99+
include/save_master_gtid.inc
100+
connection slave;
101+
unlock tables;
102+
SET debug_sync = "now SIGNAL past_mark_continue";
103+
include/sync_with_master_gtid.inc
104+
include/stop_slave.inc
105+
SET @@global.slave_parallel_mode = @saved_mode;
106+
SET @@global.debug_dbug = @@GLOBAL.debug_dbug;
107+
include/start_slave.inc
108+
connection slave;
85109
flush tables with read lock;
86110
connection master;
87111
CREATE OR REPLACE SEQUENCE s3 ENGINE=innodb;

‎mysql-test/suite/rpl/t/rpl_parallel_seq.test‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,53 @@ SET DEBUG_SYNC = 'now SIGNAL continue_worker';
128128

129129
SELECT @@global.gtid_binlog_state, @@global.gtid_slave_pos as "all through 101 have been committed";
130130

131+
#
132+
# MDEV-35570 parallel slave ALTER-SEQUNCE attemted to binlog out-of-order.
133+
# Let two transactions I_1 -> AS_2 where AS_2 depends on a commit parent I_1.
134+
# Under the bug condition AS_2 may complete its work including binlogging
135+
# while I_1 is slowly executing Xid_log_event.
136+
# The test simulate the slowness, AS_2 must defer its completion.
137+
#
138+
--connection slave
139+
--source include/stop_slave.inc
140+
set @saved_mode= @@global.slave_parallel_mode;
141+
set @@global.slave_parallel_mode = conservative;
142+
--source include/start_slave.inc
143+
144+
--connection master
145+
INSERT INTO ti SET a=2;
146+
--source include/save_master_gtid.inc
147+
148+
--connection slave
149+
--source include/sync_with_master_gtid.inc
150+
# allow to proceed to sync with the 1st following WFPT2SC wait condtion
151+
lock table ti write;
152+
# allow to proceed into commit to sync with the 2nd following WFPC wait condition
153+
--let $saved_dbug= @@GLOBAL.debug_dbug
154+
SET GLOBAL debug_dbug= "+d,halt_past_mark_start_commit";
155+
156+
--connection master
157+
INSERT INTO ti SET a=35570;
158+
ALTER SEQUENCE s2 restart with 1;
159+
--source include/save_master_gtid.inc
160+
161+
--connection slave
162+
--let $wait_condition= SELECT count(*) = 1 FROM information_schema.processlist WHERE state LIKE "Waiting for prior transaction to start commit"
163+
--source include/wait_condition.inc
164+
# the 1st wait release
165+
unlock tables;
166+
167+
--let $wait_condition= SELECT count(*) = 1 FROM information_schema.processlist WHERE state LIKE "Waiting for prior transaction to commit"
168+
--source include/wait_condition.inc
169+
# the 2nd wait release
170+
SET debug_sync = "now SIGNAL past_mark_continue";
171+
172+
--source include/sync_with_master_gtid.inc
173+
--source include/stop_slave.inc
174+
SET @@global.slave_parallel_mode = @saved_mode;
175+
--eval SET @@global.debug_dbug = $saved_dbug
176+
--source include/start_slave.inc
177+
131178
# MDEV-31792 Assertion in MDL_context::acquire_lock upon parallel replication of CREATE SEQUENCE
132179

133180
--let $iter = 3

‎sql/rpl_parallel.cc‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1537,6 +1537,14 @@ handle_rpl_parallel_thread(void *arg)
15371537
else
15381538
rgi->mark_start_commit();
15391539
DEBUG_SYNC(thd, "rpl_parallel_after_mark_start_commit");
1540+
#ifdef ENABLED_DEBUG_SYNC
1541+
DBUG_EXECUTE_IF("halt_past_mark_start_commit",
1542+
{
1543+
DBUG_ASSERT(!debug_sync_set_action
1544+
(thd, STRING_WITH_LEN("now WAIT_FOR past_mark_continue")));
1545+
DBUG_SET_INITIAL("-d,halt_past_mark_start_commit");
1546+
};);
1547+
#endif
15401548
}
15411549
}
15421550

‎sql/sql_sequence.cc‎

Lines changed: 98 additions & 96 deletions
Original file line numberDiff line numberDiff line change
@@ -960,119 +960,121 @@ bool Sql_cmd_alter_sequence::execute(THD *thd)
960960
SEQUENCE *seq;
961961
No_such_table_error_handler no_such_table_handler;
962962
DBUG_ENTER("Sql_cmd_alter_sequence::execute");
963+
{
963964
#if defined(HAVE_REPLICATION)
964-
/* No wakeup():s of subsequent commits is allowed in this function. */
965-
wait_for_commit_raii suspend_wfc(thd);
965+
/* No wakeup():s of subsequent commits is allowed in this function. */
966+
wait_for_commit_raii suspend_wfc(thd);
966967
#endif
967968

968-
if (check_access(thd, ALTER_ACL, first_table->db.str,
969-
&first_table->grant.privilege,
970-
&first_table->grant.m_internal,
971-
0, 0))
972-
DBUG_RETURN(TRUE); /* purecov: inspected */
969+
if (check_access(thd, ALTER_ACL, first_table->db.str,
970+
&first_table->grant.privilege,
971+
&first_table->grant.m_internal,
972+
0, 0))
973+
DBUG_RETURN(TRUE); /* purecov: inspected */
973974

974-
if (check_grant(thd, ALTER_ACL, first_table, FALSE, 1, FALSE))
975-
DBUG_RETURN(TRUE); /* purecov: inspected */
975+
if (check_grant(thd, ALTER_ACL, first_table, FALSE, 1, FALSE))
976+
DBUG_RETURN(TRUE); /* purecov: inspected */
976977

977978
#ifdef WITH_WSREP
978-
if (WSREP(thd) && wsrep_thd_is_local(thd))
979-
{
980-
const bool used_engine= lex->create_info.used_fields & HA_CREATE_USED_ENGINE;
981-
if (wsrep_check_sequence(thd, new_seq, used_engine))
982-
DBUG_RETURN(TRUE);
979+
if (WSREP(thd) && wsrep_thd_is_local(thd))
980+
{
981+
const bool used_engine= lex->create_info.used_fields & HA_CREATE_USED_ENGINE;
982+
if (wsrep_check_sequence(thd, new_seq, used_engine))
983+
DBUG_RETURN(TRUE);
984+
985+
if (wsrep_to_isolation_begin(thd, first_table->db.str,
986+
first_table->table_name.str,
987+
first_table))
988+
{
989+
DBUG_RETURN(TRUE);
990+
}
991+
}
992+
#endif /* WITH_WSREP */
983993

984-
if (wsrep_to_isolation_begin(thd, first_table->db.str,
985-
first_table->table_name.str,
986-
first_table))
994+
if (if_exists())
995+
thd->push_internal_handler(&no_such_table_handler);
996+
error= open_and_lock_tables(thd, first_table, FALSE, 0);
997+
if (if_exists())
998+
{
999+
trapped_errors= no_such_table_handler.safely_trapped_errors();
1000+
thd->pop_internal_handler();
1001+
}
1002+
if (unlikely(error))
9871003
{
1004+
if (trapped_errors)
1005+
{
1006+
StringBuffer<FN_REFLEN> tbl_name;
1007+
tbl_name.append(&first_table->db);
1008+
tbl_name.append('.');
1009+
tbl_name.append(&first_table->table_name);
1010+
push_warning_printf(thd, Sql_condition::WARN_LEVEL_NOTE,
1011+
ER_UNKNOWN_SEQUENCES,
1012+
ER_THD(thd, ER_UNKNOWN_SEQUENCES),
1013+
tbl_name.c_ptr_safe());
1014+
my_ok(thd);
1015+
DBUG_RETURN(FALSE);
1016+
}
9881017
DBUG_RETURN(TRUE);
9891018
}
990-
}
991-
#endif /* WITH_WSREP */
9921019

993-
if (if_exists())
994-
thd->push_internal_handler(&no_such_table_handler);
995-
error= open_and_lock_tables(thd, first_table, FALSE, 0);
996-
if (if_exists())
997-
{
998-
trapped_errors= no_such_table_handler.safely_trapped_errors();
999-
thd->pop_internal_handler();
1000-
}
1001-
if (unlikely(error))
1002-
{
1003-
if (trapped_errors)
1020+
table= first_table->table;
1021+
seq= table->s->sequence;
1022+
1023+
seq->write_lock(table);
1024+
new_seq->reserved_until= seq->reserved_until;
1025+
1026+
/* Copy from old sequence those fields that the user didn't specified */
1027+
if (!(new_seq->used_fields & seq_field_used_increment))
1028+
new_seq->increment= seq->increment;
1029+
if (!(new_seq->used_fields & seq_field_used_min_value))
1030+
new_seq->min_value= seq->min_value;
1031+
if (!(new_seq->used_fields & seq_field_used_max_value))
1032+
new_seq->max_value= seq->max_value;
1033+
if (!(new_seq->used_fields & seq_field_used_start))
1034+
new_seq->start= seq->start;
1035+
if (!(new_seq->used_fields & seq_field_used_cache))
1036+
new_seq->cache= seq->cache;
1037+
if (!(new_seq->used_fields & seq_field_used_cycle))
1038+
new_seq->cycle= seq->cycle;
1039+
1040+
/* If we should restart from a new value */
1041+
if (new_seq->used_fields & seq_field_used_restart)
10041042
{
1005-
StringBuffer<FN_REFLEN> tbl_name;
1006-
tbl_name.append(&first_table->db);
1007-
tbl_name.append('.');
1008-
tbl_name.append(&first_table->table_name);
1009-
push_warning_printf(thd, Sql_condition::WARN_LEVEL_NOTE,
1010-
ER_UNKNOWN_SEQUENCES,
1011-
ER_THD(thd, ER_UNKNOWN_SEQUENCES),
1012-
tbl_name.c_ptr_safe());
1013-
my_ok(thd);
1014-
DBUG_RETURN(FALSE);
1043+
if (!(new_seq->used_fields & seq_field_used_restart_value))
1044+
new_seq->restart= new_seq->start;
1045+
new_seq->reserved_until= new_seq->restart;
10151046
}
1016-
DBUG_RETURN(TRUE);
1017-
}
10181047

1019-
table= first_table->table;
1020-
seq= table->s->sequence;
1021-
1022-
seq->write_lock(table);
1023-
new_seq->reserved_until= seq->reserved_until;
1024-
1025-
/* Copy from old sequence those fields that the user didn't specified */
1026-
if (!(new_seq->used_fields & seq_field_used_increment))
1027-
new_seq->increment= seq->increment;
1028-
if (!(new_seq->used_fields & seq_field_used_min_value))
1029-
new_seq->min_value= seq->min_value;
1030-
if (!(new_seq->used_fields & seq_field_used_max_value))
1031-
new_seq->max_value= seq->max_value;
1032-
if (!(new_seq->used_fields & seq_field_used_start))
1033-
new_seq->start= seq->start;
1034-
if (!(new_seq->used_fields & seq_field_used_cache))
1035-
new_seq->cache= seq->cache;
1036-
if (!(new_seq->used_fields & seq_field_used_cycle))
1037-
new_seq->cycle= seq->cycle;
1038-
1039-
/* If we should restart from a new value */
1040-
if (new_seq->used_fields & seq_field_used_restart)
1041-
{
1042-
if (!(new_seq->used_fields & seq_field_used_restart_value))
1043-
new_seq->restart= new_seq->start;
1044-
new_seq->reserved_until= new_seq->restart;
1045-
}
1048+
/* Let check_and_adjust think all fields are used */
1049+
new_seq->used_fields= ~0;
1050+
if (new_seq->check_and_adjust(0))
1051+
{
1052+
my_error(ER_SEQUENCE_INVALID_DATA, MYF(0),
1053+
first_table->db.str,
1054+
first_table->table_name.str);
1055+
error= 1;
1056+
seq->write_unlock(table);
1057+
goto end;
1058+
}
10461059

1047-
/* Let check_and_adjust think all fields are used */
1048-
new_seq->used_fields= ~0;
1049-
if (new_seq->check_and_adjust(0))
1050-
{
1051-
my_error(ER_SEQUENCE_INVALID_DATA, MYF(0),
1052-
first_table->db.str,
1053-
first_table->table_name.str);
1054-
error= 1;
1060+
if (likely(!(error= new_seq->write(table, 1))))
1061+
{
1062+
/* Store the sequence values in table share */
1063+
seq->copy(new_seq);
1064+
}
1065+
else
1066+
table->file->print_error(error, MYF(0));
10551067
seq->write_unlock(table);
1056-
goto end;
1057-
}
1058-
1059-
if (likely(!(error= new_seq->write(table, 1))))
1060-
{
1061-
/* Store the sequence values in table share */
1062-
seq->copy(new_seq);
1068+
if (trans_commit_stmt(thd))
1069+
error= 1;
1070+
if (trans_commit_implicit(thd))
1071+
error= 1;
1072+
DBUG_EXECUTE_IF("hold_worker_on_schedule",
1073+
{
1074+
/* delay binlogging of a parent trx in rpl_parallel_seq */
1075+
my_sleep(100000);
1076+
});
10631077
}
1064-
else
1065-
table->file->print_error(error, MYF(0));
1066-
seq->write_unlock(table);
1067-
if (trans_commit_stmt(thd))
1068-
error= 1;
1069-
if (trans_commit_implicit(thd))
1070-
error= 1;
1071-
DBUG_EXECUTE_IF("hold_worker_on_schedule",
1072-
{
1073-
/* delay binlogging of a parent trx in rpl_parallel_seq */
1074-
my_sleep(100000);
1075-
});
10761078
if (likely(!error))
10771079
error= write_bin_log(thd, 1, thd->query(), thd->query_length());
10781080
if (likely(!error))

0 commit comments

Comments
 (0)