Skip to content
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

Jump to bottom

MDEV-35889 set information_schema.system_variables NUMERIC_MIN_VALUE for the innodb_buffer_pool_size system variable based on innodb_page_size #3777

Open
wants to merge 1 commit into
base: 10.5
Choose a base branch
from
Open

MDEV-35889 set information_schema.system_variables NUMERIC_MIN_VALUE for the innodb_buffer_pool_size system variable based on innodb_page_size #3777

wants to merge 1 commit into from

Conversation

grooverdan
Copy link
Member

  • The Jira issue number for this PR is: MDEV-35889

Description

reasons for a new function plugin_opt_set_limits_by_name:

  • plugin_opt_set_limits looks like it will do the job, however struct my_option * never gets exposed to the plugin to use. Also to preserve the plugin_opt_set_limits if somehow an plugin did use the interface.
  • exposing intern_find_sys_var to a plugin looks like a layering violation, but otherwise including sql/sys_vars_shared.h does a compile error on THD (for other functions there).

The innodb_buffer_pool_size is dependent on the innodb_page_size. While the minimum is enforced for resizing the minimum isn't exposed by the information_schema.system_variables table.

This creates a plugin function plugin_opt_set_limits_by_name that allows plugins to adjust the limits of their system variables during plugin initialization.

Release Notes

information.system_variables no includes the right NUMBERIC_MIN_VALUE for innodb_buffer_pool_size, regardless of which innodb_page_size is used.

How can this PR be tested?

mtr case included

If the changes are not amenable to automated testing, please explain why not and carefully describe how to test manually.

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.

@grooverdan grooverdan added the MariaDB Foundation Pull requests created by MariaDB Foundation label Jan 20, 2025
@grooverdan grooverdan requested review from vuvova and dr-m January 20, 2025 13:06
vuvova
vuvova requested changes Jan 20, 2025
View reviewed changes
Copy link
Member

@vuvova vuvova left a comment

Choose a reason for hiding this comment

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

I think it went the wrong way. Instead of adding a function for plugin to call to adjust the limits, I'd add an assert into OPTION_SET_LIMITS that max, min, and default values are divisible by the block size. And the same into Sys_var_integer::Sys_var_integer. Also, other asserts from Sys_var_integer could make sense in OPTION_SET_LIMITS too (like min < max)

@grooverdan
Copy link
Member Author

I think it went the wrong way. Instead of adding a function for plugin to call to adjust the limits, I'd add an assert into OPTION_SET_LIMITS that max, min, and default values are divisible by the block size. And the same into Sys_var_integer::Sys_var_integer. Also, other asserts from Sys_var_integer could make sense in OPTION_SET_LIMITS too (like min < max)

I don't follow how assertions are solving a problem. I'm not solving MDEV-35890 here. The innodb buffer pool size is dependent on the page size, set in innodb_init_params, but Sys_vars are initialized before this is called.

Another deeper change possibility that I think is more correct is changing the sys_var min/max/default to pointers and point them to the st_mysql_sys_var values rather than copying them. This saves doing a plugin_opt_set_limits call, but I haven't looked all the way though here.

And missed including test result change. opps.

@vuvova
Copy link
Member

vuvova commented Jan 20, 2025

Variable metadata (for a integer sysvar) include min and max values and a block size. It's reasonable to expect these values to be consistent with each other. min should be less than max. both min and max should be divisible by block size.

Asserts would ensure that variable metadata is internally consistent.

@grooverdan grooverdan force-pushed the 10.5-innodb_page_size_minimium branch from 5abd367 to b2324fa Compare January 24, 2025 07:58
@grooverdan
MDEV-35889 set information_schema.system_variables NUMERIC_MIN_VALUE ...
7e51e20 
for the innodb_buffer_pool_size system variable based on innodb_page_size.

The innodb_buffer_pool_size is dependent on the innodb_page_size. While
the minimum is enforced for resizing the minimum isn't exposed by the
information_schema.system_variables table.

Add a bunch of assertions to ensure that the plugin relations between
default, minimum, maximum and block size hold.

Expose the plugin_var point from sys_var and while retreiving fill_sysvars
update the limits in case the plugin happens to have changed them.
@grooverdan grooverdan force-pushed the 10.5-innodb_page_size_minimium branch from b2324fa to 7e51e20 Compare January 24, 2025 08:33
@grooverdan
Copy link
Member Author

Added assertions.

Replaced previous retrieval, with explicitly updated the limits when the information schema is retrieved.

The min value now maps to what innodb changes the minium to in innodb_init_params as tested by the innodb.restart test change.

MariaDB [(none)]>  SELECT @@innodb_buffer_pool_size, @@innodb_page_size, information_schema.system_variables.NUMERIC_MIN_VALUE FROM information_schema.system_variables where information_schema.system_variables.variable_name='INNODB_BUFFER_POOL_SIZE';
+---------------------------+--------------------+-------------------+
| @@innodb_buffer_pool_size | @@innodb_page_size | NUMERIC_MIN_VALUE |
+---------------------------+--------------------+-------------------+
|                 134217728 |              16384 | 5242880           |
+---------------------------+--------------------+-------------------+
1 row in set (0.002 sec)

MariaDB [(none)]>  SELECT @@innodb_buffer_pool_size, @@innodb_page_size, information_schema.system_variables.NUMERIC_MIN_VALUE FROM information_schema.system_variables where information_schema.system_variables.variable_name='INNODB_BUFFER_POOL_SIZE';

+---------------------------+--------------------+-------------------+
| @@innodb_buffer_pool_size | @@innodb_page_size | NUMERIC_MIN_VALUE |
+---------------------------+--------------------+-------------------+
|                 134217728 |              65536 | 20971520          |
+---------------------------+--------------------+-------------------+
1 row in set (0.046 sec)

MariaDB [(none)]>  SELECT @@innodb_buffer_pool_size, @@innodb_page_size, information_schema.system_variables.NUMERIC_MIN_VALUE FROM information_schema.system_variables where information_schema.system_variables.variable_name='INNODB_BUFFER_POOL_SIZE';

+---------------------------+--------------------+-------------------+
| @@innodb_buffer_pool_size | @@innodb_page_size | NUMERIC_MIN_VALUE |
+---------------------------+--------------------+-------------------+
|                 134217728 |               4096 | 2097152           |
+---------------------------+--------------------+-------------------+

Alternately if we consider the plugin init as the only location where the variables could change, we could walk though them after the plugin init in plugin_initialize.

vuvova
vuvova requested changes Jan 24, 2025
View reviewed changes
sql/sql_plugin.cc
options->def_value= (longlong) getopt_double2ulonglong((opt)->def_val); \
assert((opt)->min_val <= (opt)->max_val); \
Copy link
Member

Choose a reason for hiding this comment

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

it's the server code, you could use DBUG_ASSERT here, as far as I can see

Copy link
Member Author

Choose a reason for hiding this comment

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

Note, might need to drop the max divisible by block size - myisam_max_sort_file_size fails this as would other with a max value divisible by a power of two value.

(gdb) p *(sysvar_ulonglong_t*) opt
$4 = {flags = 132, name = 0x1793cda "max_sort_file_size", 
  comment = 0x187e40a "Don't use the fast sort index method to created index if the temporary file would get bigger than this", check = 0x9129c0 <check_func_longlong(THD*, st_mysql_sys_var*, void*, st_mysql_value*)>, 
  update = 0x912b80 <update_func_longlong(THD*, st_mysql_sys_var*, void*, void const*)>, 
  value = 0x1f78888 <myisam_max_temp_length>, def_val = 9223372036853727232, min_val = 0, 
  max_val = 9223372036854775807, blk_sz = 1048576}
(gdb) p (((sysvar_ulonglong_t*) opt)->max_val % ((sysvar_ulonglong_t*) opt)->blk_sz)
$5 = 1048575
(gdb) p 9223372036854775807 % 1048576
$6 = 1048575
(gdb) p 9223372036854775808 % 1048576
$7 = 0

sql/set_var.cc
@@ -1117,6 +1117,9 @@ int fill_sysvars(THD *thd, TABLE_LIST *tables, COND *cond)

mysql_mutex_lock(&LOCK_global_system_variables);

if (var->cast_pluginvar())
plugin_opt_set_limits(&var->option, var->cast_mysql_sys_var());

Copy link
Member

Choose a reason for hiding this comment

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

why do you need that?

Copy link
Member Author

Choose a reason for hiding this comment

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

During plugin initialization the mysq_sys_var is copied to var->option giving the value of 2M for the variable:

Old value = 0
New value = 2097152
0x000000000090d6be in plugin_opt_set_limits (options=0x732ef70, opt=0x1f6ba90 <mysql_sysvar_buffer_pool_size>)
    at /home/dan/repos/mariadb-server-10.5-quick/sql/sql_plugin.cc:3730
3730	    OPTION_SET_LIMITS(GET_ULL, options, (sysvar_ulonglong_t*) opt);
(rr) p options
$4 = (my_option *) 0x732ef70
(rr) p *options
$5 = {name = 0x732ef28 "innodb_buffer_pool_size", id = 0, 
  comment = 0x180465a "The size of the memory buffer InnoDB uses to cache data and indexes of its tables.", 
  value = 0x2132a58 <global_system_variables>, u_max_value = 0x0, typelib = 0x0, var_type = 8, arg_type = NO_ARG, def_value = 134217728, 
  min_value = 2097152, max_value = 0, sub_size = 0, block_size = 0, app_type = 0x732ef40}
(rr) bt
#0  0x000000000090d6be in plugin_opt_set_limits (options=0x732ef70, opt=0x1f6ba90 <mysql_sysvar_buffer_pool_size>)
    at /home/dan/repos/mariadb-server-10.5-quick/sql/sql_plugin.cc:3730
#1  0x000000000090cc89 in sys_var_pluginvar::sys_var_pluginvar (this=0x732ef40, chain=0x7ffc59ce9360, 
    name_arg=0x732ef28 "innodb_buffer_pool_size", p=0x731fdc0, pv=0x1f6ba90 <mysql_sysvar_buffer_pool_size>, substitute=0x0)
    at /home/dan/repos/mariadb-server-10.5-quick/sql/sql_plugin.cc:3486
#2  0x000000000091097a in test_plugin_options (tmp_root=0x7ffc59ce99a0, tmp=0x731fdc0, argc=0x2137bc8 <remaining_argc>, argv=0x72dac88)
    at /home/dan/repos/mariadb-server-10.5-quick/sql/sql_plugin.cc:4244
#3  0x000000000090741b in plugin_initialize (tmp_root=0x7ffc59ce99a0, plugin=0x731fdc0, argc=0x2137bc8 <remaining_argc>, argv=0x72dac88, 
    options_only=false) at /home/dan/repos/mariadb-server-10.5-quick/sql/sql_plugin.cc:1497
#4  0x0000000000906b9e in plugin_init (argc=0x2137bc8 <remaining_argc>, argv=0x72dac88, flags=0)
    at /home/dan/repos/mariadb-server-10.5-quick/sql/sql_plugin.cc:1762
#5  0x000000000077fe63 in init_server_components () at /home/dan/repos/mariadb-server-10.5-quick/sql/mysqld.cc:4954
#6  0x000000000077c60c in mysqld_main (argc=9, argv=0x72dac88) at /home/dan/repos/mariadb-server-10.5-quick/sql/mysqld.cc:5562
#7  0x0000000000778bb2 in main (argc=8, argv=0x7ffc59ceb5a8) at /home/dan/repos/mariadb-server-10.5-quick/sql/main.cc:25
(rr) c
Continuing.

When InnoDB initializes, it changes it min_value because the innodb_page_size in this case is 64k.

Thread 1 hit Hardware watchpoint 1: -location mysql_sysvar_buffer_pool_size.min_val

Old value = 2097152
New value = 20971520
innodb_init_params () at /home/dan/repos/mariadb-server-10.5-quick/storage/innobase/handler/ha_innodb.cc:3551
3551		if (innobase_buffer_pool_size < MYSQL_SYSVAR_NAME(buffer_pool_size).min_val) {
(rr) bt
#0  innodb_init_params () at /home/dan/repos/mariadb-server-10.5-quick/storage/innobase/handler/ha_innodb.cc:3551
#1  0x00000000010d2368 in innodb_init (p=0x73ea3e8) at /home/dan/repos/mariadb-server-10.5-quick/storage/innobase/handler/ha_innodb.cc:4028
#2  0x0000000000c7af11 in ha_initialize_handlerton (plugin_=0x731fdc0) at /home/dan/repos/mariadb-server-10.5-quick/sql/handler.cc:648
#3  0x0000000000907b62 in plugin_do_initialize (plugin=0x731fdc0, state=@0x7ffc59ce93c8: 4)
    at /home/dan/repos/mariadb-server-10.5-quick/sql/sql_plugin.cc:1453
#4  0x0000000000907481 in plugin_initialize (tmp_root=0x7ffc59ce99a0, plugin=0x731fdc0, argc=0x2137bc8 <remaining_argc>, argv=0x72dac88, 
    options_only=false) at /home/dan/repos/mariadb-server-10.5-quick/sql/sql_plugin.cc:1507
#5  0x0000000000906b9e in plugin_init (argc=0x2137bc8 <remaining_argc>, argv=0x72dac88, flags=0)
    at /home/dan/repos/mariadb-server-10.5-quick/sql/sql_plugin.cc:1762
#6  0x000000000077fe63 in init_server_components () at /home/dan/repos/mariadb-server-10.5-quick/sql/mysqld.cc:4954
#7  0x000000000077c60c in mysqld_main (argc=9, argv=0x72dac88) at /home/dan/repos/mariadb-server-10.5-quick/sql/mysqld.cc:5562
#8  0x0000000000778bb2 in main (argc=8, argv=0x7ffc59ceb5a8) at /home/dan/repos/mariadb-server-10.5-quick/sql/main.cc:25
(rr) c

Performing and information_schema select returns the original 2M value, despite the current situtation that the minimum is now 20M.

Continuing.
2025-01-28 16:18:35 0 [Note] InnoDB: innodb_page_size=65536
2025-01-28 16:18:35 0 [Note] InnoDB: !!!!!!!! UNIV_DEBUG switched on !!!!!!!!!
2025-01-28 16:18:35 0 [Note] InnoDB: Uses event mutexes
2025-01-28 16:18:35 0 [Note] InnoDB: Compressed tables use zlib 1.3.1.zlib-ng
2025-01-28 16:18:35 0 [Note] InnoDB: Number of pools: 1
2025-01-28 16:18:35 0 [Note] InnoDB: Using AVX512 instructions
2025-01-28 16:18:35 0 [Warning] InnoDB: Linux Native AIO disabled.
2025-01-28 16:18:35 0 [Note] InnoDB: Initializing buffer pool, total size = 134217728, chunk size = 134217728
2025-01-28 16:18:35 0 [Note] InnoDB: Completed initialization of buffer pool
2025-01-28 16:18:35 0 [Note] InnoDB: 128 rollback segments are active.
2025-01-28 16:18:35 0 [Note] InnoDB: Creating shared tablespace for temporary tables
2025-01-28 16:18:35 0 [Note] InnoDB: Setting file './ibtmp1' size to 12 MB. Physically writing the file full; Please wait ...
2025-01-28 16:18:35 0 [Note] InnoDB: File './ibtmp1' size is now 12 MB.
2025-01-28 16:18:35 0 [Note] InnoDB: 10.5.28 started; log sequence number 45554; transaction id 20
2025-01-28 16:18:35 0 [Note] Plugin 'FEEDBACK' is disabled.
2025-01-28 16:18:35 0 [Note] InnoDB: Loading buffer pool(s) from /tmp/build-mariadb-server-10.5-quick-debug-datadir/ib_buffer_pool
2025-01-28 16:18:35 0 [Note] InnoDB: Buffer pool(s) load completed at 250128 16:18:35
2025-01-28 16:18:35 0 [Note] Reading of all Master_info entries succeeded
2025-01-28 16:18:35 0 [Note] Added new Master_info '' to hash table
2025-01-28 16:18:35 0 [Note] sql/mariadbd: ready for connections.
Version: '10.5.28-MariaDB-debug'  socket: '/tmp/build-mariadb-server-10.5-quick-debug.sock'  port: 0  Source distribution
[New Thread 112246.112319]
[New Thread 112246.112249]
[New Thread 112246.112251]
[New Thread 112246.112252]
[New Thread 112246.112253]
[New Thread 112246.112254]
[New Thread 112246.112255]
[New Thread 112246.112256]
[New Thread 112246.112257]
[Switching to Thread 112246.112319]

Thread 13 hit Breakpoint 3, sys_var_pluginvar::cast_pluginvar (this=0x732ef40) at /home/dan/repos/mariadb-server-10.5-quick/sql/sql_plugin.cc:317
317	  sys_var_pluginvar *cast_pluginvar() override { return this; }
(rr) p option
$6 = {name = 0x732ef28 "innodb_buffer_pool_size", id = 0, 
  comment = 0x180465a "The size of the memory buffer InnoDB uses to cache data and indexes of its tables.", 
  value = 0x2132a58 <global_system_variables>, u_max_value = 0x0, typelib = 0x0, var_type = 8, arg_type = REQUIRED_ARG, def_value = 134217728, 
  min_value = 2097152, max_value = 9223372036854775807, sub_size = 0, block_size = 1048576, app_type = 0x732ef40}
(rr) bt
#0  sys_var_pluginvar::cast_pluginvar (this=0x732ef40) at /home/dan/repos/mariadb-server-10.5-quick/sql/sql_plugin.cc:317
#1  0x000000000090c26f in find_sys_var (thd=0x7fbdec002258, str=0x7fbdec019fa8 "innodb_buffer_pool_size", length=23, throw_error=false)
    at /home/dan/repos/mariadb-server-10.5-quick/sql/sql_plugin.cc:2971
#2  0x0000000000d29135 in get_system_var (thd=0x7fbdec002258, var_type=SHOW_OPT_DEFAULT, name=0x7fbe177644b0, 
    component=0x18a8000 <null_clex_str>) at /home/dan/repos/mariadb-server-10.5-quick/sql/item_func.cc:6609
#3  0x00000000008bbe09 in LEX::make_item_sysvar (this=0x7fbdec006348, thd=0x7fbdec002258, type=SHOW_OPT_DEFAULT, name=0x7fbe177644b0, 
    component=0x18a8000 <null_clex_str>) at /home/dan/repos/mariadb-server-10.5-quick/sql/sql_lex.cc:7989
#4  0x0000000000beb7f2 in LEX::make_item_sysvar (this=0x7fbdec006348, thd=0x7fbdec002258, type=SHOW_OPT_DEFAULT, name=0x7fbe177644b0)
    at /home/dan/repos/mariadb-server-10.5-quick/sql/sql_lex.h:4218
#5  0x0000000000bd16a6 in MYSQLparse (thd=0x7fbdec002258) at /home/dan/repos/mariadb-server-10.5-quick/sql/sql_yacc.yy:11401
#6  0x00000000008fcbe3 in parse_sql (thd=0x7fbdec002258, parser_state=0x7fbe17765838, creation_ctx=0x0, do_pfs_digest=true)
    at /home/dan/repos/mariadb-server-10.5-quick/sql/sql_parse.cc:10683
#7  0x00000000008e3d5a in mysql_parse (thd=0x7fbdec002258, 
    rawbuf=0x7fbdec0198b0 "SELECT @@innodb_buffer_pool_size, @@innodb_page_size, information_schema.system_variables.NUMERIC_MIN_VALUE FROM information_schema.system_variables where information_schema.system_variables.variable_"..., length=230, parser_state=0x7fbe17765838, 
    is_com_multi=false, is_next_command=false) at /home/dan/repos/mariadb-server-10.5-quick/sql/sql_parse.cc:8203
#8  0x00000000008e06ed in dispatch_command (command=COM_QUERY, thd=0x7fbdec002258, 
    packet=0x7fbdec00e7b9 "SELECT @@innodb_buffer_pool_size, @@innodb_page_size, information_schema.system_variables.NUMERIC_MIN_VALUE FROM information_schema.system_variables where information_schema.system_variables.variable_"..., packet_length=230, is_com_multi=false, 
    is_next_command=false) at /home/dan/repos/mariadb-server-10.5-quick/sql/sql_parse.cc:1891
#9  0x00000000008e4dd3 in do_command (thd=0x7fbdec002258) at /home/dan/repos/mariadb-server-10.5-quick/sql/sql_parse.cc:1375
#10 0x0000000000a9e9f8 in do_handle_one_connection (connect=0x79946c8, put_in_cache=true)
    at /home/dan/repos/mariadb-server-10.5-quick/sql/sql_connect.cc:1386
#11 0x0000000000a9e790 in handle_one_connection (arg=0x79946c8) at /home/dan/repos/mariadb-server-10.5-quick/sql/sql_connect.cc:1298
#12 0x0000000000fe30ec in pfs_spawn_thread (arg=0x78986c8) at /home/dan/repos/mariadb-server-10.5-quick/storage/perfschema/pfs.cc:2201
#13 0x00007fbe2c47d148 in start_thread () from /lib64/libc.so.6
#14 0x00007fbe2c500ed4 in clone () from /lib64/libc.so.6

At some point to make information_schema.sysvars correct we need to pull the new mysql_sysvar_buffer_pool_size.min_value into the options used by the information schema. As the innodb_buffer_pool_size is dynamic, it makes sense to have a value in the information_schema that reflects its current value.

@vuvova
Copy link
Member

vuvova commented Jan 24, 2025

btw, still no result file in the commit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vuvova vuvova vuvova requested changes

@dr-m dr-m Awaiting requested review from dr-m

Assignees
No one assigned
Labels
MariaDB Foundation Pull requests created by MariaDB Foundation
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@grooverdan @vuvova