Hi, Sachin! On Mar 15, Sachin Setiya wrote:
Hi Serg!
On Sun, Mar 12, 2017 at 11:50 PM, Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Sachin!
On Feb 10, Sachin Setiya wrote:
revision-id: b6f4d16a015fc778186ae4d2c1820a78cd9645d1 (mariadb-10.2.3-157-gb6f4d16) parent(s): 5285504857df6caf417c8a56601913a95c9f7abc author: Sachin Setiya committer: Sachin Setiya timestamp: 2017-02-10 18:24:12 +0530 message:
MDEV-10436 non-deterministic vcol does not force rbr
Problem:- Since 10.2.1 one can create virtual generated columns with non-deterministic functions. Arguably, in cases like INSERT t1 SELECT * FROM t2 if t2 contains such non-deterministic generated columns and replication mode is MIXED, the statement should be logged in the row format. But it is logged in statement format.
Some explanation of the fix would be good here. Especially if it's not trivial.
Agreed, will change it.
Why did you need set_cs_bin_format_row_if_mixed_and_nd_vcol() function and why are you doing something in end_send()? You've fixed THD::decide_logging_format(), why was that not enough?
No it wasnt , We require both Fix in thd_decide_logging_format and in end_send() For example consider this "create table t3 as select * from t1" Logging format of this can be evaluated by THD::decide_logging_format(), But in the case of insert into t3 select t1.a , t1.c , t2.b from t1 , t2 where t1.b = t2.a. We first have evaluate select condition then table->read_set will be set. Then is why set_cs_bin_format_row_if_mixed_and_nd_vcol() after end_data().
Okay. A couple of minor comments about the test: 1. please add a test for "insert into t3 select a , c from t1;" it should be logged as a statement. it is, I've tried it, but let's have it in the test. 2. You can have --sync_slave_with_master instead of a pair of --connection slave + --sync_with_master. Now the big thing. This THD::decide_logging_format() is designed to be run only once, see the comment for the commit that added it into lock_tables(), it's 41783de5496. I see that you're changing the binlog format before the first write, so that's ok. Two thoughts though: 1. it's still bad to decide on binlog format twice. 2. I don't know if it's safe to decide on binlog format that late as you're doing it. So, I'd suggest the following: to address 1) we'll decide on a binlogging format once, but late. Every table modification goes through handler::check_table_binlog_row_based_internal(). So may be we can call THD::decide_logging_format() from there? Of course, it should only be called once per statement, for for every row write, so there should be some kind of a guard. DDLs might need some special treatment, perhaps. This shoud be done in a separate refactoring commit, before MDEV-10436 bug fix. Now, 2). If the test suite will pass after moving THD::decide_logging_format that late, then, I hope, we can assume that it didn't break anything. And if that won't work, because some code might need the corect binlog format earlier - it's good that we've found it, without this test your patch would've broken it and we'd never knew. In that case, let's see how late we can do THD::decide_logging_format() without breaking anything. Regards, Sergei Chief Architect MariaDB and security@mariadb.org