MDEV-30189 Add remaining replication options as system variables #3865

ParadoxV5 · 2025-03-04T03:15:45Z

The Jira issue number for this PR is: MDEV-30189 Add ~~and alias~~ remaining mariadbd replication command line options as system variables

Description

Promote the last few SQL-inaccessible replication options (command line or mariadb.cnf) as these GLOBAL read-only system variables:

`@@master_info_file`

Multi-Source Replication uses this master.info config across all connections.

`@@replicate_same_server_id`

Server IDs are a lower level than replication sources, so I made this a global variable rather than a SHOW REPLICA STATUS column.
And it’s also too esoteric (to exist in the first place, I feel), so I left out its setter (at least for now).

`@@show_slave_auth_info`

This determines whether SHOW REPLICA HOSTS includes replicas’ @@GLOBAL.report_user & @@GLOBAL.report_password.
@bnestere finds this toggle esoteric too, so I’m skipping its setter too to avoid side effects from mutability (the scope only required getters ¯\_(ツ)_/¯)

other variables listed on the JIRA issue

MDEV-25674 and MDEV-27669 covers --master-retry-count and --skip-slave-start respectively.

Do you think this patch might introduce side-effects in other parts of the server?

Due to technical limitations, the latter two options changed from no argument to optional argument.

server/sql/sys_vars.inl

Lines 465 to 469 in 3a81664

    
           { 
        
             option.var_type|= GET_BOOL; 
        
             global_var(my_bool)= def_val; 
        
             SYSVAR_ASSERT(def_val < 2); 
        
             SYSVAR_ASSERT(getopt.arg_type == OPT_ARG || getopt.id < 0);

server/include/my_getopt.h

Lines 51 to 58 in 3a81664

    
           /** 
        
             Enumeration of the my_option::arg_type attributes. 
        
             It should be noted that for historical reasons variables with the combination 
        
             arg_type=NO_ARG, my_option::var_type=GET_BOOL still accepts 
        
             arguments. This is someone counter intuitive and care should be taken 
        
             if the code is refactored. 
        
           */ 
        
           enum get_opt_arg_type { NO_ARG, OPT_ARG, REQUIRED_ARG };

Release Notes

--master-info-file, --replicate-same-server-id and --show-slave-auth-info are now available as system variables.

https://mariadb.com/kb/ pages that need changing

As stated, those Replication and Binary Logging Options have ascended to Replication and Binary Log System Variables

How can this PR be tested?

This commit includes minimal MTR tests, one for each variable.
Each file asserts the variable’s scope and read-only property.

Thread suggesting generic (QA) testing of system variables and syntaxes: #3865 (comment)

Note that --master-info-file and --show-slave-auth-info don’t have existing function tests ❗, so I wrote #3875 to backport them.

PR quality check

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

ParadoxV5

Reminders for myself:

~~mention SET @@show_slave_auth_info’s required privilege in the commit description~~ obsoleted by MDEV-30189 Add remaining replication options as system variables #3865 (comment)
~~check option IDs – that seems to be a thing~~ nope, not here
update sys.vars results – oops forgot that one

sql/sys_vars.cc

ParadoxV5 · 2025-03-04T03:39:41Z

sql/sys_vars.cc

Is there even a convention where to put new system variables?…

I don't think any specific pattern. Generally, I'd suggest (as you've already done) to keep things logically related

sql/repl_failsafe.cc

ParadoxV5 · 2025-03-04T03:58:12Z

mysql-test/suite/sys_vars/t/show_slave_auth_info_basic.test

@@ -0,0 +1,32 @@
+--source include/not_embedded.inc
+# MDEV-30189: Test minimal correctness of `@@show_slave_auth_info` itself


Since sys_vars.sysvars_server_(not)embedded dump the output of information_schema.system_variables, my idea is to treat this view as authoritative and only test that the behavior of “basic” syntaxes matches.

server/mysql-test/suite/sys_vars/inc/sysvars_server.inc

Line 21 in 3a81664

select VARIABLE_NAME,VARIABLE_SCOPE,VARIABLE_TYPE,VARIABLE_COMMENT,NUMERIC_MIN_VALUE,NUMERIC_MAX_VALUE,NUMERIC_BLOCK_SIZE,ENUM_VALUE_LIST,READ_ONLY,COMMAND_LINE_ARGUMENT

The idea puts the correctness of “alternate” syntaxes their own responsibility that they match the behavior of the “basic” syntaxes.
Those include:

SHOW GLOBAL/SESSION VARIABLES LIKE name; SELECT GLOBAL/SESSION_VALUE FROM information_schema.SYSTEM_VARIABLES WHERE VARIABLE_NAME='name'; SELECT VARIABLE_VALUE FROM information_schema.GLOBAL/SESSION_VARIABLES WHERE VARIABLE_NAME='name'; SELECT VARIABLE_VALUE FROM performance_schema.global/session_variables WHERE VARIABLE_NAME='name'; SET GLOBAL/SESSION name = value; SELECT value INTO @@GLOBAL/SESSION.name; @@GLOBAL/SESSION.name := value; SET STATEMENT name = value FOR statement; # and automatic-scope (i.e., `GLOBAL`/`SESSION`-less) variants

Both “‘basic’ syntaxes matches IS.system_variables” and “TIMTOWTDI syntaxes matches each other” can be generic tests agnostic of specific variables.
They can test every variable with a for-loop or only a random sample.

About this ideal unit testing, @mariadb-SusilBehera do you know how well they hold up currently, or does the status quo require manually testing each of @@, @@global. and @@session.?

I'd agree with the premise of the basic test, and testing alternate syntax for the same functionality isn't really related to the addition of a variable itself. Though ensuring a variable is at the proper scope (session vs global) wouldn't fall into that category (which in this patch, you have already accounted for 👍)

#3865 (comment)
I think a single unit test which covers all sysvars and checks for sysvar compliances is good enough. Beyond this, the functional tests around the new sysvars should be done in different MTR tests.

mariadb-SusilBehera

Added my review comments on MTR tests.

mysql-test/suite/sys_vars/t/master_info_file_basic.test

mysql-test/suite/sys_vars/t/replicate_same_server_id_basic.test

mysql-test/suite/sys_vars/t/show_slave_auth_info_basic.opt

mariadb-SusilBehera · 2025-03-05T10:35:21Z

mysql-test/suite/sys_vars/t/show_slave_auth_info_basic.test

+--error ER_WRONG_VALUE_FOR_VAR
+SET @@show_slave_auth_info= 2;
+--error ER_WRONG_VALUE_FOR_VAR
+SET @@show_slave_auth_info= 'lorem';


An actual SHOW SLAVE/REPLICA HOSTS result check after a SET show_slave_auth_info= ON would have been better.

--show-slave-auth-info doen’t have existing function tests.
Though I just realized: if it does, this variable would supercede the option.

master_info_file also doesn't have any existing functional tests 🤦. I do suppose we should add test cases for both variables in with this patch, to at least prove nothing is breaking.

I’ve written test for the two options at #3875.
We can merge that PR into this one, though I prefer merging through 10.11→main.

bnestere

Thanks for the patch, @ParadoxV5 , great work! Please see my few notes

sql/repl_failsafe.cc

bnestere · 2025-03-05T23:48:32Z

sql/sys_vars.cc

I don't think any specific pattern. Generally, I'd suggest (as you've already done) to keep things logically related

sql/sys_vars.cc

bnestere · 2025-03-06T00:06:34Z

mysql-test/suite/sys_vars/t/show_slave_auth_info_basic.test

+--error ER_WRONG_VALUE_FOR_VAR
+SET @@show_slave_auth_info= 2;
+--error ER_WRONG_VALUE_FOR_VAR
+SET @@show_slave_auth_info= 'lorem';


master_info_file also doesn't have any existing functional tests 🤦. I do suppose we should add test cases for both variables in with this patch, to at least prove nothing is breaking.

bnestere · 2025-03-06T00:09:26Z

mysql-test/suite/sys_vars/t/show_slave_auth_info_basic.test

@@ -0,0 +1,32 @@
+--source include/not_embedded.inc
+# MDEV-30189: Test minimal correctness of `@@show_slave_auth_info` itself


I'd agree with the premise of the basic test, and testing alternate syntax for the same functionality isn't really related to the addition of a variable itself. Though ensuring a variable is at the proper scope (session vs global) wouldn't fall into that category (which in this patch, you have already accounted for 👍)

mysql-test/suite/sys_vars/t/show_slave_auth_info_grant.test

Promote the last few SQL-inaccessible replication options (command line or `mariadb.cnf`) as these GLOBAL read-only system variables: ``` @@master_info_file @@replicate_same_server_id @@show_slave_auth_info ``` Side effect: The latter two options changed from no argument to optional argument. Quote `include/my_getopt.h`: > It should be noted that for historical reasons variables with the > combination arg_type=NO_ARG, my_option::var_type=GET_BOOL still > accepts arguments. This is someone counter intuitive and care should > be taken if the code is refactored. Reviewed-by: Brandon Nesterenko <brandon.nesterenko@mariadb.com>

ParadoxV5 · 2025-03-12T05:09:52Z

There, with this amend I undid @@show_slave_auth_info’s SESSION refactor and added @@GLOBAL/@@SESSION/@@ testing for all.
I also rebased because I forgot that four-way diff is hard and GitHub doesn’t have this feature 😅🧒

bnestere

Looks good, thanks @ParadoxV5 ! Note, you'll need to re-record sysvars_server_notembedded to account for the read-only change.

ParadoxV5 added the MariaDB Corporation label Mar 4, 2025

ParadoxV5 requested a review from bnestere March 4, 2025 03:15

ParadoxV5 commented Mar 4, 2025

View reviewed changes

mariadb-SusilBehera reviewed Mar 5, 2025

View reviewed changes

bnestere requested changes Mar 6, 2025

View reviewed changes

ParadoxV5 force-pushed the mdev-30189 branch from 008f1ea to 9cb9fa7 Compare March 12, 2025 04:57

ParadoxV5 requested a review from bnestere March 12, 2025 05:04

bnestere approved these changes Mar 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MDEV-30189 Add remaining replication options as system variables #3865

MDEV-30189 Add remaining replication options as system variables #3865

ParadoxV5 commented Mar 4, 2025 •

edited

Loading

ParadoxV5 left a comment •

edited

Loading

ParadoxV5 Mar 4, 2025

bnestere Mar 5, 2025

ParadoxV5 Mar 4, 2025

ParadoxV5 Mar 5, 2025 •

edited

Loading

bnestere Mar 6, 2025

mariadb-SusilBehera Mar 7, 2025

mariadb-SusilBehera left a comment

mariadb-SusilBehera Mar 5, 2025

ParadoxV5 Mar 5, 2025

bnestere Mar 6, 2025

ParadoxV5 Mar 7, 2025

bnestere left a comment

bnestere Mar 5, 2025

bnestere Mar 6, 2025

bnestere Mar 6, 2025

ParadoxV5 commented Mar 12, 2025

bnestere left a comment

	{
	option.var_type\|= GET_BOOL;
	global_var(my_bool)= def_val;
	SYSVAR_ASSERT(def_val < 2);
	SYSVAR_ASSERT(getopt.arg_type == OPT_ARG \|\| getopt.id < 0);

	/**
	Enumeration of the my_option::arg_type attributes.
	It should be noted that for historical reasons variables with the combination
	arg_type=NO_ARG, my_option::var_type=GET_BOOL still accepts
	arguments. This is someone counter intuitive and care should be taken
	if the code is refactored.
	*/
	enum get_opt_arg_type { NO_ARG, OPT_ARG, REQUIRED_ARG };

		@@ -0,0 +1,32 @@
		--source include/not_embedded.inc
		# MDEV-30189: Test minimal correctness of `@@show_slave_auth_info` itself

MDEV-30189 Add remaining replication options as system variables #3865

Are you sure you want to change the base?

MDEV-30189 Add remaining replication options as system variables #3865

Conversation

ParadoxV5 commented Mar 4, 2025 • edited Loading

Description

@@master_info_file

@@replicate_same_server_id

@@show_slave_auth_info

other variables listed on the JIRA issue

Do you think this patch might introduce side-effects in other parts of the server?

Release Notes

https://mariadb.com/kb/ pages that need changing

How can this PR be tested?

PR quality check

ParadoxV5 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ParadoxV5 Mar 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariadb-SusilBehera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnestere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ParadoxV5 commented Mar 12, 2025

bnestere left a comment

Choose a reason for hiding this comment

ParadoxV5 commented Mar 4, 2025 •

edited

Loading

`@@master_info_file`

`@@replicate_same_server_id`

`@@show_slave_auth_info`

ParadoxV5 left a comment •

edited

Loading

ParadoxV5 Mar 5, 2025 •

edited

Loading