Skip to content

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.

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 2 commits into from
May 28, 2025
Merged

MDEV-36863, MDEV-36868: Regressions when shrinking innodb_buffer_pool_size #4063

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d953f2c
MDEV-36868: Inconsistency when shrinking innodb_buffer_pool_size
dr-m May 28, 2025
dbee7b7
MDEV-36863 InnoDB: Failing assertion: !block->n_hash_helps after fail…
dr-m May 28, 2025
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
9 changes: 9 additions & 0 deletions mysql-test/suite/innodb/r/innodb_buffer_pool_resize.result
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,15 @@ create table t1 (id int primary key, val int not null)
ENGINE=InnoDB ROW_FORMAT=COMPRESSED;
SET STATEMENT foreign_key_checks=0, unique_checks=0 FOR
INSERT INTO t1 SELECT seq*4,seq*4 FROM seq_1_to_262144;
SELECT @@GLOBAL.innodb_adaptive_hash_index;
@@GLOBAL.innodb_adaptive_hash_index
1
SET STATEMENT max_statement_time=1e-9 FOR
SET GLOBAL innodb_buffer_pool_size = 7340032;
SELECT @@GLOBAL.innodb_adaptive_hash_index;
@@GLOBAL.innodb_adaptive_hash_index
1
FOUND 1 /innodb_buffer_pool_size=7m.*resized from|innodb_buffer_pool_size change aborted/ in mysqld.1.err
set global innodb_buffer_pool_size = 7340032;
select count(val) from t1;
count(val)
Expand Down
10 changes: 10 additions & 0 deletions mysql-test/suite/innodb/r/innodb_buffer_pool_resize_temporary.result
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,16 @@ SET @save_limit=@@GLOBAL.innodb_limit_optimistic_insert_debug;
SET @save_size=@@GLOBAL.innodb_buffer_pool_size;
SET GLOBAL innodb_limit_optimistic_insert_debug=2;
SET GLOBAL innodb_buffer_pool_size=16777216;
call mtr.add_suppression("innodb_buffer_pool_size change aborted");
SET @old_innodb_adaptive_hash_index = @@innodb_adaptive_hash_index;
SET GLOBAL innodb_adaptive_hash_index = ON;
SET STATEMENT debug_dbug='+d,buf_shrink_fail' FOR
SET GLOBAL innodb_buffer_pool_size=8388608;
ERROR HY000: innodb_buffer_pool_size change aborted
SELECT @@GLOBAL.innodb_adaptive_hash_index,@@GLOBAL.innodb_buffer_pool_size;
@@GLOBAL.innodb_adaptive_hash_index @@GLOBAL.innodb_buffer_pool_size
1 16777216
SET GLOBAL innodb_adaptive_hash_index = @old_innodb_adaptive_hash_index;
CREATE TEMPORARY TABLE t1 (a INT PRIMARY KEY) ENGINE=InnoDB;
INSERT INTO t1 SELECT seq FROM seq_1_to_200;
SET GLOBAL innodb_max_purge_lag_wait=0;
Expand Down
12 changes: 12 additions & 0 deletions mysql-test/suite/innodb/t/innodb_buffer_pool_resize.test
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
--source include/have_innodb.inc
--source include/have_sequence.inc
--source include/not_embedded.inc # there are no messages in mysqld.1.err

--echo #
--echo # MDEV-29445: Reorganize buffer pool (and remove chunks)
Expand Down Expand Up @@ -37,6 +38,17 @@ INSERT INTO t1 SELECT seq*4,seq*4 FROM seq_1_to_262144;
SET GLOBAL innodb_read_only_compressed=@save_innodb_read_only_compressed;
--enable_query_log

SELECT @@GLOBAL.innodb_adaptive_hash_index;
--error 0,ER_WRONG_USAGE
SET STATEMENT max_statement_time=1e-9 FOR
SET GLOBAL innodb_buffer_pool_size = 7340032;
SELECT @@GLOBAL.innodb_adaptive_hash_index;

--let SEARCH_FILE= $MYSQLTEST_VARDIR/log/mysqld.1.err
--let SEARCH_PATTERN= InnoDB: Trying to shrink innodb_buffer_pool_size=7m
--let SEARCH_PATTERN= innodb_buffer_pool_size=7m.*resized from|innodb_buffer_pool_size change aborted
--source include/search_pattern_in_file.inc

# Attempt to shrink the buffer pool. This may occasionally fail.
--error 0,ER_WRONG_USAGE
set global innodb_buffer_pool_size = 7340032;
Expand Down
10 changes: 10 additions & 0 deletions mysql-test/suite/innodb/t/innodb_buffer_pool_resize_temporary.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,16 @@ SET @save_size=@@GLOBAL.innodb_buffer_pool_size;
SET GLOBAL innodb_limit_optimistic_insert_debug=2;
SET GLOBAL innodb_buffer_pool_size=16777216;

call mtr.add_suppression("innodb_buffer_pool_size change aborted");

SET @old_innodb_adaptive_hash_index = @@innodb_adaptive_hash_index;
SET GLOBAL innodb_adaptive_hash_index = ON;
--error ER_WRONG_USAGE
SET STATEMENT debug_dbug='+d,buf_shrink_fail' FOR
SET GLOBAL innodb_buffer_pool_size=8388608;
SELECT @@GLOBAL.innodb_adaptive_hash_index,@@GLOBAL.innodb_buffer_pool_size;
SET GLOBAL innodb_adaptive_hash_index = @old_innodb_adaptive_hash_index;

CREATE TEMPORARY TABLE t1 (a INT PRIMARY KEY) ENGINE=InnoDB;
INSERT INTO t1 SELECT seq FROM seq_1_to_200;

Expand Down
35 changes: 20 additions & 15 deletions storage/innobase/buf/buf0buf.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1614,6 +1614,7 @@ ATTRIBUTE_COLD buf_pool_t::shrink_status buf_pool_t::shrink(size_t size)
noexcept
{
mysql_mutex_assert_owner(&mutex);
DBUG_EXECUTE_IF("buf_shrink_fail", return SHRINK_ABORT;);
buf_load_abort();

if (!n_blocks_to_withdraw)
Expand Down Expand Up @@ -2014,25 +2015,12 @@ ATTRIBUTE_COLD void buf_pool_t::resize(size_t size, THD *thd) noexcept
if (ahi_disabled)
btr_search.enable(true);
#endif
mysql_mutex_lock(&LOCK_global_system_variables);
bool resized= n_blocks_removed < 0;
if (n_blocks_removed > 0)
{
mysql_mutex_lock(&mutex);
resized= size_in_bytes == old_size;
if (resized)
{
size_in_bytes_requested= size;
size_in_bytes= size;
}
mysql_mutex_unlock(&mutex);
}

if (resized)
if (n_blocks_removed)
sql_print_information("InnoDB: innodb_buffer_pool_size=%zum (%zu pages)"
" resized from %zum (%zu pages)",
size >> 20, n_blocks_new, old_size >> 20,
old_blocks);
mysql_mutex_lock(&LOCK_global_system_variables);
}
else
{
Expand Down Expand Up @@ -2090,11 +2078,28 @@ ATTRIBUTE_COLD void buf_pool_t::resize(size_t size, THD *thd) noexcept
ut_d(b->in_free_list= true);
ut_ad(b->state() == buf_page_t::NOT_USED);
b->lock.init();
#ifdef BTR_CUR_HASH_ADAPT
/* Clear the AHI fields, because buf_block_init_low() expects
these to be zeroed. These were not cleared when we relocated
the block to withdrawn. Had we successfully shrunk the buffer pool,
all this virtual memory would have been zeroed or made unaccessible,
and on a subsequent buffer pool extension it would be zero again. */
buf_block_t *block= reinterpret_cast<buf_block_t*>(b);
block->n_hash_helps= 0;
# if defined UNIV_AHI_DEBUG || defined UNIV_DEBUG
block->n_pointers= 0;
# endif
block->index= nullptr;
#endif
Comment on lines +2088 to +2093

@mariadb-DebarunBanerjee mariadb-DebarunBanerjee May 26, 2025

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.

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.

@dr-m dr-m May 28, 2025

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.

Bringing back the initialization could slow down InnoDB startup on a large buffer pool (MDEV-25340).

@dr-m dr-m May 28, 2025

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.

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.

@mariadb-DebarunBanerjee mariadb-DebarunBanerjee May 28, 2025

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.

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.

}

mysql_mutex_unlock(&mutex);
my_printf_error(ER_WRONG_USAGE, "innodb_buffer_pool_size change aborted",
MYF(ME_ERROR_LOG));
#ifdef BTR_CUR_HASH_ADAPT
if (ahi_disabled)
btr_search.enable(true);
#endif
mysql_mutex_lock(&LOCK_global_system_variables);
}

Expand Down