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 secur...@mariadb.org _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp