Hi, Nikita, On May 03, Nikita Malyavin wrote:
revision-id: c5ce597f06a (mariadb-11.0.1-115-gc5ce597f06a) parent(s): da5a72f32d4 author: Nikita Malyavin committer: Nikita Malyavin timestamp: 2023-05-01 01:15:41 +0300 message:
MDEV-31043 ER_KEY_NOT_FOUND upon concurrent ALTER and transaction
Non-transactional engines changes are visible immediately the row operation succeeds, so we should apply the changes to the online buffer immediately after the statement ends.
diff --git a/sql/log.cc b/sql/log.cc index f30ce3bcbd1..1d98455c2b3 100644 --- a/sql/log.cc +++ b/sql/log.cc @@ -7701,23 +7701,35 @@ static int binlog_online_alter_end_trans(THD *thd, bool all, bool commit) auto *binlog= cache.sink_log; DBUG_ASSERT(binlog);
- bool do_commit= commit || !cache.hton->rollback || - cache.hton->flags & HTON_NO_ROLLBACK; // Aria + bool do_commit= (commit && is_ending_transaction) + || cache.hton->flags & HTON_NO_ROLLBACK // Aria + || !cache.hton->rollback; + + error= binlog_flush_pending_rows_event(thd, false, true, binlog, &cache);
you've lost the useful comment about STMT_END. and you flush events for rollback too that seems like a waste. I personally would've kept the old structure of if()'s. Like - bool do_commit= commit || !cache.hton->rollback || + bool non_trans= !cache.hton->rollback || and - if (do_commit) + if (commit || non_trans) with - if (is_ending_transaction) + if (is_ending_transaction || non_trans) That would've called binlog flush only where it needs to be.
+ if (do_commit) { - // do not set STMT_END for last event to leave table open in altering thd - error= binlog_flush_pending_rows_event(thd, false, true, binlog, &cache); - if (is_ending_transaction) + /* + If the cache wasn't reinited to write, then it remains empty after + the last write. + */ + if (cache.cache_log.type != READ_CACHE && !error)
this is a confusing new condition. are you trying to avoid locking a mutex for an empty cache? If yes, you can check my_b_bytes_in_cache(), that'd be more clear.
{ mysql_mutex_lock(binlog->get_log_lock()); error= binlog->write_cache(thd, &cache.cache_log); mysql_mutex_unlock(binlog->get_log_lock()); } - else - cache.store_prev_position(); } - else if (!is_ending_transaction) + else if (!commit) // rollback + { cache.restore_prev_position(); + } + else
add // trans engine, end of statement
+ { + DBUG_ASSERT(!is_ending_transaction); + cache.store_prev_position(); + }
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org