Skip to content

MDEV-33588 buf::Block_hint is a performance hog #3098

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

Merged
dr-m merged 1 commit into from
Apr 9, 2024
Merged

MDEV-33588 buf::Block_hint is a performance hog #3098

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 0 additions & 1 deletion storage/innobase/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,6 @@ SET(INNOBASE_SOURCES
btr/btr0pcur.cc
btr/btr0sea.cc
btr/btr0defragment.cc
buf/buf0block_hint.cc
buf/buf0buddy.cc
buf/buf0buf.cc
buf/buf0dblwr.cc
Expand Down
78 changes: 42 additions & 36 deletions storage/innobase/btr/btr0btr.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,8 @@ btr_root_block_get(
mtr_t* mtr, /*!< in: mtr */
dberr_t* err) /*!< out: error code */
{
ut_ad(mode != RW_NO_LATCH);

if (!index->table || !index->table->space)
{
*err= DB_TABLESPACE_NOT_FOUND;
Expand All @@ -285,13 +287,12 @@ btr_root_block_get(

if (UNIV_LIKELY(block != nullptr))
{
if (UNIV_UNLIKELY(mode == RW_NO_LATCH));
else if (!!page_is_comp(block->page.frame) !=
index->table->not_redundant() ||
btr_page_get_index_id(block->page.frame) != index->id ||
!fil_page_index_page_check(block->page.frame) ||
index->is_spatial() !=
(fil_page_get_type(block->page.frame) == FIL_PAGE_RTREE))
if (!!page_is_comp(block->page.frame) !=
index->table->not_redundant() ||
btr_page_get_index_id(block->page.frame) != index->id ||
!fil_page_index_page_check(block->page.frame) ||
index->is_spatial() !=
(fil_page_get_type(block->page.frame) == FIL_PAGE_RTREE))
{
*err= DB_PAGE_CORRUPTED;
block= nullptr;
Expand Down Expand Up @@ -568,6 +569,31 @@ btr_page_alloc_for_ibuf(
return new_block;
}

static MY_ATTRIBUTE((nonnull, warn_unused_result))
/** Acquire a latch on the index root page for allocating or freeing pages.
@param index index tree
@param mtr mini-transaction
@param err error code
@return root page
@retval nullptr if an error occurred */
buf_block_t *btr_root_block_sx(dict_index_t *index, mtr_t *mtr, dberr_t *err)
{
buf_block_t *root=
mtr->get_already_latched(page_id_t{index->table->space_id, index->page},
MTR_MEMO_PAGE_SX_FIX);
if (!root)
{
root= btr_root_block_get(index, RW_SX_LATCH, mtr, err);
if (UNIV_UNLIKELY(!root))
return root;
}
#ifdef BTR_CUR_HASH_ADAPT
else
ut_ad(!root->index || !root->index->freed());
#endif
return root;
}

/**************************************************************//**
Allocates a new file page to be used in an index tree. NOTE: we assume
that the caller has made the reservation for free extents!
Expand All @@ -589,21 +615,9 @@ btr_page_alloc_low(
page should be initialized. */
dberr_t* err) /*!< out: error code */
{
const auto savepoint= mtr->get_savepoint();
buf_block_t *root= btr_root_block_get(index, RW_NO_LATCH, mtr, err);
buf_block_t *root= btr_root_block_sx(index, mtr, err);
if (UNIV_UNLIKELY(!root))
return root;

const bool have_latch= mtr->have_u_or_x_latch(*root);
#ifdef BTR_CUR_HASH_ADAPT
ut_ad(!have_latch || !root->index || !root->index->freed());
#endif
mtr->rollback_to_savepoint(savepoint);

if (!have_latch &&
UNIV_UNLIKELY(!(root= btr_root_block_get(index, RW_SX_LATCH, mtr, err))))
return root;

fseg_header_t *seg_header= root->page.frame +
(level ? PAGE_HEADER + PAGE_BTR_SEG_TOP : PAGE_HEADER + PAGE_BTR_SEG_LEAF);
return fseg_alloc_free_page_general(seg_header, hint_page_no, file_direction,
Expand Down Expand Up @@ -696,24 +710,16 @@ dberr_t btr_page_free(dict_index_t* index, buf_block_t* block, mtr_t* mtr,
fil_space_t *space= index->table->space;
dberr_t err;

const auto savepoint= mtr->get_savepoint();
if (buf_block_t *root= btr_root_block_get(index, RW_NO_LATCH, mtr, &err))
if (buf_block_t *root= btr_root_block_sx(index, mtr, &err))
{
const bool have_latch= mtr->have_u_or_x_latch(*root);
#ifdef BTR_CUR_HASH_ADAPT
ut_ad(!have_latch || !root->index || !root->index->freed());
#endif
mtr->rollback_to_savepoint(savepoint);
if (have_latch ||
(root= btr_root_block_get(index, RW_SX_LATCH, mtr, &err)))
err= fseg_free_page(&root->page.frame[blob ||
page_is_leaf(block->page.frame)
? PAGE_HEADER + PAGE_BTR_SEG_LEAF
: PAGE_HEADER + PAGE_BTR_SEG_TOP],
space, page, mtr, space_latched);
err= fseg_free_page(&root->page.frame[blob ||
page_is_leaf(block->page.frame)
? PAGE_HEADER + PAGE_BTR_SEG_LEAF
: PAGE_HEADER + PAGE_BTR_SEG_TOP],
space, page, mtr, space_latched);
if (err == DB_SUCCESS)
buf_page_free(space, page, mtr);
}
if (err == DB_SUCCESS)
buf_page_free(space, page, mtr);

/* The page was marked free in the allocation bitmap, but it
should remain exclusively latched until mtr_t::commit() or until it
Expand Down
2 changes: 1 addition & 1 deletion storage/innobase/btr/btr0bulk.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -837,7 +837,7 @@ PageBulk::release()
m_block->page.fix();

/* No other threads can modify this block. */
m_modify_clock = buf_block_get_modify_clock(m_block);
m_modify_clock = m_block->modify_clock;

m_mtr.commit();
}
Expand Down
90 changes: 62 additions & 28 deletions storage/innobase/btr/btr0cur.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -935,7 +935,7 @@ static inline page_cur_mode_t btr_cur_nonleaf_mode(page_cur_mode_t mode)
return PAGE_CUR_LE;
}

static MY_ATTRIBUTE((nonnull))
MY_ATTRIBUTE((nonnull,warn_unused_result))
/** Acquire a latch on the previous page without violating the latching order.
@param block index page
@param page_id page identifier with valid space identifier
Expand All @@ -946,56 +946,90 @@ static MY_ATTRIBUTE((nonnull))
@retval 0 if an error occurred
@retval 1 if the page could be latched in the wrong order
@retval -1 if the latch on block was temporarily released */
int btr_latch_prev(buf_block_t *block, page_id_t page_id, ulint zip_size,
rw_lock_type_t rw_latch, mtr_t *mtr, dberr_t *err)
static int btr_latch_prev(buf_block_t *block, page_id_t page_id,
ulint zip_size,
rw_lock_type_t rw_latch, mtr_t *mtr, dberr_t *err)
{
ut_ad(rw_latch == RW_S_LATCH || rw_latch == RW_X_LATCH);
ut_ad(page_id.space() == block->page.id().space());

const auto prev_savepoint= mtr->get_savepoint();
ut_ad(block == mtr->at_savepoint(prev_savepoint - 1));

page_id.set_page_no(btr_page_get_prev(block->page.frame));
const page_t *const page= block->page.frame;
page_id.set_page_no(btr_page_get_prev(page));
/* We are holding a latch on the current page.

We will start by buffer-fixing the left sibling. Waiting for a latch
on it while holding a latch on the current page could lead to a
deadlock, because another thread could hold that latch and wait for
a right sibling page latch (the current page).

If there is a conflict, we will temporarily release our latch on the
current block while waiting for a latch on the left sibling. The
buffer-fixes on both blocks will prevent eviction. */

retry:
buf_block_t *prev= buf_page_get_gen(page_id, zip_size, RW_NO_LATCH, nullptr,
BUF_GET, mtr, err, false);
if (UNIV_UNLIKELY(!prev))
return 0;

int ret= 1;
if (UNIV_UNLIKELY(rw_latch == RW_S_LATCH))
static_assert(MTR_MEMO_PAGE_S_FIX == mtr_memo_type_t(BTR_SEARCH_LEAF), "");
static_assert(MTR_MEMO_PAGE_X_FIX == mtr_memo_type_t(BTR_MODIFY_LEAF), "");

if (rw_latch == RW_S_LATCH
? prev->page.lock.s_lock_try() : prev->page.lock.x_lock_try())
{
if (UNIV_LIKELY(prev->page.lock.s_lock_try()))
mtr->lock_register(prev_savepoint, mtr_memo_type_t(rw_latch));
if (UNIV_UNLIKELY(prev->page.id() != page_id))
{
mtr->lock_register(prev_savepoint, MTR_MEMO_PAGE_S_FIX);
goto prev_latched;
fail:
/* the page was just read and found to be corrupted */
mtr->rollback_to_savepoint(prev_savepoint);
return 0;
}
block->page.lock.s_unlock();
}
else
{
if (UNIV_LIKELY(prev->page.lock.x_lock_try()))
ut_ad(mtr->at_savepoint(mtr->get_savepoint() - 1)->page.id() == page_id);
mtr->release_last_page();
if (rw_latch == RW_S_LATCH)
block->page.lock.s_unlock();
else
block->page.lock.x_unlock();

prev= buf_page_get_gen(page_id, zip_size, rw_latch, prev,
BUF_GET, mtr, err);
if (rw_latch == RW_S_LATCH)
block->page.lock.s_lock();
else
block->page.lock.x_lock();

const page_id_t prev_page_id= page_id;
page_id.set_page_no(btr_page_get_prev(page));

if (UNIV_UNLIKELY(page_id != prev_page_id))
{
mtr->lock_register(prev_savepoint, MTR_MEMO_PAGE_X_FIX);
goto prev_latched;
mtr->release_last_page();
if (page_id.page_no() == FIL_NULL)
return -1;
goto retry;
}
block->page.lock.x_unlock();

if (UNIV_UNLIKELY(!prev))
goto fail;

ret= -1;
}

ret= -1;
mtr->lock_register(prev_savepoint - 1, MTR_MEMO_BUF_FIX);
mtr->rollback_to_savepoint(prev_savepoint);
prev= buf_page_get_gen(page_id, zip_size, rw_latch, prev,
BUF_GET, mtr, err, false);
if (UNIV_UNLIKELY(!prev))
return 0;
mtr->upgrade_buffer_fix(prev_savepoint - 1, rw_latch);

prev_latched:
if (memcmp_aligned<2>(FIL_PAGE_TYPE + prev->page.frame,
FIL_PAGE_TYPE + block->page.frame, 2) ||
memcmp_aligned<2>(PAGE_HEADER + PAGE_INDEX_ID + prev->page.frame,
PAGE_HEADER + PAGE_INDEX_ID + block->page.frame, 8) ||
page_is_comp(prev->page.frame) != page_is_comp(block->page.frame))
const page_t *const p= prev->page.frame;
if (memcmp_aligned<4>(FIL_PAGE_NEXT + p, FIL_PAGE_OFFSET + page, 4) ||
memcmp_aligned<2>(FIL_PAGE_TYPE + p, FIL_PAGE_TYPE + page, 2) ||
memcmp_aligned<2>(PAGE_HEADER + PAGE_INDEX_ID + p,
PAGE_HEADER + PAGE_INDEX_ID + page, 8) ||
page_is_comp(p) != page_is_comp(page))
{
ut_ad("corrupted" == 0); // FIXME: remove this
Comment on lines +1027 to 1034

@dr-m dr-m Mar 11, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Matthias reported an assertion failure here, due to the FIL_PAGE_NEXT check. In the rr replay trace that I analyzed, the cause is that while btr_latch_prev() was waiting for the previous page latch in buf_page_get_gen(), there was a btr_page_split_and_insert() executed in another thread.

I think that we must implement a retry logic in case the FIL_PAGE_PREV of our current page changed while we were not holding a page latch.

@dr-m dr-m Mar 11, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

a4389ec should address this.

@mariadb-DebarunBanerjee mariadb-DebarunBanerjee Mar 14, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Indeed, latching right to left is not straight forward. AFAIK, we don't need such latching order in general case. It is needed for backward scan and perhaps some other special cases. In 8.0, for pessimistic operation, the order is allowed to reverse for non-leaf pages with certain strict latching rules (Yasufulmi's improvement SX latching).

Can you tell me the scenario when we have this flow ? Yet to see the details but hope it is not in genera; DML/Query path if the design is not significantly different from 5.7/8.0.

@dr-m dr-m Mar 14, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The SX latch mode was implemented in MySQL 5.7 already. I renamed it to U in the sux_lock. It is not free from hangs; see f209647 for a prominent example.

This code is mostly being used in btr_pcur_move_backward_from_page() but also in the change buffer (BTR_MODIFY_PREV).

*err= DB_CORRUPTION;
Expand Down
Loading