Skip to content

MDEV-38202: make init_rpl_role visible at runtime#4904

Open
Mahmoud-kh1 wants to merge 1 commit intoMariaDB:mainfrom
Mahmoud-kh1:init_rpl_role
Open

MDEV-38202: make init_rpl_role visible at runtime#4904
Mahmoud-kh1 wants to merge 1 commit intoMariaDB:mainfrom
Mahmoud-kh1:init_rpl_role

Conversation

@Mahmoud-kh1
Copy link
Copy Markdown

@Mahmoud-kh1 Mahmoud-kh1 commented Apr 4, 2026

This PR adds a new feature to see the server’s initial replication role at runtime.

I added a new read-only status variable init_rpl_role that captures the value of rpl_status right after startup and keeps it unchanged.

Reason
Previously, init_rpl_role could be set in the cnf file but was not visible in runtime which will be helpful for investigating issues as it will allow us to see with which value mariadb was started.

Tests :
Added basic tests to verify that init_rpl_role is correctly set at startup and displayed properly.

@Mahmoud-kh1 Mahmoud-kh1 force-pushed the init_rpl_role branch 6 times, most recently from f6f6a30 to 029860e Compare April 4, 2026 17:30
@gkodinov gkodinov added the External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements. label Apr 6, 2026
Copy link
Copy Markdown
Member

@gkodinov gkodinov left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! This is a preliminary review.

Please, in addition to the notes below:

  • add a commit message to your commit explaining what it is that you do
  • update the Jira with a complete description of what the new variable is (data type, validity period, purpose), when and how it can be set, etc. : helps documenting it.
  • Make sure that there are no failing buildbot hosts.

Comment thread mysql-test/suite/sys_vars/t/init_rpl_role_variable_basic.test Outdated
Comment thread mysql-test/suite/sys_vars/t/init_rpl_role_variable_basic.test Outdated
Comment thread sql/sys_vars.cc
NOT_IN_BINLOG, ON_CHECK(check_init_string));

#ifdef HAVE_REPLICATION
static Sys_var_enum Sys_init_rpl_role(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Feel free to leave that to the final reviewer. But: is there any specific reason this should be a system variable?
It's read only at runtime and not settable via the command line/config file at startup. So, in essence, you can only read it. So why not make this a status variable?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I just make it system variable to support this syntax
SELECT @@global.init_rpl_role;
SELECT @@init_rpl_role;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

again, why? what's better about this syntax compared to SHOW GLOBAL STATUS LIKE 'init_rpl_role' ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I thought it will be just a nice bonus :) , I will turn it to status variable as you suggested.

Copy link
Copy Markdown
Contributor

@ParadoxV5 ParadoxV5 Apr 7, 2026

Choose a reason for hiding this comment

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

I stumbled upon the System & Status Variables Guide.

In general, I believe this should be a system variable, as it’s associated with a mode configuration rather than a status report.

In theory, it is possible to make --init-rpl-role mutable during runtime for recovering complex Semi-Sync networks/situations.
But in practice, --init-rpl-role doesn’t scale. (see also MDEV-34878)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FWIW, I've requested that this is a status variable for the following reasons:

  • I've assumed that a value will never be assigned directly to it.
  • Status variables are more light-weight than system variables: less locking needed etc.
  • I personally find a non-settable system variable to be an abomination and a gotcha: why would you put something non-settable in a settable variables namespace?

This said, it's your review and your codebase. So feel free to ignore the above or not.

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've assumed that a value will never be assigned directly to it.
  • Status variables are more light-weight than system variables: less locking needed etc.
  • I personally find a non-settable system variable to be an abomination and a gotcha: why would you put something non-settable in a settable variables namespace?

There are existing global read-only variables.
But yes, the system variables system should solve these missed optimizations.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Hi @ParadoxV5 I return it back to be a system variable .

Comment thread sql/sys_vars.cc Outdated
@gkodinov gkodinov self-assigned this Apr 7, 2026
@Mahmoud-kh1 Mahmoud-kh1 force-pushed the init_rpl_role branch 3 times, most recently from 1979739 to 8795f3b Compare April 7, 2026 12:17
@Mahmoud-kh1 Mahmoud-kh1 changed the title MDEV-38202: add init_rpl_role in SHOW VARIABLES MDEV-38202: make init_rpl_role visible at runtime Apr 7, 2026
@Mahmoud-kh1 Mahmoud-kh1 requested a review from gkodinov April 7, 2026 12:51
Copy link
Copy Markdown
Member

@gkodinov gkodinov left a comment

Choose a reason for hiding this comment

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

Thank you for making it a status variable!

Some cleanups below. And then it's ready.

Comment thread mysql-test/suite/sys_vars/t/init_rpl_role_variable_basic.test Outdated
Comment thread mysql-test/suite/sys_vars/t/init_rpl_role_variable_basic.test
Comment thread sql/sys_vars.cc
NOT_IN_BINLOG, ON_CHECK(check_init_string));

#ifdef HAVE_REPLICATION
static Sys_var_enum Sys_init_rpl_role(
Copy link
Copy Markdown
Contributor

@ParadoxV5 ParadoxV5 Apr 7, 2026

Choose a reason for hiding this comment

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

I stumbled upon the System & Status Variables Guide.

In general, I believe this should be a system variable, as it’s associated with a mode configuration rather than a status report.

In theory, it is possible to make --init-rpl-role mutable during runtime for recovering complex Semi-Sync networks/situations.
But in practice, --init-rpl-role doesn’t scale. (see also MDEV-34878)

@ParadoxV5 ParadoxV5 self-assigned this Apr 7, 2026
Copy link
Copy Markdown
Member

@gkodinov gkodinov left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for your work. Please stay tuned for the final review.

Copy link
Copy Markdown
Contributor

@ParadoxV5 ParadoxV5 left a comment

Choose a reason for hiding this comment

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

Thanks and well done on the first-time contribution, @Mahmoud-kh1!

Comment thread sql/sys_vars.cc Outdated
Comment thread mysql-test/suite/sys_vars/t/init_rpl_role_variable_basic.test Outdated
Comment thread mysql-test/suite/sys_vars/t/init_rpl_role_variable_basic.test
@Mahmoud-kh1 Mahmoud-kh1 force-pushed the init_rpl_role branch 2 times, most recently from 98393ff to d9aa486 Compare April 11, 2026 20:59
Comment thread sql/sys_vars.cc Outdated
Comment thread mysql-test/suite/sys_vars/t/init_rpl_role_variable_basic.test
Comment thread mysql-test/main/mysqld--help.result Outdated
@Mahmoud-kh1 Mahmoud-kh1 force-pushed the init_rpl_role branch 3 times, most recently from 37a4765 to e74a17c Compare April 11, 2026 22:09
@Mahmoud-kh1 Mahmoud-kh1 requested a review from ParadoxV5 April 11, 2026 22:13
Copy link
Copy Markdown
Contributor

@ParadoxV5 ParadoxV5 left a comment

Choose a reason for hiding this comment

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

Are you still cleaning up?

The expected results of sys_vars.sysvars_server_notembedded and main.mysqld--help tests need updates to match the new refined description.
You can have path/to/mtr --record suite_name.test_name rewrite the results if you don’t like manual copy-pastes.

While there, some small details that I missed last time:

  • Your added lines have numerous (unnecessary) trailing spaces. Please clean them up.
  • Re. the commit message
    • It still says “status variable” 😄.
    • The last paragraph of your commit message (added later?) exceeds the guidelines.
      I suggest breaking the line up:
      Example usage:
      * SHOW GLOBAL VARIABLES LIKE 'init_rpl_role'
      * SELECT @@GLOBAL.init_rpl_role;
    • Would you like to include the reviewers?
      Reviewed-by: ParadoxV5 <paradox.ver5@gmail.com>
      Reviewed-by: Georgi (Joro) Kodinov <joro@mariadb.org>
    • Nit: doubled space in … investigation issues because

Comment thread mysql-test/suite/sys_vars/t/init_rpl_role_variable_basic.test Outdated
Comment thread mysql-test/suite/sys_vars/t/init_rpl_role_variable_basic.test Outdated
@Mahmoud-kh1 Mahmoud-kh1 force-pushed the init_rpl_role branch 5 times, most recently from 839be78 to 8f83b3b Compare April 13, 2026 01:17
@Mahmoud-kh1 Mahmoud-kh1 requested a review from ParadoxV5 April 13, 2026 01:48
Problem:
The --init-rpl-role option can be set in the .cnf file or command
line to configure the server replication role at startup (MASTER or
SLAVE), but there was no way to check this value at runtime. This
made it hard to investigate issues because we could not know which
role the server was started with.

Solution:
I added a new read-only system variable init_rpl_role that is set
after startup and kept unchanged.

Example usage:
* SHOW GLOBAL VARIABLES LIKE 'init_rpl_role'
* SELECT @@GLOBAL.init_rpl_role

Reviewed-by: ParadoxV5 <paradox.ver5@gmail.com>
Reviewed-by: Georgi (Joro) Kodinov <joro@mariadb.org>
Comment thread sql/sys_vars.cc
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.

LGTM except a few remaining trailing spaces

@ParadoxV5
Copy link
Copy Markdown
Contributor

ParadoxV5 commented Apr 17, 2026

Oh, almost forgot:
Before merging new features, they also need to be approved by QA and typically pre-released in a preview build for at least one release cycle.
QA will let us know when this is good to go.

P.S.
Meanwhile, if you’d like something to do, try preparing an edit for https://github.com/mariadb-corporation/mariadb-docs!

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

Labels

External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements.

Development

Successfully merging this pull request may close these issues.

3 participants