MDEV-7394 : Making slave_skip_errors writable at runtime when slave is stopped

[Mahmoud-kh1]

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

• Split init_slave_skip_errors() into a bitmap-init phase (init_slave_skip_errors_bitmap()) and the existing make_slave_skip_errors_printable()
• 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]

All committers have signed the CLA.

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

[grooverdan]

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.

[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]

Please also address the buildbot failure. It seems related.

[bnestere]

Ahh okay I see. I'm pretty sure the current way you have it also leaks, as the memory used by Sys_var_charptr is pre-allocated, but the underlying pointer for opt_slave_skip_errors is over-written, and thus the old sysvar string value still exists afterwards and is never cleaned up.

Without the PREALLOCATED flag, Sys_var_charptr will auto-cleanup itself as a part of ::cleanup(). As opt_slave_skip_errors manually handles its own memory/pointer, this cleanup function should be called manually to ensure the state is correct.

What that would then look like is the logic within init_slave_skip_errors() should be split into two functions. First is the bitmap initialization from opt_slave_skip_errors->slave_error_mask, and second (which is already split) is make_slave_skip_errors_printable(). The mysqld use-case of init_slave_skip_errors doesn't need to free its opt_slave_skip_errors in-between these steps, but the sys_var update case does (I think, please verify that).

The way I see it then, is your new update_slave_skip_errors() function should manually call:

bool update_slave_skip_errors(sys_var *self, THD *thd, enum_var_type type):
  new_function_for_phase_1();
  self->cleanup();
  make_slave_skip_errors_printable();

and make_slave_skip_errors_printable() should be reverted to override opt_slave_skip_errors again.

What do you think?

[bnestere]

Given the nature of slave_skip_errors output re-ordering its input numbers, I think it would be good to add another test case with multiple error numbers input as out-of-order, and then their output shown as in-order.

[gkodinov]

Please keep a single commit in your PR. Now you have 4. And need to squash them. Otherwise, LGTM.

[bnestere]

Hi @Mahmoud-kh1 !

This is very close now, thanks for your continued work! See my last couple notes.

The next round of review should be final, so please now squash all your commits together. In your git commit message, the convention for a maximum line width is 72 characters, please follow that. Please also add at the end of your message your reviewers:

Reviewed-by: Brandon Nesterenko <brandon.nesterenko@mariadb.com>
Reviewed-by: Georgi Kodinov <joro@mariadb.org>

[bnestere]

Many of the lines in this file have whitespace at the end. Please look at your changes with git show to see this, and remove lines with such whitespace.

[bnestere]

This is still relevant

[bnestere]

Please add a comment explaining why we need to manually call self->cleanup()

[bnestere]

Hi @Mahmoud-kh1 ! The excess whitespace comment is still relevant. That's my last note. Thanks for the PR review reminder :)

[bnestere]

This is still relevant

[gkodinov]

LGTM. Please keep working with Brandon.

[bnestere]

Approved for the 13.1 preview, but slave_skip_errors_writable.test still has bad white space. It'll need to be fixed before the RC release (planned for July 28).

To explain our release process, the first public release we do is a preview release, which is when our testers do QA testing for new features. For 13.1, this is from when the preview release is assembled (probably in the next few days) through July 28. If our testers find any issues with this feature in this time, they must be fixed by July 28 to qualify for the RC release. Will you be generally available in this time to fix any possible issues?

Thanks for your continued work on this patch, @Mahmoud-kh1 !

[knielsen]

I took a look at this patch, which is AFAIU included in the new preview release:

From: Mahmoud Khaled ***@***.***> Allow slave_skip_errors to be changed at runtime when the slave is stopped. Previously it was read-only.

Unfortunately, I am not sure this is a good idea to do at all with the --slave-skip-errors option. The --slave-skip-errors option is a very dangerous option and not something to encourage the use of. Honestly I'd prefer to remove it, rather than make it dynamic :-). Well, we probably cannot remove it for backwards-compatibility reasons. But promoting it this way with a feature that makes it easier to use, is that really something we want to do? What is the motivation to do so? Secondly, even if some suitable motivation for this is given, I think we need to first fix some very serious problems in the functionality of --slave-skip-errors. For background, see here: https://jira.mariadb.org/browse/MDEV-33954 The MDEV-33954 touches a lot of points, but I think what needs fixing (in a separate commit) before further promoting the --slave-skip-errors option are these relaively simple points: 1. Errors seen during a SPECULATE_OPTIMISTIC transaction must _never_ be ignored, those transactions should be rolled back and retried as normal. 2. Some errors should not be allowed in --slave-skip-errors (nor included in --slave-skip-errors=all): ER_PRIOR_COMMIT_FAILED and other errors where the slave cannot meaningfully recover and resume replication. 3. deadlock and other temporary errors (ie. --slave_transaction_retry_errors) should never be ignored due to --slave-skip-errors (they should be retried instead). 4. An error must never be ignored by --slave-skip-errors after an ER_PRIOR_COMMIT_FAILED error has been signalled to a following transaction in parallel replication; or conversely, ER_PRIOR_COMMIT_FAILED must not be sent to a following transaction if the error causing it is going to be ignored due to --slave-skip-errors. Normally I would be reluctant to reject one patch by requiring another patch to be implemented first. However in this case I think it is appropriate, just because without the above 4 points fixed one way or another, the --slave-skip-errors option seems completely unusable. So adding the ability to set it dynamically will only lead to users getting into problems. And fixing these shouldn't be too hard. Given that, on to some detailed comments on the patch:

Split init_slave_skip_errors() into a bitmap-init phase (init_slave_skip_errors_bitmap()) and the existing make_slave_skip_errors_printable(). Added check function check_slave_skip_errors() that rejects changes while the slave is running, and update function update_slave_skip_errors() that rebuilds the bitmap and printable string on SET GLOBAL. The update callback calls self->cleanup() between the two phases to free the heap string allocated by global_update() before the pointer is overwritten with the static slave_skip_error_names[] buffer.

This part of the commit comment isn't very good. It is mechanically describing how the patch looks, instead of explaining _why_ it is written the way it is. The second part in particular seems much too trivial to be included in a commit comment. Frankly, it looks like something some LLM tool would add based on an earlier version of the patch having some problem which then needed to be corrected. It is necessary nowadays to be particularly picky about these kinds of things, as it is _very_ important that the author of a patch understands the hows and the whys of the changed in depth.

diff --git a/mysql-test/main/variables-notembedded.test b/mysql-test/main/variables-notembedded.test index 6c08f5357d0..30f1e0e3cc8 100644 --- a/mysql-test/main/variables-notembedded.test +++ b/mysql-test/main/variables-notembedded.test +SET @@global.slave_skip_errors= "OFF";

I'm curious, is "OFF" really a valid value to set? What does it mean, same as empty string? The test should SELECT the value after setting to OFF. The test should also try to set a non-existing error code and test it works correctly (whether that means to fail or to just add the numeric code to the list nevertheless I'm not sure, probably the latter). And the test case should also test what happens when a completely invalid string is supplied, which is not a list of numbers (this should presumably give an error?).

diff --git a/mysql-test/suite/rpl/t/slave_skip_errors_writable.test b/mysql-test/suite/rpl/t/slave_skip_errors_writable.test new file mode 100644 index 00000000000..a62bea2ae7f --- /dev/null +++ b/mysql-test/suite/rpl/t/slave_skip_errors_writable.test @@ -0,0 +1,69 @@ +--echo # MDEV-7394 test dynamic slave_skip_errors (writable when slaves stopped) + +--source include/master-slave.inc +connection slave; +STOP SLAVE;

This should use instead: --source include/stop_slave.inc and similar for start_slave.inc (here and below).

+SET GLOBAL slave_skip_errors = "1062";

Add a comment what is the name of the error 1062.

+connection master; +INSERT INTO t1 VALUES (2); +--source include/sync_slave_sql_with_master.inc + +connection slave; +source include/check_slave_is_running.inc; +START SLAVE; +--source include/wait_for_slave_to_start.inc

This part of the test is very strange. First check that the slave is already running, then explicitly START SLAVE? How did these lines end up in the test case?

+SET GLOBAL slave_skip_errors = "1050,1004,1104,1146";

Add comments with the names of these error codes.

+--echo # erros should be showed in sorted order +SELECT @@global.slave_skip_errors;

Typo: erros -> errors. (And should probably be "error codes should...").

+connection master; +INSERT INTO t1 VALUES (3); +--source include/sync_slave_sql_with_master.inc + +connection slave; +SELECT COUNT(*) FROM t1 WHERE id=3;

What is the purpose of this SELECT COUNT? Ie. why do this for the id=3 in particular, not for other IDs?

--- a/mysql-test/suite/sys_vars/t/slave_skip_errors_basic.test +++ b/mysql-test/suite/sys_vars/t/slave_skip_errors_basic.test @@ -10,10 +10,3 @@ show session variables like 'slave_skip_errors'; select * from information_schema.global_variables where variable_name='slave_skip_errors'; select * from information_schema.session_variables where variable_name='slave_skip_errors'; -# -# show that it's read-only -# ---error ER_INCORRECT_GLOBAL_LOCAL_VAR -set global slave_skip_errors=1; ---error ER_INCORRECT_GLOBAL_LOCAL_VAR -set session slave_skip_errors=1;

Why remove the tests completely here, instead of modifying the expected behaviour? Is it because it duplicates what's already done in main.variables-notembedded ?

diff --git a/sql/slave.cc b/sql/slave.cc index 54c6c33b2c2..629a5fb6c53 100644 --- a/sql/slave.cc +++ b/sql/slave.cc @@ -829,12 +828,14 @@ static void make_slave_skip_errors_printable(void) { /* purecov: begin tested */ memcpy(slave_skip_error_names, STRING_WITH_LEN("OFF")); + slave_skip_error_names[3]= '\0';

Aha, this worked before because the slave_skip_error_names array was inited to zeros and the memcpy() only done at server start. But then it's simpler to just use strcpy, eg. strcpy(slave_skip_error_names, "OFF").

@@ -885,14 +875,20 @@ 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 (!use_slave_mask) + { + if (my_bitmap_init(&slave_error_mask,0,MAX_SLAVE_ERROR)) + DBUG_RETURN(1); + }

This code was confusing, it took me a lot of time to understand what is going on. Please add - A comment on the variable use_slave_mask which explains it is set if the bitmap for --slave-skip-errors has been allocated (or even better rename the variable to "slave_skip_errors_bitmap_allocated" or something). - A comment here on the code explaining it is to ensure the bitmap is allocated when it is first needed.

- if (!system_charset_info->strnncoll((uchar*)arg,4,(const uchar*)"all",4)) - { + if (strlen(arg) == 3 && !system_charset_info->strnncoll((uchar*)arg,3,(const uchar*)"all",3)) {

Is there a reason to change the code like this here, if so what? Also formatting, line length is too long and the curly brace "{" goes on the next line. - Kristian.

[knielsen]

See mail comments