Skip to content

10.11 patch for MDEV-37541 (Race of rolling back and committing transaction to binlog #4376

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: 10.11
Choose a base branch
from
+139 −18
Open

10.11 patch for MDEV-37541 (Race of rolling back and committing transaction to binlog #4376

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eb8314b
MDEV-37541 Race of rolling back and committing transaction to binlog
emoonrain Oct 16, 2025
a8d2c97
MDEV-37541 Race of rolling back and committing transaction to binlog
emoonrain Oct 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions mysql-test/suite/binlog/r/binlog_mdev37541.result
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
create table t1 (a int primary key, b text) engine=innodb;
connect trx1_rollback,localhost,root,,;
CREATE TABLE t_x (a int) engine=MEMORY;
SET binlog_format=row;
CREATE TEMPORARY TABLE tt_tmp ( id INT ) ENGINE = Memory;
BEGIN;
insert into t_x values (1);
drop temporary table tt_tmp;
insert into t1 values (99, "gotta log first");
SET DEBUG_SYNC= 'reset';
SET DEBUG_SYNC= "before_group_commit_queue SIGNAL trx1_at_log WAIT_FOR trx1_go";
ROLLBACK;
connect trx2_commit,localhost,root,,;
insert into t1 values (99, "second best in binlog");
connection default;
SET DEBUG_SYNC= "now WAIT_FOR trx1_at_log";
SET DEBUG_SYNC= "now SIGNAL trx1_go";
connection trx2_commit;
select * from t1;
a b
99 second best in binlog
# Prove the logging order is Trx1, Trx2
include/show_binlog_events.inc
Log_name Pos Event_type Server_id End_log_pos Info
master-bin.000001 # Gtid # # BEGIN GTID #-#-#
master-bin.000001 # Annotate_rows # # insert into t_x values (1)
master-bin.000001 # Table_map # # table_id: # (test.t_x)
master-bin.000001 # Write_rows_v1 # # table_id: # flags: STMT_END_F
master-bin.000001 # Query # # COMMIT
master-bin.000001 # Gtid # # BEGIN GTID #-#-#
master-bin.000001 # Annotate_rows # # insert into t1 values (99, "gotta log first")
master-bin.000001 # Table_map # # table_id: # (test.t1)
master-bin.000001 # Write_rows_v1 # # table_id: # flags: STMT_END_F
master-bin.000001 # Query # # ROLLBACK
master-bin.000001 # Gtid # # BEGIN GTID #-#-#
master-bin.000001 # Query # # use `test`; insert into t1 values (99, "second best in binlog")
master-bin.000001 # Xid # # COMMIT /* XID */
drop table t_x, t1;
disconnect trx1_rollback;
disconnect trx2_commit;
# end of the tests
50 changes: 50 additions & 0 deletions mysql-test/suite/binlog/t/binlog_mdev37541.test
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
--source include/have_debug_sync.inc
Copy link
Contributor

@bnestere bnestere Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment to explain the purpose/methodology of the test?

--source include/have_innodb.inc
--source include/have_binlog_format_mixed.inc

create table t1 (a int primary key, b text) engine=innodb;

connect(trx1_rollback,localhost,root,,);
CREATE TABLE t_x (a int) engine=MEMORY;

--let $master_file= query_get_value(SHOW MASTER STATUS, File, 1)
--let $binlog_start=query_get_value(SHOW MASTER STATUS, Position, 1)
SET binlog_format=row;
CREATE TEMPORARY TABLE tt_tmp ( id INT ) ENGINE = Memory;
BEGIN;
insert into t_x values (1);
drop temporary table tt_tmp;
insert into t1 values (99, "gotta log first");

SET DEBUG_SYNC= 'reset';
SET DEBUG_SYNC= "before_group_commit_queue SIGNAL trx1_at_log WAIT_FOR trx1_go";

--send ROLLBACK

connect(trx2_commit,localhost,root,,);
--send insert into t1 values (99, "second best in binlog")

connection default;

# Make sure ROLLBACK is in the binlogging phase ..
SET DEBUG_SYNC= "now WAIT_FOR trx1_at_log";
# .. and the contender trx2 in the locking phase ..
let $wait_condition=
select count(*) = 1 from information_schema.innodb_trx
where trx_state = "LOCK WAIT" and trx_query like "%insert into t1 values%";
source include/wait_condition.inc;
# .. when both provided unfreeze the trx:s ..
SET DEBUG_SYNC= "now SIGNAL trx1_go";

connection trx2_commit;
reap;
select * from t1;

# .. to observe proper binlogging.
--echo # Prove the logging order is Trx1, Trx2
--source include/show_binlog_events.inc

drop table t_x, t1;
disconnect trx1_rollback;
disconnect trx2_commit;
--echo # end of the tests
61 changes: 45 additions & 16 deletions sql/handler.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2150,12 +2150,37 @@ static bool is_ro_1pc_trans(THD *thd, Ha_trx_info *ha_info, bool all,
return !rw_trans;
}

static bool has_binlog_hton(Ha_trx_info *ha_info)
inline Ha_trx_info* get_binlog_hton(Ha_trx_info *ha_info)
Copy link
Contributor

@bnestere bnestere Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If each and every transaction is always going to check if it has a binlog_hton registered in its ha_list no matter what, would it make more sense to just extend THD or transaction with a member variable to indicate if the binlog_hton is a part of the transaction or not? Then we could directly get binlog_hton. It feels there is bloat no matter what, but the bloat that doesn't involve extra loops/conditionals seems less heavy to me.

{
bool rc;
for (rc= false; ha_info && !rc; ha_info= ha_info->next())
rc= ha_info->ht() == binlog_hton;
for (; ha_info; ha_info= ha_info->next())
if (ha_info->ht() == binlog_hton)
return ha_info;

return ha_info;
}

static int run_binlog_first(THD *thd, bool all, THD_TRANS *trans,
bool is_real_trans, bool is_commit)
{
int rc= 0;
Ha_trx_info *ha_info= trans->ha_list;

if (mysql_bin_log.is_open())
{
if ((ha_info= get_binlog_hton(ha_info)))
{
int err;
if ((err= is_commit ?
binlog_commit(thd, all,
is_ro_1pc_trans(thd, ha_info, all, is_real_trans)) :
binlog_rollback(ha_info->ht(), thd, all)))
Copy link
Contributor

@bnestere bnestere Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, starting in 11.8, binlog_rollback no longer has a handlerton* as a parameter. I was thinking you could remove the need for a ternary operator by just passing in the function to call (i.e. binlog_commit or binlog_rollback) depending on the context which it is called.

{
my_error(is_commit? ER_ERROR_DURING_COMMIT : ER_ERROR_DURING_ROLLBACK,
MYF(0), err);
rc= 1;
}
}
}
return rc;
}

Expand All @@ -2172,18 +2197,17 @@ commit_one_phase_2(THD *thd, bool all, THD_TRANS *trans, bool is_real_trans)
if (ha_info)
{
int err= 0;

if (has_binlog_hton(ha_info) &&
(err= binlog_commit(thd, all,
is_ro_1pc_trans(thd, ha_info, all, is_real_trans))))
/*
Binlog hton must be called first regardless of its position
in trans->ha_list.
Copy link
Contributor

@bnestere bnestere Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code is pretty clear in that with the name run_binlog_first; for the comment here, I'd instead provide

  1. the context as to why it should be first (or even just refer to some comment elsewhere that explains it);
  2. that we _try _to keep the binlog_hton first naturally, but sometimes can't (and list what may cause it to break).

and same for the rollback branch

*/
for (int binlog_err= error=
run_binlog_first(thd, all, trans, is_real_trans, true);
ha_info; ha_info= ha_info_next)
{
my_error(ER_ERROR_DURING_COMMIT, MYF(0), err);
error= 1;
if (binlog_err)
Copy link
Contributor

@bnestere bnestere Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps run_binlog_first() should be taken out of the loop, so we don't need to check if (binlog_err) for each iteration. I think it would make the code cleaner and easier to debug too.

goto err;

goto err;
}
for (; ha_info; ha_info= ha_info_next)
{
handlerton *ht= ha_info->ht();
if ((err= ht->commit(ht, thd, all)))
{
Expand Down Expand Up @@ -2309,11 +2333,16 @@ int ha_rollback_trans(THD *thd, bool all)
if (is_real_trans) /* not a statement commit */
thd->stmt_map.close_transient_cursors();

for (; ha_info; ha_info= ha_info_next)
/*
Binlog hton must be called first regardless of its position
in trans->ha_list.
*/
for (error= run_binlog_first(thd, all, trans, is_real_trans, false);
ha_info; ha_info= ha_info_next)
{
int err;
handlerton *ht= ha_info->ht();
if ((err= ht->rollback(ht, thd, all)))
if (ht != binlog_hton && (err= ht->rollback(ht, thd, all)))
Copy link
Contributor

@bnestere bnestere Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does the rollback branch have

ht != binlog_hton

but the commit branch doesn't?

{
// cannot happen
my_error(ER_ERROR_DURING_ROLLBACK, MYF(0), err);
Expand Down
4 changes: 2 additions & 2 deletions sql/log.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,6 @@ static int binlog_savepoint_set(handlerton *hton, THD *thd, void *sv);
static int binlog_savepoint_rollback(handlerton *hton, THD *thd, void *sv);
static bool binlog_savepoint_rollback_can_release_mdl(handlerton *hton,
THD *thd);
static int binlog_rollback(handlerton *hton, THD *thd, bool all);
static int binlog_prepare(handlerton *hton, THD *thd, bool all);
static int binlog_start_consistent_snapshot(handlerton *hton, THD *thd);
static int binlog_flush_cache(THD *thd, binlog_cache_mngr *cache_mngr,
Expand Down Expand Up @@ -2439,7 +2438,7 @@ int binlog_commit(THD *thd, bool all, bool ro_1pc)

@see handlerton::rollback
*/
static int binlog_rollback(handlerton *hton, THD *thd, bool all)
int binlog_rollback(handlerton *hton, THD *thd, bool all)
{
DBUG_ENTER("binlog_rollback");

Expand Down Expand Up @@ -8471,6 +8470,7 @@ MYSQL_BIN_LOG::queue_for_group_commit(group_commit_entry *orig_entry)
bool
MYSQL_BIN_LOG::write_transaction_to_binlog_events(group_commit_entry *entry)
{
DEBUG_SYNC(entry->thd, "before_group_commit_queue");
int is_leader= queue_for_group_commit(entry);
#ifdef WITH_WSREP
/* commit order was released in queue_for_group_commit() call,
Expand Down
1 change: 1 addition & 0 deletions sql/log.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1277,6 +1277,7 @@ const char *
get_gtid_list_event(IO_CACHE *cache, Gtid_list_log_event **out_gtid_list);

int binlog_commit(THD *thd, bool all, bool is_ro_1pc= false);
int binlog_rollback(handlerton *hton, THD *thd, bool all);
int binlog_commit_by_xid(handlerton *hton, XID *xid);
int binlog_rollback_by_xid(handlerton *hton, XID *xid);
bool write_bin_log_start_alter(THD *thd, bool& partial_alter,
Expand Down