MDEV-7394 : Making slave_skip_errors writable at runtime when slave is stopped by Mahmoud-kh1 · Pull Request #4634 · MariaDB/server

Mahmoud-kh1 · 2026-02-08T20:47:50Z

Now we can update slave_skip_errors at runtime when slaves stopped

This feature makes the slave_skip_errors system variable dynamic allowing it
to be changed at runtime when the replication slave is stopped.
Previously slave_skip_errors was read only at runtime and required a
server restart to be changed.
Runtime updates are now validated and safely rejected when the slave is
running preventing inconsistent replication state.

Key Changes

Added ON_CHECK handler to verify that updates are only allowed while the
slave is stopped.
Added ON_UPDATE handler to reinitialize the internal skip error state
when the variable is changed.
Added an rpl mtr test that verifies slave_skip_errors can be changed
dynamically when the slave is stopped and verified that updates are rejected while the slave is running.

behavior now is like that :

Feature :
MDEV-7394

CLAassistant · 2026-02-08T20:47:58Z

All committers have signed the CLA.

Mahmoud-kh1 · 2026-02-09T08:13:13Z

The following test cases fail because they assume that slave_skip_errors is read only which is no longer true which making their checks fail also
sys_vars.sysvars_server_notembedded
main.variables-notembedded
sys_vars.slave_skip_errors_basic

gkodinov

This is a preliminary review. I'd like to request changes mostly because there are tests that need to be re-recorded (and possibly fixed too).

The rest of the comments are just my own limited take on the change. Feel free to ignore and leave for the final review.

gkodinov · 2026-02-10T14:46:07Z

sql/slave.cc

-  /* Make @@slave_skip_errors show the nice human-readable value.  */
-  opt_slave_skip_errors= slave_skip_error_names;
+  /* we should not touch opt_slave_skip_errors here. we just build the printable string only. */
+  (void) opt_slave_skip_errors;


remove all these lines please.

thanks for your review I will do this

sql/slave.cc

gkodinov · 2026-02-10T15:11:19Z

mysql-test/suite/rpl/t/rpl_skip_error_dynamic.test

+START SLAVE;
+--echo # should reduce error because slave is not stopped
+--error ER_SLAVE_MUST_STOP
+SET GLOBAL slave_skip_errors = "1040";


please also add actual functionality tests for the newly set variable values.

ok I will do this

thanks for the great feedback.
I have added a new test I'd be grateful if you take a look on it , and should I fix the tests that fail because it expect the variable to be read-only or leave it for now.

gkodinov · 2026-02-11T10:24:39Z

sql/slave.cc

@@ -774,8 +774,15 @@ bool init_slave_skip_errors(const char* arg)
  if (!arg || !*arg)                            // No errors defined
    goto end;

-  if (my_bitmap_init(&slave_error_mask,0,MAX_SLAVE_ERROR))
-    DBUG_RETURN(1);
+  if (!slave_error_mask.bitmap)


This is bad pattern. I'd initialize the bitmap to empty at the start of the server as it's done now.
And I'd update its contents as needed without de-allocating and re-allocating it.

grooverdan · 2026-02-11T23:22:10Z

Thank you so much for implementing my 11 year old bug report. I'd be very grateful if you stick through the review process on this. There's a lot to keep correct in the server to implement this change.

Make slave_skip_errors dynamic so it can be changed while the slave is stopped. Attempts to change it while the slave is running are rejected with a clear error.

gkodinov

Please just fix the spacing issues. The rest is pretty much as one expects it to be. Aside from some polishing. Once the space changes are gone I will do another round and approve it.

gkodinov · 2026-02-13T11:58:41Z

sql/slave.cc


-  /* Make @@slave_skip_errors show the nice human-readable value.  */
-  opt_slave_skip_errors= slave_skip_error_names;
+


why keep 2 empty lines?

gkodinov · 2026-02-13T11:59:30Z

sql/slave.cc

    if (!(p= str2int(p, 10, 0, LONG_MAX, &err_code)))
      break;
    if (err_code < MAX_SLAVE_ERROR)
-       bitmap_set_bit(&slave_error_mask,(uint)err_code);


no white-space only changes please

gkodinov · 2026-02-13T11:59:46Z

sql/slave.cc


-end:
-  make_slave_skip_errors_printable();
+  end:


no whitespace only changes please

gkodinov · 2026-02-13T12:00:26Z

sql/slave.cc

  }
  if (ret)
    rgi->last_event_start_time= 0;
-  


no whitespace only changes please

gkodinov · 2026-02-13T12:00:35Z

sql/slave.cc

          SLAVE_WAIT_GROUP_DONE ? FALSE : TRUE;

-        DBUG_EXECUTE_IF("stop_slave_middle_group", 
+        DBUG_EXECUTE_IF("stop_slave_middle_group",


no whitespace only changes please

gkodinov · 2026-02-13T12:00:57Z

sql/slave.cc

 #endif

-  return FALSE;   
+  return FALSE;


gkodinov · 2026-02-13T12:01:05Z

sql/slave.cc

    It's necessary to adjust FD_q.(A) at this point because in the following
    course FD_q is going to be dumped to RL.
-    Generally FD_q is derived from a received FD_m (roughly FD_q := FD_m) 
+    Generally FD_q is derived from a received FD_m (roughly FD_q := FD_m)


gkodinov · 2026-02-13T12:01:31Z

sql/slave.cc

  DBUG_ASSERT(mi->rli.relay_log.description_event_for_queue->used_checksum_alg !=
              BINLOG_CHECKSUM_ALG_UNDEF);
  DBUG_ASSERT(mi->rli.relay_log.relay_log_checksum_alg !=
-              BINLOG_CHECKSUM_ALG_UNDEF); 


last whitespace remark. you get the gist: don't do that. for the whole file.

gkodinov · 2026-02-13T12:04:21Z

sql/slave.cc

+  mysql_mutex_unlock(&LOCK_global_system_variables);
+  mysql_mutex_lock(&LOCK_active_mi);
+
+  if (give_error_if_slave_running(1))


Interesting question: if the slave's not running, is there something to even access the skip errors list?
I'd guess not? I noticed that the other update methods would just check if the slave is running (and pass 0 here) and if it isn't they'd just do the update of the global values. I know what you did here is better. But I wonder if what the other methods are doing isn't enough?

it's just a flag to this function if 0 means we don't hold LOCK_acitve_mi and the function do and release after checking , but here I need to hold this lock to the end until the update is done so I pass 1 to tell it I will control this lock

gkodinov · 2026-02-13T12:08:21Z

sql/slave.cc

+    my_error(ER_SLAVE_MUST_STOP, MYF(0));
+    res= true;
+  }
+  else


Ignore please if you want, but this goes a bit against the coding style in the rest of the server.

It's usually like this:

bool err = false; if (do_something_and_it_fails()) { err = true; } if (!err && do something_else_and_it_fails()) { err = true; }

please also fix the failing tests!

I will go with your advice for sure and follow the same pattern

Removed unnecessary blank lines before the conditional check.

Mahmoud-kh1 force-pushed the dynamic-slave-skip-error branch 6 times, most recently from 87dbe8f to 3fa1017 Compare February 9, 2026 07:34

gkodinov added the External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements. label Feb 9, 2026

gkodinov requested changes Feb 10, 2026

View reviewed changes

gkodinov reviewed Feb 11, 2026

View reviewed changes

MDEV-7394 Make slave_skip_errors dynamic

440eec2

Make slave_skip_errors dynamic so it can be changed while the slave is stopped. Attempts to change it while the slave is running are rejected with a clear error.

Mahmoud-kh1 force-pushed the dynamic-slave-skip-error branch from 3fa1017 to 440eec2 Compare February 12, 2026 12:41

Merge branch 'main' into dynamic-slave-skip-error

8ac6d7b

gkodinov requested changes Feb 13, 2026

View reviewed changes

Mahmoud-kh1 added 2 commits February 13, 2026 17:26

Remove blank lines in slave.cc

33fb7cf

Removed unnecessary blank lines before the conditional check.

formatting in slave.cc

dbfa8eb


		/* Make @@slave_skip_errors show the nice human-readable value. */
		opt_slave_skip_errors= slave_skip_error_names;

Uh oh!

Conversation

Mahmoud-kh1 commented Feb 8, 2026

Uh oh!

CLAassistant commented Feb 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mahmoud-kh1 commented Feb 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gkodinov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grooverdan commented Feb 11, 2026

Uh oh!

gkodinov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

CLAassistant commented Feb 8, 2026 •

edited

Loading

Mahmoud-kh1 commented Feb 9, 2026 •

edited

Loading