-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
MDEV-36863, MDEV-36868: Regressions when shrinking innodb_buffer_pool_size #4063
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.
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
Conversation
|
|
mariadb-DebarunBanerjee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I have a suggestion for your consideration.
| block->n_hash_helps= 0; | ||
| # if defined UNIV_AHI_DEBUG || defined UNIV_DEBUG | ||
| block->n_pointers= 0; | ||
| # endif | ||
| block->index= nullptr; | ||
| #endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, the initialization looks good.
bug should not be reproducible in 10.11 or 11.4 because buf_block_init_low() is actually initializing the adaptive hash index fields. Only starting with MDEV-35049 we are not initializing the fields
After reading this. I feel it was not a good idea to remove such initialization from buf_block_init_low(). Such micro-optimizations tend incur more cost in long run. Please consider having the initialization back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bringing back the initialization could slow down InnoDB startup on a large buffer pool (MDEV-25340).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested the following patch:
diff --git a/storage/innobase/buf/buf0buf.cc b/storage/innobase/buf/buf0buf.cc
index 3fce60922b5..e1167ef0679 100644
--- a/storage/innobase/buf/buf0buf.cc
+++ b/storage/innobase/buf/buf0buf.cc
@@ -1189,6 +1189,8 @@ static void buf_block_init_low(buf_block_t *block) noexcept
ut_a(!block->n_pointers);
ut_a(!block->n_hash_helps);
# endif
+ block->index= nullptr;
+ block->n_hash_helps= 0;
}
#else /* BTR_CUR_HASH_ADAPT */
inline void buf_block_init_low(buf_block_t*) {}This would (among others) increase the size of the function buf_block_t::initialise() on AMD64: 11 bytes for movq $0,0x90(%rdi) and 9 bytes for movw $0,0x8c(%rdi), and some bytes saved elsewhere. (I don’t know why an immediate constant was being used instead of a register.) This function is being invoked during normal operation in buf_page_init_for_read() and buf_page_create_low(). There is no change to buf_pool_t::create() after all. In debug builds that function checks that the block descriptors are zero-initialized, and in release builds we only perform minimal initialization:
for (byte *frame= reinterpret_cast<byte*>(extent) +
first_frame_in_extent[ssize];
block < extent_end; block++, frame+= srv_page_size)
{
ut_ad(!memcmp(block, field_ref_zero, sizeof *block));
block->page.frame= frame;
block->page.lock.init();
UT_LIST_ADD_LAST(free, &block->page);
ut_d(block->page.in_free_list= true);
}You might argue that the overhead of the two extra instructions is negligible and page creation or page reads are rare. I prefer to have additional checks in debug builds and try to design data structures so that they are all zero in their neutral state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to have additional checks in debug builds and try to design data structures so that they are all zero in their neutral state.
yes, I would have preferred the debug asserts put there along with the initialization. I doubt we could see any visible impact but I could be wrong. I don't insist on this change and I have already approved the patch.
|
buf_pool_t::resize(): After successfully shrinking the buffer pool, announce the success. The size had already been updated in shrunk(). After failing to shrink the buffer pool, re-enable the adaptive hash index if it had been enabled. Reviewed by: Debarun Banerjee
…ing to shrink innodb_buffer_pool_size buf_pool_t::resize(): After failing to shrink the buffer pool, clear any adaptive hash index fields for blocks that were restored from buf_pool.withdrawn to buf_pool.free, so that they will be in the expected state when the blocks will be allocated later. This fixes a logic conflict between commit 4dcb1b5 (MDEV-35049) and commit b692342 (MDEV-29445).
Description
This includes #4062. This also fixes a merge conflict between #3826 and #3562 that affects the non-default setting
innodb_adaptive_hash_index=ON.buf_pool_t::resize(): After failing to shrink the buffer pool, clear any adaptive hash index fields for blocks that were restored frombuf_pool.withdrawntobuf_pool.free, so that they will be in the expected state when the blocks will be allocated later. This fixes a logic conflict between commit 4dcb1b5 (#3562, MDEV-35049) and commit b692342 (#3826, MDEV-29445).Release Notes
If this fix is included in the 11.8.2 release, nothing needs to be noted.
How can this PR be tested?
Not sure. @mariadb-SaahilAlam reproduced this while testing #4014. I tried and failed to reproduce this myself.
Basing the PR against the correct MariaDB version
mainbranch.PR quality check