MDEV-7270: Fix replicas not honoring slave_skip_errors for 1677 by OmarGamal10 · Pull Request #4653 · MariaDB/server

OmarGamal10 · 2026-02-15T00:26:31Z

Summary

This PR fixes replicas crashing on column type mismatch for row-based replication, not honoring the slave_skip_errors configuration.
The issue comments on JIRA pointed to an upstream (mysql) test, which was helpful for tracking the bug and eventually implementing the fix.
The replica now skips the event gracefully (in case the error is actually caught and the column is not convertible), logs a warning and does not abort.

Fix: MDEV-7270

gkodinov

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

Please squash all of your commits into a single one and add a commit message to it that complies with CODING_STANDARDS.md.

bnestere

Hi @OmarGamal10 !

Referencing your Zulip note

Though, I did this on main, then found out I had to do this on the oldest maintained version, which is 10.6 . Now there's some changes in the Rows_log_event::do_apply_event function, upon which I depended on with my fix, these changes do not exist in 10.6 along with some others, what should I do in this case?
...
I expect it would be to find a way to do the fix for 10.6, but the thing is the main has some helper functions and some modifications ~9 months ago-ish that makes handling the row errors much cleaner.

Let's actually keep this change on 12.3 where such changes exist (that should reduce the size of the patch). --skip-slave-errors is mostly used for testing purposes, not production, so missing this in some earlier versions won't be a big deal, and it reduces risk of accidental bugs in early GA versions. Also note, 10.6 is restricted to critical bug fixes only, so we couldn't put this in 10.6 anyway.

Sorry I wasn't able to answer earlier :( I was on vacation when you initially asked the question on Zulip, and have only just been able to take a look at your PR now.

mysql-test/suite/rpl/t/rpl_row_slave_skip_errors-slave.opt

CLAassistant · 2026-03-29T14:30:28Z

All committers have signed the CLA.

OmarGamal10 · 2026-03-29T14:36:22Z

Hi @OmarGamal10 !

Referencing your Zulip note

Though, I did this on main, then found out I had to do this on the oldest maintained version, which is 10.6 . Now there's some changes in the Rows_log_event::do_apply_event function, upon which I depended on with my fix, these changes do not exist in 10.6 along with some others, what should I do in this case?
...
I expect it would be to find a way to do the fix for 10.6, but the thing is the main has some helper functions and some modifications ~9 months ago-ish that makes handling the row errors much cleaner.

Let's actually keep this change on 12.3 where such changes exist (that should reduce the size of the patch). --skip-slave-errors is mostly used for testing purposes, not production, so missing this in some earlier versions won't be a big deal, and it reduces risk of accidental bugs in early GA versions. Also note, 10.6 is restricted to critical bug fixes only, so we couldn't put this in 10.6 anyway.

Sorry I wasn't able to answer earlier :( I was on vacation when you initially asked the question on Zulip, and have only just been able to take a look at your PR now.

No worries :). I noticed error handling changed from 10.6 to 12.3, and that comptaible_with() does not handle the error itself anymore but marks it for handling later. So I just added a bool to the thd, set in compatible_with() to skip the event in do_apply_event().

bnestere

Thank you for rebasing and updating the patch, @OmarGamal10 ! I've left some notes.

sql/rpl_utility_server.cc

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

sql/slave.h

bnestere

Thanks for the continued work, @OmarGamal10 !

I've left another round of feedback. It is quite close!

Another note for your git commit, please mention your reviewer (me):

Reviewed-by: Brandon Nesterenko <brandon.nesterenko@mariadb.com>

sql/rpl_utility_server.cc

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

bnestere · 2026-04-08T16:20:30Z

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

+call mtr.add_suppression("Slave SQL.*Error executing row event: .Table .test.t. doesn.t exist., Error_code: 1146");
+call mtr.add_suppression("Slave SQL.*Column 0 of table .test.t. cannot be converted from type.* Error_code: 1677");
+call mtr.add_suppression("The slave coordinator and worker threads are stopped, possibly leaving data in inconsistent state");
+call mtr.add_suppression("Got error 1 during COMMIT");


Nor this nor the one above it. You should only need

call mtr.add_suppression("Slave SQL.*Column 0 of table .test.t. cannot be converted from type.* Error_code: 1677");

I think we don't need any. The pattern for suppressing warnings is added to global_suppressions.

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

bnestere · 2026-04-08T16:39:29Z

sql/rpl_record.cc

+            Field *field= table->field[slave_idx];
+            set_field_to_null(field);
+          }
+        }


I'm not sure we want to always override to NULL. The slave can make it's own

ALTER TABLE <table> ALTER <broken_column> set default <whatever>;

which could be set to NULL if a user wants that behavior, and set differently otherwise. I think we can just scrap this change so the default behavior is taken.

It'd be good to test that as well so it is explicit that behavior is expected, e.g.

connection slave; ALTER TABLE t ALTER name set default "slave_default";

and also to update the git commit message.

Though I think I recall mentioning at some point "the slave would use NULL", or something like that, but I suppose here I meant whatever its default is, sorry for the miscommunication

No worries, yes I set it to NULL based on your suggestion, but it does make more sense to set it to the default field. I'll add this and add another row in the test.

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

gkodinov

LGTM. Please keep working with Brandon on the final review.

Setting slave_skip_errors to 1677 (ER_SLAVE_CONVERSION_FAILED) doesn't work as intended. The slave aborts replication on column mismatch. This fix makes it log a warning and set the problematic field to the default value gracefully. Reviewed-by: Brandon Nesterenko <brandon.nesterenko@mariadb.com>

ParadoxV5 requested a review from bnestere February 15, 2026 18:13

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

gkodinov requested changes Mar 13, 2026

View reviewed changes

bnestere added the Replication Patches involved in replication label Mar 13, 2026

OmarGamal10 force-pushed the mdev-7270 branch from 39e0f13 to 04756a2 Compare March 14, 2026 12:28

bnestere requested changes Mar 20, 2026

View reviewed changes

mysql-test/suite/rpl/t/rpl_row_slave_skip_errors-slave.opt Outdated Show resolved Hide resolved

OmarGamal10 force-pushed the mdev-7270 branch from 04756a2 to 144b7ce Compare March 29, 2026 14:30

OmarGamal10 changed the base branch from 10.6 to 12.3 March 29, 2026 14:32

OmarGamal10 requested review from bnestere and gkodinov March 29, 2026 14:52

bnestere requested changes Mar 31, 2026

View reviewed changes

OmarGamal10 force-pushed the mdev-7270 branch 4 times, most recently from ca21a82 to c32a9d0 Compare March 31, 2026 22:06

OmarGamal10 requested a review from bnestere March 31, 2026 22:14

bnestere requested changes Apr 8, 2026

View reviewed changes

gkodinov approved these changes Apr 9, 2026

View reviewed changes

OmarGamal10 force-pushed the mdev-7270 branch from c32a9d0 to a69b765 Compare April 9, 2026 23:49

OmarGamal10 force-pushed the mdev-7270 branch from a69b765 to 91292b2 Compare April 10, 2026 11:14

Uh oh!

Conversation

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

Uh oh!

Summary

Fix: MDEV-7270

Uh oh!

gkodinov left a comment

Choose a reason for hiding this comment

Uh oh!

bnestere left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

OmarGamal10 commented Mar 29, 2026

Uh oh!

bnestere left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bnestere left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bnestere Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

OmarGamal10 Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bnestere Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

OmarGamal10 Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gkodinov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

OmarGamal10 commented Feb 15, 2026 •

edited

Loading

CLAassistant commented Mar 29, 2026 •

edited

Loading

bnestere left a comment •

edited

Loading