Commit 0fbcb0a2 authored by Marko Mäkelä's avatar Marko Mäkelä

MDEV-29383 Assertion mysql_mutex_assert_owner(&log_sys.flush_order_mutex) failed in mtr_t::commit()

In commit 0b47c126 (MDEV-13542)
a few calls to mtr_t::memo_push() were moved before a write latch
on the page was acquired. This introduced a race condition:

1. is_block_dirtied() returned false to mtr_t::memo_push()
2. buf_page_t::write_complete() was executed, the block marked clean,
and a page latch released
3. The page latch was acquired by the caller of mtr_t::memo_push(),
and mtr_t::m_made_dirty was not set even though the block is in
a clean state.

The impact of this race condition is that crash recovery and backups
may fail.

btr_cur_latch_leaves(), btr_store_big_rec_extern_fields(),
btr_free_externally_stored_field(), trx_purge_free_segment():
Acquire the page latch before invoking mtr_t::memo_push().
This fixes the regression caused by MDEV-13542.

Side note: It would suffice to set mtr_t::m_made_dirty at the time
we set the MTR_MEMO_MODIFY flag for a block. Currently that flag is
unnecessarily set if a mini-transaction acquires a page latch on
a page that is in a clean state, and will not actually modify the block.
This may cause unnecessary acquisitions of log_sys.flush_order_mutex
on mtr_t::commit().

mtr_t::free(): If the block had been exclusively latched in this
mini-transaction, set the m_made_dirty flag so that the flush order mutex
will be acquired during mtr_t::commit(). This should have been part of
commit 4179f93d (MDEV-18976).
It was necessary to change mtr_t::free() so that
WriteOPT_PAGE_CHECKSUM::operator() would be able to avoid writing
checksums for freed pages.
parent 76bb671e
...@@ -278,10 +278,10 @@ btr_cur_latch_leaves( ...@@ -278,10 +278,10 @@ btr_cur_latch_leaves(
latch_leaves->blocks[1] = block; latch_leaves->blocks[1] = block;
} }
mtr->memo_push(block, MTR_MEMO_PAGE_X_FIX);
block->page.fix(); block->page.fix();
block->page.lock.x_lock(); block->page.lock.x_lock();
mtr->memo_push(block, MTR_MEMO_PAGE_X_FIX);
#ifdef BTR_CUR_HASH_ADAPT #ifdef BTR_CUR_HASH_ADAPT
ut_ad(!btr_search_check_marked_free_index(block)); ut_ad(!btr_search_check_marked_free_index(block));
#endif #endif
...@@ -7019,10 +7019,11 @@ btr_store_big_rec_extern_fields( ...@@ -7019,10 +7019,11 @@ btr_store_big_rec_extern_fields(
mtr.start(); mtr.start();
index->set_modified(mtr); index->set_modified(mtr);
mtr.set_log_mode_sub(*btr_mtr); mtr.set_log_mode_sub(*btr_mtr);
mtr.memo_push(rec_block, MTR_MEMO_PAGE_X_FIX);
rec_block->page.fix(); rec_block->page.fix();
rec_block->page.lock.x_lock(); rec_block->page.lock.x_lock();
mtr.memo_push(rec_block, MTR_MEMO_PAGE_X_FIX);
#ifdef BTR_CUR_HASH_ADAPT #ifdef BTR_CUR_HASH_ADAPT
ut_ad(!btr_search_check_marked_free_index(rec_block)); ut_ad(!btr_search_check_marked_free_index(rec_block));
#endif #endif
...@@ -7397,9 +7398,10 @@ btr_free_externally_stored_field( ...@@ -7397,9 +7398,10 @@ btr_free_externally_stored_field(
/* The buffer pool block containing the BLOB pointer is /* The buffer pool block containing the BLOB pointer is
exclusively latched by local_mtr. To satisfy some design exclusively latched by local_mtr. To satisfy some design
constraints, we must recursively latch it in mtr as well. */ constraints, we must recursively latch it in mtr as well. */
mtr.memo_push(block, MTR_MEMO_PAGE_X_FIX);
block->fix(); block->fix();
block->page.lock.x_lock(); block->page.lock.x_lock();
mtr.memo_push(block, MTR_MEMO_PAGE_X_FIX);
#ifdef BTR_CUR_HASH_ADAPT #ifdef BTR_CUR_HASH_ADAPT
ut_ad(!btr_search_check_marked_free_index(block)); ut_ad(!btr_search_check_marked_free_index(block));
#endif #endif
......
...@@ -1599,8 +1599,10 @@ void mtr_t::free(const fil_space_t &space, uint32_t offset) ...@@ -1599,8 +1599,10 @@ void mtr_t::free(const fil_space_t &space, uint32_t offset)
if (is_logged()) if (is_logged())
{ {
m_memo.for_each_block_in_reverse CIterate<MarkFreed> mf{MarkFreed{{space.id, offset}}};
(CIterate<MarkFreed>((MarkFreed{{space.id, offset}}))); m_memo.for_each_block_in_reverse(mf);
if (mf.functor.freed && !m_made_dirty)
m_made_dirty= is_block_dirtied(mf.functor.freed);
m_log.close(log_write<FREE_PAGE>({space.id, offset}, nullptr)); m_log.close(log_write<FREE_PAGE>({space.id, offset}, nullptr));
} }
} }
...@@ -399,11 +399,11 @@ static dberr_t trx_purge_free_segment(trx_rseg_t *rseg, fil_addr_t hdr_addr) ...@@ -399,11 +399,11 @@ static dberr_t trx_purge_free_segment(trx_rseg_t *rseg, fil_addr_t hdr_addr)
mtr.commit(); mtr.commit();
mtr.start(); mtr.start();
mtr.flag_modified(); mtr.flag_modified();
mtr.memo_push(rseg_hdr, MTR_MEMO_PAGE_X_FIX);
mtr.memo_push(block, MTR_MEMO_PAGE_X_MODIFY);
rseg->latch.wr_lock(SRW_LOCK_CALL); rseg->latch.wr_lock(SRW_LOCK_CALL);
rseg_hdr->page.lock.x_lock(); rseg_hdr->page.lock.x_lock();
block->page.lock.x_lock(); block->page.lock.x_lock();
mtr.memo_push(rseg_hdr, MTR_MEMO_PAGE_X_FIX);
mtr.memo_push(block, MTR_MEMO_PAGE_X_MODIFY);
} }
/* The page list may now be inconsistent, but the length field /* The page list may now be inconsistent, but the length field
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment