sachin.setiya@mariadb.com writes:

revision-id: b60fb6daee2fcc2f433a50b5a5639065f5a46fe8 (mariadb-galera-10.0.28-8-gb60fb6d) parent(s): be430b80df0cdd4eba32df1570195721dbfd1b39 author: Sachin Setiya committer: Sachin Setiya timestamp: 2016-12-22 19:22:36 +0530 message:

MDEV-11636 Extra persistent columns on slave always gets NULL in RBR

Problem:- In replication if slave has extra persistent column then these column are not computed while applying write-set from master.

Generally, when slave and master differs there will be situations where things will not fully work. However, in this case, the result is really corrupt data (a persistent column with wrong values), and I agree this should be fixed (and the patch looks simple). I am not familiar with the code for virtual columns. I assume you looked at the normal code path for virtual columns and made similar functionality for row-based replication applier. Your code looks reasonable to me. You add code in Rows_log_event::write_row(). What about update_row? It is not tested in the testcase, you should add test of the UPDATE case (and fix the code, if needed). The testcase needs more work, see comments inline. Getting replication tests to work reliably (not generate occasional spurious errors on buildbot etc.) is quite tricky (spoken from painful experience :-). Please push this to a feature tree and make sure the test passes on all builders before pushing to main. (A useful trick also is to run on your own machine something like: ./mysql-test-run.pl rpl.rpl_alter_extra_persistent --repeat=1000 to catch at least some timing-dependent errors).

diff --git a/mysql-test/suite/rpl/t/rpl_alter_extra_persistent.test b/mysql-test/suite/rpl/t/rpl_alter_extra_persistent.test

+--sync_slave_with_master + +connection slave; +select * from t1;

Use ORDER BY a (here and below), to avoid any risk of getting different output order.

+--connection master +insert into t1 values(5); +insert into t1 values(6); + +--connection slave +sleep 2;

Never use sleep in test cases. Use an appropriate mechanism to sync the slave to the master.

+select * from t1; + +--echo #Break Unique Constraint +alter table t1 add column z4 int as (a % 6) persistent unique; + +--connection master + +--echo #entering duplicate value for slave persistent column +insert into t1 values(7); +select * from t1; + +--connection slave +select * from t1; +--query_vertical show slave status;

This will not work reliably. You cannot put SHOW SLAVE STATUS output in .result, it will not be stable between test runs. And there is a race on whether the slave has time to hit the duplicate error. For this, use the existing include/wait_for_slave_sql_error.inc file, which can reliably detect a slave failure. See comments in the file and example usage in other test files.

+ +--connection master +drop table t1; + +--connection slave +alter table t1 drop column z4 ; +start slave; +drop table t1;

This drop table should not be there (the drop table from the master will be replicated).

diff --git a/sql/log_event.cc b/sql/log_event.cc index 1cc58cd..a8f035b 100644 --- a/sql/log_event.cc +++ b/sql/log_event.cc @@ -11541,6 +11541,9 @@ Rows_log_event::write_row(rpl_group_info *rgi, DBUG_RETURN(error); }

+ if ((error= fill_extra_persistent_columns(table, &m_cols)) + DBUG_RETURN(error); +

Is similar code needed for update_row? At least, this should be tested in the test case. - Kristian.