Re: [Maria-developers] c5ce597f06a: MDEV-31043 ER_KEY_NOT_FOUND upon concurrent ALTER and transaction
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
Hello Sergei! On Wed, 3 May 2023 at 15:29, Sergei Golubchik <serg@mariadb.org> wrote:
you've lost the useful comment about STMT_END.
Okay. I just thought that from the reader's perspective it doesn't look important. But you are the reader, and you say it's important. Bringing back.
and you flush events for rollback too that seems like a waste.
I can avoid it, but then we'll have to clear the pending event. Ok i'll do it, i think i didn't see the need to optimize these few bits of rollback.
I personally would've kept the old structure of if()'s.
I like the new one more, as it resembles the symmetry between storing and restoring binlog's position, and besides it has fewer nesting. See my new commit b804bcbc6 <here: https://github.com/MariaDB/server/commit/b804bcbc64dfc0bc7b244eaebbd64c987bdb5a93>, or the diff with the old one, here <https://github.com/MariaDB/server/compare/c5ce597f06a...b804bcbc64dfc0bc7b244eaebbd64c987bdb5a93> . -- Yours truly, Nikita Malyavin
Hi, Nikita, On May 04, Nikita Malyavin wrote:
See my new commit b804bcbc6
You forgot to address the following:
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
You forgot to address the following:
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.
Pardon. My subconscious ignores uncomfortable questions:) No, i didn't. reinit_io_cache is called inside
error= binlog->write_cache(thd, &cache.cache_log);
This reinit function behaves differently when it reinits WRITE_CACHE -> READ_CACHE vs READ_CACHE -> READ_CACHE. The latter happens when we issue COMMIT when non-trans trable was involved. This simply leads to a crash. I didn't investigate for something smarter, so yes, checking my_b_bytes_in_cache() may also work, I suppose. Let's see... Anyway, calling Event_log::write_cache with cache.cache_log.type == READ_CACHE is error-prone, so I'll leave it as an assertion. Sergei, once I reiterated to this MDEV, found out that I was wrong about this:
then we'll have to clear the pending event.
The pending event is deleted during truncate() (called from restore_prev_position()). Also I noticed another reinit_io_cache there. I think it's not vulnerable to the problem above, but it may end up in a real OS ftell. Even though our file is temporary, I will add at least an assertion to the zero reinit_io_cache result. The commit to see is 4747e9cd584ee <https://github.com/MariaDB/server/commit/4747e9cd584ee982b0033061957d76eb09b13554> . -- Yours truly, Nikita Malyavin
participants (2)
-
Nikita Malyavin -
Sergei Golubchik