Skip to content

Conversation

@dr-m
Copy link
Contributor

@dr-m dr-m commented May 23, 2025

  • The Jira issue number for this PR is: MDEV-36863, MDEV-36868

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 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 (#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

  • This is a new feature or a refactoring, and the PR is based against the main branch.
  • This is a bug fix, and the PR is based against the earliest maintained branch in which the bug can be reproduced.

PR quality check

  • I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
  • For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

@dr-m dr-m self-assigned this May 23, 2025
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Contributor

@mariadb-DebarunBanerjee mariadb-DebarunBanerjee left a 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.

Comment on lines +2087 to +2093
block->n_hash_helps= 0;
# if defined UNIV_AHI_DEBUG || defined UNIV_DEBUG
block->n_pointers= 0;
# endif
block->index= nullptr;
#endif
Copy link
Contributor

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.

Copy link
Contributor Author

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).

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@mariadb-SaahilAlam
Copy link

The fix is working fine the reduced rqg simplifier did not hit TBR-2280 on feature branch , fix is working fine .
Also i have run standard Innodb test .No New issue found

dr-m added 2 commits May 28, 2025 14:33
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).
@dr-m dr-m force-pushed the 11.8-MDEV-36863 branch from 2980d90 to dbee7b7 Compare May 28, 2025 11:35
@dr-m dr-m merged commit dbee7b7 into 11.8 May 28, 2025
10 of 13 checks passed
@dr-m dr-m deleted the 11.8-MDEV-36863 branch May 28, 2025 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

5 participants